[SPARK-23090][SQL] polish ColumnVector #20277

cloud-fan · 2018-01-16T16:43:20Z

What changes were proposed in this pull request?

Several improvements:

provide a default implementation for the batch get methods
rename getChildColumn to getChild, which is more concise
remove getStruct(int, int), it's only used to simplify the codegen, which is an internal thing, we should not add a public API for this purpose.

How was this patch tested?

existing tests

cloud-fan · 2018-01-16T16:43:51Z

cc @hvanhovell @kiszk @viirya @gatorsmile

cloud-fan · 2018-01-16T16:45:39Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java


  private final ArrowVectorAccessor accessor;
  private ArrowColumnVector[] childColumns;

-  private void ensureAccessible(int index) {


ColumnVector is a performance critical place, we don't need index checking here, like other column vector implementations.

SparkQA · 2018-01-16T16:47:52Z

Test build #86179 has started for PR 20277 at commit ba7ca0f.

hvanhovell · 2018-01-16T17:40:57Z

@cloud-fan did you do some benchmarks? I'd like to make sure that the abstract class to interface change does not negatively impact performance.

cloud-fan · 2018-01-17T05:06:21Z

@hvanhovell good idea. I ran the ColumnarBatchBenchmark and found getArray has a regression as it's not final anymore. I've reverted the interface stuff and ColumnVector is still abstract class now.

SparkQA · 2018-01-17T05:12:06Z

Test build #86240 has finished for PR 20277 at commit 08d06a7.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-01-17T05:17:02Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java

-    ensureAccessible(index, 1);
-  }
-
-  private void ensureAccessible(int index, int count) {


ColumnVector is a performance critical place, we don't need index checking here, like other column vector implementations.

I agree with this in non-debug version. Can we add assert of this check at each caller site for debugging?

p.s. Sorry for slow reviews since I am on vacation this week.

How about we do it later? We need to find a central place to put this check, instead of doing it in every implementation.

It is good to do it later. I agree that we do the same check at one place.

SparkQA · 2018-01-17T05:32:03Z

Test build #86222 has finished for PR 20277 at commit 3cba91b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class OrcColumnVector implements org.apache.spark.sql.vectorized.ColumnVector
public abstract class WritableColumnVector implements ColumnVector
public final class ArrowColumnVector implements ColumnVector

SparkQA · 2018-01-17T08:05:01Z

Test build #86246 has finished for PR 20277 at commit f3f9d5e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-17T08:05:02Z

Test build #86245 has finished for PR 20277 at commit 77e8c4b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-17T08:05:02Z

Test build #86242 has finished for PR 20277 at commit bc6d0af.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-01-17T08:21:36Z

retest this please

SparkQA · 2018-01-17T11:36:06Z

Test build #86254 has finished for PR 20277 at commit f3f9d5e.

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

SparkQA · 2018-01-18T13:26:30Z

Test build #86340 has finished for PR 20277 at commit 212c841.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-19T03:19:48Z

Test build #86371 has finished for PR 20277 at commit eccdca1.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2018-01-19T03:36:16Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java

@@ -53,166 +41,83 @@ public int numNulls() {
  @Override
  public void close() {
    if (childColumns != null) {
-      for (int i = 0; i < childColumns.length; i++) {
-        childColumns[i].close();
+      for (ArrowColumnVector childColumn : childColumns) {


Will this be faster?

Should also apply similar change to WritableColumnVector.close()

the performance is same, it's just a more standard way to iterate an array in java

jiangxb1987 · 2018-01-19T03:40:26Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ColumnVector.java

  /**
   * Returns the array for rowId.
   */
  public final ColumnarArray getArray(int rowId) {
-    return new ColumnarArray(arrayData(), getArrayOffset(rowId), getArrayLength(rowId));
+    return new ColumnarArray(getChild(0), getArrayOffset(rowId), getArrayLength(rowId));


Why change this?

jiangxb1987

Please also paste the benchmark result after the changes.

viirya · 2018-01-19T04:42:20Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ColumnarBatchScan.scala

+      s"$columnVar.getStruct($ordinal)"
+    } else {
+      ctx.getValue(columnVar, dataType, ordinal)
+    }


Can't we use this API?

/** * Returns the specialized code to access a value from a column vector for a given `DataType`. */ def getValue(vector: String, rowId: String, dataType: DataType): String = { ... }

ah I didn't know there is such an API. I'll use it instead.

viirya · 2018-01-19T04:42:53Z

...ore/src/main/scala/org/apache/spark/sql/execution/aggregate/VectorizedHashMapGenerator.scala

+        val value = if (key.dataType.isInstanceOf[StructType]) {
+          s"vectors[$ordinal].getStruct(buckets[idx])"
+        } else {
+          ctx.getValue(s"vectors[$ordinal]", "buckets[idx]", key.dataType)


SparkQA · 2018-01-19T06:22:10Z

Test build #86372 has finished for PR 20277 at commit 37c82e6.

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

SparkQA · 2018-01-19T08:05:01Z

Test build #86376 has finished for PR 20277 at commit 3972093.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-01-19T08:05:57Z

retest this please

SparkQA · 2018-01-19T11:30:44Z

Test build #86380 has finished for PR 20277 at commit 3972093.

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

cloud-fan · 2018-01-19T17:54:00Z

retest this please, since the ColumnarBatch PR has been merged.

SparkQA · 2018-01-19T21:26:06Z

Test build #86394 has finished for PR 20277 at commit 3972093.

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

kiszk · 2018-01-19T22:32:17Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java

@@ -55,164 +43,82 @@ public void close() {
    if (childColumns != null) {
      for (int i = 0; i < childColumns.length; i++) {
        childColumns[i].close();
+        childColumns[i] = null;


Is it OK not to call close() while ColumnVector.close() is provided?

what do you mean? ColumnVector.close is a required interface.

ueshin · 2018-01-22T05:59:43Z

sql/core/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java

@@ -55,164 +43,82 @@ public void close() {
    if (childColumns != null) {
      for (int i = 0; i < childColumns.length; i++) {
        childColumns[i].close();
+        childColumns[i] = null;
      }
    }


We need to do childColumns = null after the for loop, otherwise NullPointerException will be thrown if close() is called twice?

ueshin · 2018-01-22T07:15:19Z

...ore/src/main/scala/org/apache/spark/sql/execution/aggregate/VectorizedHashMapGenerator.scala

-          key.dataType), key.name)})"""
+        // `ColumnVector.getStruct` is different from `InternalRow.getStruct`, it only takes an
+        // `ordinal` parameter.
+        val value = ctx.getValue(s"vectors[$ordinal]", key.dataType, "buckets[idx]")


getValueFromVector instead of getValue?

ueshin · 2018-01-22T07:33:09Z

LGTM pending Jenkins.

SparkQA · 2018-01-22T08:05:02Z

Test build #86459 has finished for PR 20277 at commit 55a288e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-22T08:05:02Z

Test build #86463 has finished for PR 20277 at commit 0c22f5b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-01-22T08:56:47Z

Jenkins, retest this please.

SparkQA · 2018-01-22T12:19:33Z

Test build #86469 has finished for PR 20277 at commit 0c22f5b.

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

cloud-fan · 2018-01-22T12:57:12Z

thanks, merging to master/2.3!

## What changes were proposed in this pull request? Several improvements: * provide a default implementation for the batch get methods * rename `getChildColumn` to `getChild`, which is more concise * remove `getStruct(int, int)`, it's only used to simplify the codegen, which is an internal thing, we should not add a public API for this purpose. ## How was this patch tested? existing tests Author: Wenchen Fan <wenchen@databricks.com> Closes #20277 from cloud-fan/column-vector. (cherry picked from commit 5d680ca) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan commented Jan 16, 2018

View reviewed changes

cloud-fan force-pushed the column-vector branch 3 times, most recently from 08d06a7 to bc6d0af Compare January 17, 2018 05:04

cloud-fan force-pushed the column-vector branch from bc6d0af to 77e8c4b Compare January 17, 2018 05:15

cloud-fan commented Jan 17, 2018

View reviewed changes

polish ColumnVector

f3f9d5e

cloud-fan force-pushed the column-vector branch from 77e8c4b to f3f9d5e Compare January 17, 2018 05:18

remove ColumnVector.getStruct(int, int)

eccdca1

cloud-fan force-pushed the column-vector branch from 212c841 to eccdca1 Compare January 19, 2018 03:07

cloud-fan added 2 commits January 19, 2018 11:08

Merge remote-tracking branch 'origin/master' into column-vector

ab4a625

fix merge conflict

37c82e6

jiangxb1987 reviewed Jan 19, 2018

View reviewed changes

viirya reviewed Jan 19, 2018

View reviewed changes

address comments

3972093

kiszk reviewed Jan 19, 2018

View reviewed changes

ueshin reviewed Jan 22, 2018

View reviewed changes

address comment

55a288e

ueshin reviewed Jan 22, 2018

View reviewed changes

fix a mistake

0c22f5b

asfgit closed this in 5d680ca Jan 22, 2018

[SPARK-23090][SQL] polish ColumnVector #20277

[SPARK-23090][SQL] polish ColumnVector #20277

Conversation

cloud-fan commented Jan 16, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

cloud-fan commented Jan 16, 2018

Choose a reason for hiding this comment

SparkQA commented Jan 16, 2018

hvanhovell commented Jan 16, 2018

cloud-fan commented Jan 17, 2018

SparkQA commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

cloud-fan commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 18, 2018

SparkQA commented Jan 19, 2018

Choose a reason for hiding this comment

jiangxb1987 Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangxb1987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 19, 2018

SparkQA commented Jan 19, 2018

cloud-fan commented Jan 19, 2018

SparkQA commented Jan 19, 2018

cloud-fan commented Jan 19, 2018

SparkQA commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Jan 22, 2018

SparkQA commented Jan 22, 2018

SparkQA commented Jan 22, 2018

ueshin commented Jan 22, 2018

SparkQA commented Jan 22, 2018

cloud-fan commented Jan 22, 2018

cloud-fan commented Jan 16, 2018 •

edited

Loading

jiangxb1987 Jan 19, 2018 •

edited

Loading