[SPARK-37728][SQL] Reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException #35002

yimin-yang · 2021-12-23T13:06:25Z

What changes were proposed in this pull request?

When an OrcColumnarBatchReader is created, method initBatch will be called only once. In method initBatch:

orcVectorWrappers[i] = OrcColumnVectorUtils.toOrcColumnVector(dt, wrap.batch().cols[colId]);

When the second argument of toOrcColumnVector is a ListColumnVector/MapColumnVector, orcVectorWrappers[i] is initialized with the ListColumnVector or MapColumnVector's offsets and lengths.

However, when method nextBatch of OrcColumnarBatchReader is called, method ensureSize of ColumnVector (and its subclasses, like MultiValuedColumnVector) could be called, then the ListColumnVector/MapColumnVector's offsets and lengths could refer to new array objects. This could result in the ArrayIndexOutOfBoundsException.

This PR makes OrcArrayColumnVector.getArray and OrcMapColumnVector.getMap always get offsets and lengths from the underlying ColumnVector, which can resolve this issue.

Why are the changes needed?

Bugfix

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the CIs with the newly added test case.

yimin-yang · 2021-12-23T13:30:47Z

@c21 Could you take a look when you are free? Thanks! Looking forward to your feedback.

c21 · 2021-12-23T20:56:05Z

@yym1995 thank you submitting a fix! Could you help add a unit test case as well?

HyukjinKwon · 2021-12-24T02:04:42Z

ok to test

HyukjinKwon · 2021-12-24T02:04:47Z

cc @dongjoon-hyun FYI

SparkQA · 2021-12-24T03:04:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/51020/

SparkQA · 2021-12-24T04:02:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/51020/

yimin-yang · 2021-12-24T04:25:10Z

@yym1995 thank you submitting a fix! Could you help add a unit test case as well?

@c21 I just added a unit test case. Please take a look, thanks!

SparkQA · 2021-12-24T05:40:44Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/51025/

SparkQA · 2021-12-24T06:41:19Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/51025/

SparkQA · 2021-12-24T07:19:51Z

Test build #146545 has finished for PR 35002 at commit f975af9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-12-24T10:00:26Z

Test build #146550 has finished for PR 35002 at commit ec2e0db.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

c21

Thank you @yym1995 for the fix! Having some comments for code structure, but the fix is valid.

c21 · 2021-12-27T00:46:48Z

sql/core/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcArrayColumnVector.java

