You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2018/01/25 14:38:18 UTC
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/20395
[SPARK-23218][SQL] simplify ColumnVector.getArray
## What changes were proposed in this pull request?
`ColumnVector` is very flexible about how to implement array type. As a result `ColumnVector` has 3 abstract methods for array type: `arrayData`, `getArrayOffset`, `getArrayLength`. For example, in `WritableColumnVector` we use the first child vector as the array data vector, and store offsets and lengths in 2 arrays in the parent vector. `ArrowColumnVector` has a different implementation.
This PR simplifies `ColumnVector` by using only one abstract for array type: `getArray`.
## How was this patch tested?
existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark vector
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20395.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20395
----
commit f3ca6c73e86928d0a087fbfd36de968ae873bbe3
Author: Wenchen Fan <we...@...>
Date: 2018-01-25T14:30:57Z
simplify ColumnVector.getArray
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/20395
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/275/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/20395
LGTM with few minor comments.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/273/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/20395
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the issue:
https://github.com/apache/spark/pull/20395
LGTM pending Jenkins.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164030712
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -450,13 +439,11 @@ final boolean isNullAt(int rowId) {
}
@Override
- final int getArrayLength(int rowId) {
- return accessor.getInnerValueCountAt(rowId);
- }
-
- @Override
- final int getArrayOffset(int rowId) {
- return accessor.getOffsetBuffer().getInt(rowId * accessor.OFFSET_WIDTH);
+ final ColumnarArray getArray(int rowId) {
+ int index = rowId * accessor.OFFSET_WIDTH;
+ int start = offsets.getInt(index);
+ int end = offsets.getInt(index + accessor.OFFSET_WIDTH);
--- End diff --
I believe it is, by looking at `accessor.getInnerValueCountAt`. cc @BryanCutler for confirmation.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20395
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164030610
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchBenchmark.scala ---
@@ -479,10 +475,10 @@ object ColumnarBatchBenchmark {
Array Vector Read: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
- On Heap Read Size Only 416 / 423 393.5 2.5 1.0X
- Off Heap Read Size Only 396 / 404 413.6 2.4 1.1X
- On Heap Read Elements 2569 / 2590 63.8 15.7 0.2X
- Off Heap Read Elements 3302 / 3333 49.6 20.2 0.1X
+ On Heap Read Size Only 426 / 437 384.9 2.6 1.0X
+ Off Heap Read Size Only 406 / 421 404.0 2.5 1.0X
+ On Heap Read Elements 2636 / 2642 62.2 16.1 0.2X
+ Off Heap Read Elements 3770 / 3774 43.5 23.0 0.1X
--- End diff --
orc and parquet vectorized reader don't support array type yet.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/20395
Thanks! Merged to master/2.3
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164066651
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -450,13 +437,12 @@ final boolean isNullAt(int rowId) {
}
@Override
- final int getArrayLength(int rowId) {
- return accessor.getInnerValueCountAt(rowId);
- }
-
- @Override
- final int getArrayOffset(int rowId) {
- return accessor.getOffsetBuffer().getInt(rowId * accessor.OFFSET_WIDTH);
+ final ColumnarArray getArray(int rowId) {
+ ArrowBuf offsets = accessor.getOffsetBuffer();
+ int index = rowId * accessor.OFFSET_WIDTH;
+ int start = offsets.getInt(index);
+ int end = offsets.getInt(index + accessor.OFFSET_WIDTH);
--- End diff --
At the last rowId, what `offsets.getInt(index + accessor.OFFSET_WIDTH)` returns?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164067505
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java ---
@@ -602,7 +603,17 @@ public final int appendStruct(boolean isNull) {
// `WritableColumnVector` puts the data of array in the first child column vector, and puts the
// array offsets and lengths in the current column vector.
@Override
- public WritableColumnVector arrayData() { return childColumns[0]; }
+ public final ColumnarArray getArray(int rowId) {
+ return new ColumnarArray(arrayData(), getArrayOffset(rowId), getArrayLength(rowId));
+ }
+
+ public WritableColumnVector arrayData() {
+ return childColumns[0];
+ }
+
+ public abstract int getArrayLength(int rowId);
+
+ public abstract int getArrayOffset(int rowId);
--- End diff --
Shall we make these two methods private/protected?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164047732
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -433,10 +418,14 @@ final long getLong(int rowId) {
private static class ArrayAccessor extends ArrowVectorAccessor {
private final ListVector accessor;
+ private final ArrowBuf offsets;
+ private final ArrowColumnVector arrayData;
ArrayAccessor(ListVector vector) {
super(vector);
this.accessor = vector;
+ this.offsets = vector.getOffsetBuffer();
--- End diff --
Maybe `vector.getOffsetBuffer()` will change when loading the next batch so we can't reuse the `ArrowBuf`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86684/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164086579
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -450,13 +437,12 @@ final boolean isNullAt(int rowId) {
}
@Override
- final int getArrayLength(int rowId) {
- return accessor.getInnerValueCountAt(rowId);
- }
-
- @Override
- final int getArrayOffset(int rowId) {
- return accessor.getOffsetBuffer().getInt(rowId * accessor.OFFSET_WIDTH);
+ final ColumnarArray getArray(int rowId) {
+ ArrowBuf offsets = accessor.getOffsetBuffer();
+ int index = rowId * accessor.OFFSET_WIDTH;
+ int start = offsets.getInt(index);
+ int end = offsets.getInt(index + accessor.OFFSET_WIDTH);
--- End diff --
the end offset of the last record. see https://arrow.apache.org/docs/memory_layout.html#list-type
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86691 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86691/testReport)** for PR 20395 at commit [`957d040`](https://github.com/apache/spark/commit/957d040a47c5a60bdbb19d0490dc748f66680376).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86691 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86691/testReport)** for PR 20395 at commit [`957d040`](https://github.com/apache/spark/commit/957d040a47c5a60bdbb19d0490dc748f66680376).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164088592
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java ---
@@ -602,7 +603,17 @@ public final int appendStruct(boolean isNull) {
// `WritableColumnVector` puts the data of array in the first child column vector, and puts the
// array offsets and lengths in the current column vector.
@Override
- public WritableColumnVector arrayData() { return childColumns[0]; }
+ public final ColumnarArray getArray(int rowId) {
+ return new ColumnarArray(arrayData(), getArrayOffset(rowId), getArrayLength(rowId));
+ }
+
+ public WritableColumnVector arrayData() {
+ return childColumns[0];
+ }
+
+ public abstract int getArrayLength(int rowId);
+
+ public abstract int getArrayOffset(int rowId);
--- End diff --
Ok. Make sense.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86641 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86641/testReport)** for PR 20395 at commit [`f3ca6c7`](https://github.com/apache/spark/commit/f3ca6c73e86928d0a087fbfd36de968ae873bbe3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86691/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/20395
cc @hvanhovell @ueshin @viirya @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86695/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164086245
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java ---
@@ -602,7 +603,17 @@ public final int appendStruct(boolean isNull) {
// `WritableColumnVector` puts the data of array in the first child column vector, and puts the
// array offsets and lengths in the current column vector.
@Override
- public WritableColumnVector arrayData() { return childColumns[0]; }
+ public final ColumnarArray getArray(int rowId) {
+ return new ColumnarArray(arrayData(), getArrayOffset(rowId), getArrayLength(rowId));
+ }
+
+ public WritableColumnVector arrayData() {
+ return childColumns[0];
+ }
+
+ public abstract int getArrayLength(int rowId);
+
+ public abstract int getArrayOffset(int rowId);
--- End diff --
`WritableColumnVector` is an internal class, so I want to keep the visibility wider for the ease of use.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r163965864
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchBenchmark.scala ---
@@ -479,10 +475,10 @@ object ColumnarBatchBenchmark {
Array Vector Read: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
- On Heap Read Size Only 416 / 423 393.5 2.5 1.0X
- Off Heap Read Size Only 396 / 404 413.6 2.4 1.1X
- On Heap Read Elements 2569 / 2590 63.8 15.7 0.2X
- Off Heap Read Elements 3302 / 3333 49.6 20.2 0.1X
+ On Heap Read Size Only 426 / 437 384.9 2.6 1.0X
+ Off Heap Read Size Only 406 / 421 404.0 2.5 1.0X
+ On Heap Read Elements 2636 / 2642 62.2 16.1 0.2X
+ Off Heap Read Elements 3770 / 3774 43.5 23.0 0.1X
--- End diff --
Thank you for updating this in the PR. It seems to cause a little performance degradation.
Could you update `OrcReadBenchmark` and `FilterPushdownBenchmark`, too?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86641 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86641/testReport)** for PR 20395 at commit [`f3ca6c7`](https://github.com/apache/spark/commit/f3ca6c73e86928d0a087fbfd36de968ae873bbe3).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164160869
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchBenchmark.scala ---
@@ -479,10 +475,10 @@ object ColumnarBatchBenchmark {
Array Vector Read: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
- On Heap Read Size Only 416 / 423 393.5 2.5 1.0X
- Off Heap Read Size Only 396 / 404 413.6 2.4 1.1X
- On Heap Read Elements 2569 / 2590 63.8 15.7 0.2X
- Off Heap Read Elements 3302 / 3333 49.6 20.2 0.1X
+ On Heap Read Size Only 426 / 437 384.9 2.6 1.0X
+ Off Heap Read Size Only 406 / 421 404.0 2.5 1.0X
+ On Heap Read Elements 2636 / 2642 62.2 16.1 0.2X
+ Off Heap Read Elements 3770 / 3774 43.5 23.0 0.1X
--- End diff --
Right. I see.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86684 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86684/testReport)** for PR 20395 at commit [`f3ca6c7`](https://github.com/apache/spark/commit/f3ca6c73e86928d0a087fbfd36de968ae873bbe3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86695 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86695/testReport)** for PR 20395 at commit [`957d040`](https://github.com/apache/spark/commit/957d040a47c5a60bdbb19d0490dc748f66680376).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86684 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86684/testReport)** for PR 20395 at commit [`f3ca6c7`](https://github.com/apache/spark/commit/f3ca6c73e86928d0a087fbfd36de968ae873bbe3).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by BryanCutler <gi...@git.apache.org>.
Github user BryanCutler commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164249345
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -450,13 +439,11 @@ final boolean isNullAt(int rowId) {
}
@Override
- final int getArrayLength(int rowId) {
- return accessor.getInnerValueCountAt(rowId);
- }
-
- @Override
- final int getArrayOffset(int rowId) {
- return accessor.getOffsetBuffer().getInt(rowId * accessor.OFFSET_WIDTH);
+ final ColumnarArray getArray(int rowId) {
+ int index = rowId * accessor.OFFSET_WIDTH;
+ int start = offsets.getInt(index);
+ int end = offsets.getInt(index + accessor.OFFSET_WIDTH);
--- End diff --
yes, the code is correct. `offsetBuffer` will be sized to at least `(numRows + 1) * accessor.OFFSET_WIDTH`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20395
**[Test build #86695 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86695/testReport)** for PR 20395 at commit [`957d040`](https://github.com/apache/spark/commit/957d040a47c5a60bdbb19d0490dc748f66680376).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164086846
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ColumnVector.java ---
@@ -182,57 +187,54 @@
}
/**
- * Returns the length of the array for rowId.
- */
- public abstract int getArrayLength(int rowId);
-
- /**
- * Returns the offset of the array for rowId.
- */
- public abstract int getArrayOffset(int rowId);
-
- /**
- * Returns the struct for rowId.
+ * Returns the struct type value for rowId.
+ *
+ * To support struct type, implementations must implement {@link #getChild(int)} and make this
+ * vector a tree structure. The number of child vectors must be same as the number of fields of
+ * the struct type, and each child vector is responsible to store the data for its corresponding
+ * struct field.
*/
public final ColumnarRow getStruct(int rowId) {
return new ColumnarRow(this, rowId);
}
/**
- * Returns the array for rowId.
+ * Returns the array type value for rowId.
+ *
+ * To support array type, implementations must construct an {@link ColumnarArray} and return it in
+ * this method. {@link ColumnarArray} requires a {@link ColumnVector} that stores the data of all
--- End diff --
The comments here are mostly about how to construct the `ColumnarArray`, but about `ColumnarArray` itself, so I'd like to keep them here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/268/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164049147
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -433,10 +418,14 @@ final long getLong(int rowId) {
private static class ArrayAccessor extends ArrowVectorAccessor {
private final ListVector accessor;
+ private final ArrowBuf offsets;
+ private final ArrowColumnVector arrayData;
ArrayAccessor(ListVector vector) {
super(vector);
this.accessor = vector;
+ this.offsets = vector.getOffsetBuffer();
--- End diff --
good catch!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/232/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r163915680
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java ---
@@ -450,13 +439,11 @@ final boolean isNullAt(int rowId) {
}
@Override
- final int getArrayLength(int rowId) {
- return accessor.getInnerValueCountAt(rowId);
- }
-
- @Override
- final int getArrayOffset(int rowId) {
- return accessor.getOffsetBuffer().getInt(rowId * accessor.OFFSET_WIDTH);
+ final ColumnarArray getArray(int rowId) {
+ int index = rowId * accessor.OFFSET_WIDTH;
+ int start = offsets.getInt(index);
+ int end = offsets.getInt(index + accessor.OFFSET_WIDTH);
--- End diff --
Does this code work when `rowId == arrayLength`? In other words, is the length of `offsetBuffer` equal to `(rowId + 1) * accessor.OFFSET_WIDTH`?
cc: @ueshin
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20395
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86641/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20395: [SPARK-23218][SQL] simplify ColumnVector.getArray
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20395#discussion_r164065557
--- Diff: sql/core/src/main/java/org/apache/spark/sql/vectorized/ColumnVector.java ---
@@ -182,57 +187,54 @@
}
/**
- * Returns the length of the array for rowId.
- */
- public abstract int getArrayLength(int rowId);
-
- /**
- * Returns the offset of the array for rowId.
- */
- public abstract int getArrayOffset(int rowId);
-
- /**
- * Returns the struct for rowId.
+ * Returns the struct type value for rowId.
+ *
+ * To support struct type, implementations must implement {@link #getChild(int)} and make this
+ * vector a tree structure. The number of child vectors must be same as the number of fields of
+ * the struct type, and each child vector is responsible to store the data for its corresponding
+ * struct field.
*/
public final ColumnarRow getStruct(int rowId) {
return new ColumnarRow(this, rowId);
}
/**
- * Returns the array for rowId.
+ * Returns the array type value for rowId.
+ *
+ * To support array type, implementations must construct an {@link ColumnarArray} and return it in
+ * this method. {@link ColumnarArray} requires a {@link ColumnVector} that stores the data of all
--- End diff --
Shall we move the details of `ColumnarArray` implementation to `ColumnarArray` class?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org