[SPARK-23272][SQL] add calendar interval type support to ColumnVector #20438

cloud-fan · 2018-01-30T11:40:50Z

What changes were proposed in this pull request?

ColumnVector is aimed to support all the data types, but CalendarIntervalType is missing. Actually we do support interval type for inner fields, e.g. ColumnarRow, ColumnarArray both support interval type. It's weird if we don't support interval type at the top level.

This PR adds the interval type support.

This PR also makes ColumnVector.getChild protect. We need it public because MutableColumnaRow.getInterval needs it. Now the interval implementation is in ColumnVector.getInterval.

How was this patch tested?

a new test.

cloud-fan · 2018-01-30T11:41:12Z

cc @sameeragarwal @viirya @kiszk @ueshin @gatorsmile

ueshin · 2018-01-30T11:50:36Z

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

   * struct field.
   */
  public final ColumnarRow getStruct(int rowId) {
+    if (isNullAt(rowId)) return null;


Good catch!

ueshin · 2018-01-30T11:59:38Z

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

-    int month = data.getChild(0).getInt(offset + ordinal);
-    long microseconds = data.getChild(1).getLong(offset + ordinal);
-    return new CalendarInterval(month, microseconds);
+    return data.getInterval(offset + ordinal);


We should insert if (data.isNullAt(offset + ordinal)) return null; to be consistent with other ColumnarXxxs?
Or I guess we can remove these null-checks from all other ColumnarXxxs and leave it to ColumnVector.getInterval()?

Good catch! I'm going to require ColumnVector.getXXX to return null if the value is null, but I'll do it in another PR, to update all the documents and define the behavior of batched getXXX methods.

jaceklaskowski · 2018-01-30T12:39:09Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/CalendarInterval.java

  }

-  public static long toLongWithRange(String fieldName,
+  private static long toLongWithRange(String fieldName,


Why?! It's much harder (if at all possible) to test private methods (been bitten few times this week and remember the pain).

jaceklaskowski · 2018-01-30T12:42:34Z

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

+   *
+   * In Spark, calendar interval type value is basically an integer value representing the number of
+   * months in this interval, and a long value representing the number of microseconds in this
+   * interval. A interval type vector is same as a struct type vector with 2 fields: `months` and


"An interval" + "is the same as"

jaceklaskowski · 2018-01-30T12:43:10Z

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

+   * `microseconds`.
+   *
+   * To support interval type, implementations must implement {@link #getChild(int)} and define 2
+   * child vectors: the first child vector is a int type vector, containing all the month values of


is an int type vector

jaceklaskowski · 2018-01-30T12:44:03Z

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

+   */
+  public final CalendarInterval getInterval(int rowId) {
+    if (isNullAt(rowId)) return null;
+    final int months = getChild(0).getInt(rowId);


What's the purpose of final keyword here?

It's from the previous code, probably it tries to make the compiler happy and run the code faster.

jaceklaskowski · 2018-01-30T12:45:16Z

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

+  }
+
  /**
   * Returns the ordinal's child column vector.


@return [[ColumnVector]] at the ordinal

kiszk · 2018-01-30T15:07:20Z

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

+   * interval. An interval type vector is the same as a struct type vector with 2 fields: `months`
+   * and `microseconds`.
+   *
+   * To support interval type, implementations must implement {@link #getChild(int)} and define 2


nit: interval type -> calendar interval type

It's a little annoying to type calendar interval type all the time...

SparkQA · 2018-01-30T15:36:41Z

Test build #86819 has finished for PR 20438 at commit 4e53891.

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

SparkQA · 2018-01-30T16:30:28Z

Test build #86825 has finished for PR 20438 at commit 2f23a1d.

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

dongjoon-hyun · 2018-01-30T16:50:36Z

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

+   * @return child [[ColumnVector]] at the given ordinal.
   */
-  public abstract ColumnVector getChild(int ordinal);
+  protected abstract ColumnVector getChild(int ordinal);


Oh, I see. Now, it became protected.

Since ColumnVector is public, could you add some description in PR description for this kind of visibility change?

ueshin · 2018-01-31T03:02:07Z

LGTM.

viirya · 2018-01-31T03:08:48Z

sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/MutableColumnarRow.java

@@ -146,9 +146,7 @@ public byte[] getBinary(int ordinal) {
  @Override
  public CalendarInterval getInterval(int ordinal) {
    if (columns[ordinal].isNullAt(rowId)) return null;


Do we still need this null check?

see #20438 (comment) . In this PR I just fixed the returning null issue for getStruct and getInterval, because they are non-abstract. There should be a follow up to clearly document that ColumnVector.getBinary/getUTF8String/... should return null if this slot is null. Then we can remove these null checks here. I appreciate it if you have time to take this :)

Let me try to prepare a PR tonight.

viirya · 2018-01-31T03:11:08Z

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

@@ -139,9 +139,7 @@ public byte[] getBinary(int ordinal) {
  @Override
  public CalendarInterval getInterval(int ordinal) {
    if (data.getChild(ordinal).isNullAt(rowId)) return null;


Can we remove this null check now?

cloud-fan · 2018-01-31T07:13:42Z

thanks, merging to master/2.3!

## What changes were proposed in this pull request? `ColumnVector` is aimed to support all the data types, but `CalendarIntervalType` is missing. Actually we do support interval type for inner fields, e.g. `ColumnarRow`, `ColumnarArray` both support interval type. It's weird if we don't support interval type at the top level. This PR adds the interval type support. This PR also makes `ColumnVector.getChild` protect. We need it public because `MutableColumnaRow.getInterval` needs it. Now the interval implementation is in `ColumnVector.getInterval`. ## How was this patch tested? a new test. Author: Wenchen Fan <[email protected]> Closes #20438 from cloud-fan/interval. (cherry picked from commit 695f714) Signed-off-by: Wenchen Fan <[email protected]>

add calendar interval type support to ColumnVector

4e53891

ueshin reviewed Jan 30, 2018

View reviewed changes

jaceklaskowski reviewed Jan 30, 2018

View reviewed changes

address comments

2f23a1d

kiszk reviewed Jan 30, 2018

View reviewed changes

dongjoon-hyun reviewed Jan 30, 2018

View reviewed changes

viirya reviewed Jan 31, 2018

View reviewed changes

asfgit closed this in 695f714 Jan 31, 2018

[SPARK-23272][SQL] add calendar interval type support to ColumnVector #20438

[SPARK-23272][SQL] add calendar interval type support to ColumnVector #20438

Uh oh!

Conversation

cloud-fan commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 30, 2018

Uh oh!

SparkQA commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ueshin commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cloud-fan commented Jan 30, 2018 •

edited

Loading

cloud-fan Jan 30, 2018 •

edited

Loading

kiszk Jan 30, 2018 •

edited

Loading