@@ -31,26 +32,27 @@
 */
 public class OrcArrayColumnVector extends OrcColumnVector {
  private final OrcColumnVector data;
-  private final long[] offsets;
-  private final long[] lengths;
+  private ListColumnVector listData;

  OrcArrayColumnVector(


I think we don't need to store another copy of vector here. We can change OrcColumnVector.baseData from private to protected, and just use baseData to get offset and length. So how about defining the constructor like below:

OrcArrayColumnVector( DataType type, ListColumnVector vector, OrcColumnVector data) { super(type, vector); this.data = data; }

c21 · 2021-12-27T00:52:47Z

sql/core/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcMapColumnVector.java

@@ -32,28 +33,30 @@
 public class OrcMapColumnVector extends OrcColumnVector {
  private final OrcColumnVector keys;
  private final OrcColumnVector values;
-  private final long[] offsets;
-  private final long[] lengths;
+  private MapColumnVector mapData;

  OrcMapColumnVector(


Similarly we can define OrcMapColumnVector constructor as below:

OrcMapColumnVector( DataType type, MapColumnVector vector, OrcColumnVector keys, OrcColumnVector values) { super(type, vector); this.keys = keys; this.values = values; }

c21 · 2021-12-27T00:54:18Z

sql/core/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcMapColumnVector.java

  }

  @Override
  public ColumnarMap getMap(int ordinal) {
-    return new ColumnarMap(keys, values, (int) offsets[ordinal], (int) lengths[ordinal]);
+    return new ColumnarMap(keys, values, (int) mapData.offsets[ordinal],


public ColumnarMap getMap(int ordinal) { int offset = (int) ((MapColumnVector) baseData).offsets[ordinal]; int length = (int) ((MapColumnVector) baseData).lengths[ordinal]; return new ColumnarMap(keys, values, offset, length); }

c21 · 2021-12-27T00:56:40Z

sql/core/src/main/java/org/apache/spark/sql/execution/datasources/orc/OrcArrayColumnVector.java

  }

  @Override
  public ColumnarArray getArray(int rowId) {
-    return new ColumnarArray(data, (int) offsets[rowId], (int) lengths[rowId]);
+    return new ColumnarArray(data, (int) listData.offsets[rowId], (int) listData.lengths[rowId]);


We can just use baseData here:

public ColumnarArray getArray(int rowId) { int offset = (int) ((ListColumnVector) baseData).offsets[rowId]; int length = (int) ((ListColumnVector) baseData).lengths[rowId]; return new ColumnarArray(data, offset, length); }

yimin-yang · 2021-12-27T02:55:49Z

@c21 Thank you for the feedback! I have already changed the code structure.

yimin-yang · 2021-12-27T03:06:46Z

@dongjoon-hyun This PR fixed a bug in ORC vectorized reader. @c21 has reviewed this PR, and I have improved the code according to the feedback. I was wondering if you could merge this PR, thanks!

c21 · 2021-12-27T03:07:54Z

LGTM with pending CI tests. cc @viirya and @cloud-fan as well.

c21 · 2021-12-27T03:08:27Z

retest this please

cloud-fan · 2021-12-27T11:32:51Z

The fix LGTM. Please click the failed Github Action, follow the instructions to fix the issues.

… ORC files

viirya

The fix looks good. Pending CI.

viirya · 2021-12-27T20:26:14Z

GA seems unstable now. You can submit an empty commit to re-trigger it.

viirya · 2021-12-27T20:27:29Z

Tested manually. Just follow the bug reproduction steps in https://issues.apache.org/jira/browse/SPARK-37728 .

As you added an unit test, you can modify the PR description too.

dongjoon-hyun

+1, LGTM (Pending CI)

yimin-yang · 2021-12-28T06:05:14Z

Now all checks have passed. cc @cloud-fan @dongjoon-hyun @HyukjinKwon @viirya

dongjoon-hyun · 2021-12-28T06:10:29Z

I revised the PR description. Merged to master.
Thank you, @yym1995 and all!

dongjoon-hyun · 2021-12-28T06:12:50Z

Welcome to the Apache Spark community, @yym1995 .
I added you to the Apache Spark contributor group and assigned SPARK-37728 to you.
Could you make three backporting PRs to branch-3.2 and branch-3.1 and branch-3.0? We need to pass the UTs on those branches.

yimin-yang · 2021-12-28T06:23:25Z

Welcome to the Apache Spark community, @yym1995 . I added you to the Apache Spark contributor group and assigned SPARK-37728 to you. Could you make three backporting PRs to branch-3.2 and branch-3.1 and branch-3.0? We need to pass the UTs on those branches.

OK, will do.

c21 · 2021-12-28T07:37:47Z

@dongjoon-hyun - thanks for merging. We only need backport to branch-3.2, right? The related code was only introduced in 3.2 branch (in PR for ORC vectorized reader of nested column)

dongjoon-hyun · 2021-12-28T07:44:42Z

Yes, we only need to backport to the branches where the test case fails.

BTW, @yym1995 , if branch-3.2 is the only branch, you should remove Affected Version of SPARK-37728. Currently, it has 3.0.2 and 3.1.2.

…can cause ArrayIndexOutOfBoundsException When an OrcColumnarBatchReader is created, method initBatch will be called only once. In method initBatch: `orcVectorWrappers[i] = OrcColumnVectorUtils.toOrcColumnVector(dt, wrap.batch().cols[colId]);` When the second argument of toOrcColumnVector is a ListColumnVector/MapColumnVector, orcVectorWrappers[i] is initialized with the ListColumnVector or MapColumnVector's offsets and lengths. However, when method nextBatch of OrcColumnarBatchReader is called, method ensureSize of ColumnVector (and its subclasses, like MultiValuedColumnVector) could be called, then the ListColumnVector/MapColumnVector's offsets and lengths could refer to new array objects. This could result in the ArrayIndexOutOfBoundsException. This PR makes OrcArrayColumnVector.getArray and OrcMapColumnVector.getMap always get offsets and lengths from the underlying ColumnVector, which can resolve this issue. Bugfix No Pass the CIs with the newly added test case. Closes apache#35002 from yym1995/fix-nested. Lead-authored-by: Yimin <[email protected]> Co-authored-by: Yimin <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ader can cause ArrayIndexOutOfBoundsException ### What changes were proposed in this pull request? This is a backport of #35002 . When an OrcColumnarBatchReader is created, method initBatch will be called only once. In method initBatch: `orcVectorWrappers[i] = OrcColumnVectorUtils.toOrcColumnVector(dt, wrap.batch().cols[colId]);` When the second argument of toOrcColumnVector is a ListColumnVector/MapColumnVector, orcVectorWrappers[i] is initialized with the ListColumnVector or MapColumnVector's offsets and lengths. However, when method nextBatch of OrcColumnarBatchReader is called, method ensureSize of ColumnVector (and its subclasses, like MultiValuedColumnVector) could be called, then the ListColumnVector/MapColumnVector's offsets and lengths could refer to new array objects. This could result in the ArrayIndexOutOfBoundsException. This PR makes OrcArrayColumnVector.getArray and OrcMapColumnVector.getMap always get offsets and lengths from the underlying ColumnVector, which can resolve this issue. ### Why are the changes needed? Bugfix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass the CIs with the newly added test case. Closes #35038 from yym1995/branch-3.2. Lead-authored-by: Yimin <[email protected]> Co-authored-by: Yimin Yang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ader can cause ArrayIndexOutOfBoundsException ### What changes were proposed in this pull request? This is a backport of apache#35002 . When an OrcColumnarBatchReader is created, method initBatch will be called only once. In method initBatch: `orcVectorWrappers[i] = OrcColumnVectorUtils.toOrcColumnVector(dt, wrap.batch().cols[colId]);` When the second argument of toOrcColumnVector is a ListColumnVector/MapColumnVector, orcVectorWrappers[i] is initialized with the ListColumnVector or MapColumnVector's offsets and lengths. However, when method nextBatch of OrcColumnarBatchReader is called, method ensureSize of ColumnVector (and its subclasses, like MultiValuedColumnVector) could be called, then the ListColumnVector/MapColumnVector's offsets and lengths could refer to new array objects. This could result in the ArrayIndexOutOfBoundsException. This PR makes OrcArrayColumnVector.getArray and OrcMapColumnVector.getMap always get offsets and lengths from the underlying ColumnVector, which can resolve this issue. ### Why are the changes needed? Bugfix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass the CIs with the newly added test case. Closes apache#35038 from yym1995/branch-3.2. Lead-authored-by: Yimin <[email protected]> Co-authored-by: Yimin Yang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ader can cause ArrayIndexOutOfBoundsException ### What changes were proposed in this pull request? This is a backport of apache#35002 . When an OrcColumnarBatchReader is created, method initBatch will be called only once. In method initBatch: `orcVectorWrappers[i] = OrcColumnVectorUtils.toOrcColumnVector(dt, wrap.batch().cols[colId]);` When the second argument of toOrcColumnVector is a ListColumnVector/MapColumnVector, orcVectorWrappers[i] is initialized with the ListColumnVector or MapColumnVector's offsets and lengths. However, when method nextBatch of OrcColumnarBatchReader is called, method ensureSize of ColumnVector (and its subclasses, like MultiValuedColumnVector) could be called, then the ListColumnVector/MapColumnVector's offsets and lengths could refer to new array objects. This could result in the ArrayIndexOutOfBoundsException. This PR makes OrcArrayColumnVector.getArray and OrcMapColumnVector.getMap always get offsets and lengths from the underlying ColumnVector, which can resolve this issue. ### Why are the changes needed? Bugfix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass the CIs with the newly added test case. Closes apache#35038 from yym1995/branch-3.2. Lead-authored-by: Yimin <[email protected]> Co-authored-by: Yimin Yang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 5f9b92c) Signed-off-by: Dongjoon Hyun <[email protected]>

github-actions bot added the SQL label Dec 23, 2021

HyukjinKwon changed the title ~~[SPARK-37728][SQL] reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException~~ [SPARK-37728][SQL] Reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException Dec 24, 2021

c21 reviewed Dec 27, 2021

View reviewed changes

yimin-yang force-pushed the fix-nested branch 2 times, most recently from 5cd704b to ee293e3 Compare December 27, 2021 12:21

fixed ArrayIndexOutOfBoundsException when reading nested columns from…

0805ece

… ORC files

yimin-yang force-pushed the fix-nested branch from ee293e3 to 0805ece Compare December 27, 2021 12:30

viirya approved these changes Dec 27, 2021

View reviewed changes

dongjoon-hyun approved these changes Dec 27, 2021

View reviewed changes

Merge branch 'apache:master' into fix-nested

8dce12d

dongjoon-hyun closed this in d245aef Dec 28, 2021

yimin-yang mentioned this pull request Dec 28, 2021

[SPARK-37728][SQL][3.2] Reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException #35038

Closed

yimin-yang deleted the fix-nested branch December 29, 2021 06:56

[SPARK-37728][SQL] Reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException #35002

[SPARK-37728][SQL] Reading nested columns with ORC vectorized reader can cause ArrayIndexOutOfBoundsException #35002

Uh oh!

Conversation

yimin-yang commented Dec 23, 2021 • edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

yimin-yang commented Dec 23, 2021

Uh oh!

c21 commented Dec 23, 2021

Uh oh!

HyukjinKwon commented Dec 24, 2021

Uh oh!

HyukjinKwon commented Dec 24, 2021

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

yimin-yang commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

SparkQA commented Dec 24, 2021

Uh oh!

c21 left a comment

Choose a reason for hiding this comment

Uh oh!

c21 Dec 27, 2021

Choose a reason for hiding this comment

Uh oh!

c21 Dec 27, 2021

Choose a reason for hiding this comment

Uh oh!

c21 Dec 27, 2021

Choose a reason for hiding this comment

Uh oh!

c21 Dec 27, 2021

Choose a reason for hiding this comment

Uh oh!

yimin-yang commented Dec 27, 2021

Uh oh!

yimin-yang commented Dec 27, 2021

Uh oh!

c21 commented Dec 27, 2021

Uh oh!

c21 commented Dec 27, 2021

Uh oh!

cloud-fan commented Dec 27, 2021

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Dec 27, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

yimin-yang commented Dec 28, 2021

Uh oh!

dongjoon-hyun commented Dec 28, 2021

Uh oh!

dongjoon-hyun commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yimin-yang commented Dec 28, 2021

Uh oh!

c21 commented Dec 28, 2021

yimin-yang commented Dec 23, 2021 •

edited by dongjoon-hyun

Loading

yimin-yang commented Dec 24, 2021 •

edited

Loading

viirya commented Dec 27, 2021 •

edited

Loading

dongjoon-hyun commented Dec 28, 2021 •

edited

Loading

dongjoon-hyun commented Dec 28, 2021 •

edited

Loading