[SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite #18327

jinxing64 · 2017-06-16T11:05:42Z

What changes were proposed in this pull request?

Current ColumnarBatchSuite has very simple test cases for Array and Struct. This pr wants to add some test suites for complicated cases in ColumnVector.

…uite.

jinxing64 · 2017-06-16T11:07:11Z

@kiszk
Would you mind if I make a try for this JIRA?

SparkQA · 2017-06-16T13:17:45Z

Test build #78178 has finished for PR 18327 at commit 5668978.

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

kiszk · 2017-06-19T10:02:19Z

Thanks, let me take a look ~~tonight~~ tomorrow.

kiszk · 2017-06-20T10:22:00Z

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

+        return getArray(ordinal);
+      } else if (dataType instanceof StructType) {
+        return getStruct(ordinal, ((StructType)dataType).fields().length);
+      } else {


We need to support other data types (e.g. ByteType, StringType, BinaryType, DateType, TimestampType, and MapType). It would be good to see copy() method.

kiszk · 2017-06-20T10:22:43Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

    }}
  }

+  test("Nest Array in Array.") {


Can we prepare a test for primitive type array (e.g. new ArrayType(new ArrayType(IntegerType, false), true), too?

Sorry, I'm not pretty sure about this. Could you please give more details ?

Sure. My previous comment was a little bit wrong.

What I want to say is the following. It would be good to prepare the following two cases for array test.
In the case of new ArrayType(new ArrayType(IntegerType, false), true), all of the elements in each leaf array must not have null (i.e. this test case).
In the case of new ArrayType(new ArrayType(IntegerType, true), true), any element in each leaf array may have null (e.g. [[0]], [[1, 2], []], [[], [3, null, 5]]).

kiszk · 2017-06-20T10:24:20Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+      c1.putArray(2, 3, 3)
+
+      assert(column.getStruct(0).getInt(0) === 0)
+      assert(column.getStruct(0).getArray(1).toIntArray === Array(0, 1))


nit: toIntArray()? since other places use toIntArray().

kiszk · 2017-06-20T10:26:15Z

LGTM except three comments.
@cloud-fan could you please look at this? I believe that these test suites would help make implementations of the API stable.

jinxing64 · 2017-06-20T11:58:59Z

@kiszk
Thank you so much !
I will read your comments carefully and refine this pr : )

SparkQA · 2017-06-21T12:56:42Z

Test build #78380 has finished for PR 18327 at commit 66b4eff.

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

SparkQA · 2017-06-21T13:03:23Z

Test build #78381 has finished for PR 18327 at commit 36dff00.

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

SparkQA · 2017-06-22T03:04:26Z

Test build #78416 has finished for PR 18327 at commit 78102f3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-06-22T06:36:19Z

Test build #78423 has finished for PR 18327 at commit 9319a2f.

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

jinxing64 · 2017-06-22T09:06:48Z

@kiszk
I tried to add a test Nest Array(containing null) in Array.. Please take a look when you have time and I will continue working on this :)

heary-cao · 2017-06-22T09:27:24Z

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

    @Override
    public Object get(int ordinal, DataType dataType) {
-      throw new UnsupportedOperationException();
+      if (dataType instanceof BooleanType) {


Suggest to use ‘match’ style.

Thanks for review :)
Could you be specific how to do that in this java code ?

kiszk · 2017-06-22T12:27:27Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+      assert(column.getArray(1).getArray(0).toIntArray() === Array(1, 2))
+      assert(column.getArray(1).getArray(1).toIntArray() === Array())
+      assert(column.getArray(2).getArray(0).toIntArray() === Array())
+      assert(column.getArray(2).getArray(1)isNullAt(0))


Should it be getArray(1).isNullAt(0)?

SparkQA · 2017-06-22T15:33:00Z

Test build #78456 has finished for PR 18327 at commit 47be362.

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

kiszk · 2017-06-23T02:49:25Z

LGTM
@cloud-fan could you please review this?

cloud-fan · 2017-06-23T03:15:25Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+    }
+  }
+
+  test("Nest Array(containing null) in Array.") {


why we have 2 tests for containing null and not containing null?

One is for ColumnVector.allocate(10, new ArrayType(new ArrayType(IntegerType, true), true)). The other is for ColumnVector.allocate(10, new ArrayType(new ArrayType(IntegerType, false), true)). For different schemes.
What do you think?

I think the containing null case covers the not containing null case?

Ah, you are right since now we are testing correct cases.

Is it better to add a test suite to put null into an array for the case ColumnVector.allocate(10, new ArrayType(new ArrayType(IntegerType, false), true))?

@kiszk @cloud-fan
It seems that there is no check for putting null into a ArrayType(IntegerType, false).
And do you think it's better to fail the putNullmethod in OffHeapColumnVector and OnHeapColumnVector when containsNull=false. I'm happy to make another pr for that :)

Yeah, I agree. That's why I asked to prepare two cases.

cloud-fan · 2017-06-23T03:21:26Z

LGTM except one comment

cloud-fan · 2017-06-23T05:48:06Z

sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala

+      assert(column.getArray(2).getArray(0).toIntArray() === Array())
+      assert(column.getArray(2).getArray(1).isNullAt(0))
+      assert(Array(column.getArray(2).getArray(1).getInt(1),
+        column.getArray(2).getArray(1).getInt(2)) === Array(4, 5))


nit: we can split this into 2 asserts

cloud-fan · 2017-06-23T07:47:10Z

LGTM pending jenkins

jinxing64 · 2017-06-23T07:48:21Z

@cloud-fan
Thanks a lot for taking time review this :)

SparkQA · 2017-06-23T09:55:08Z

Test build #78514 has finished for PR 18327 at commit ed210fb.

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

cloud-fan · 2017-06-23T12:41:30Z

thanks, merging to master!

jinxing64 · 2017-06-23T13:15:45Z

@cloud-fan
Thanks for merging :)

…uite ## What changes were proposed in this pull request? Current ColumnarBatchSuite has very simple test cases for `Array` and `Struct`. This pr wants to add some test suites for complicated cases in ColumnVector. Author: jinxing <jinxing6042@126.com> Closes apache#18327 from jinxing64/SPARK-21047.

[SPARK-21047] Add test suites for complicated cases in ColumnarBatchS…

5668978

…uite.

jinxing64 changed the title ~~[SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite~~ [WIP][SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite Jun 16, 2017

jinxing64 changed the title ~~[WIP][SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite~~ [SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite Jun 19, 2017

kiszk reviewed Jun 20, 2017

View reviewed changes

resolve kiszk's comments.

36dff00

jinxing64 force-pushed the SPARK-21047 branch from 66b4eff to 36dff00 Compare June 21, 2017 10:50

Add 'Nest Array(containing null) in Array.'

9319a2f

jinxing64 force-pushed the SPARK-21047 branch from 78102f3 to 9319a2f Compare June 22, 2017 04:21

heary-cao reviewed Jun 22, 2017

View reviewed changes

kiszk reviewed Jun 22, 2017

View reviewed changes

fix

47be362

cloud-fan reviewed Jun 23, 2017

View reviewed changes

Resolve cloud-fan's comments

ed210fb

asfgit closed this in 153dd49 Jun 23, 2017

[SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite #18327

[SPARK-21047] Add test suites for complicated cases in ColumnarBatchSuite #18327

Uh oh!

Conversation

jinxing64 commented Jun 16, 2017

What changes were proposed in this pull request?

Uh oh!

jinxing64 commented Jun 16, 2017

Uh oh!

SparkQA commented Jun 16, 2017

Uh oh!

kiszk commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Jun 20, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Jun 20, 2017

Uh oh!

jinxing64 commented Jun 20, 2017

Uh oh!

SparkQA commented Jun 21, 2017

Uh oh!

SparkQA commented Jun 21, 2017

Uh oh!

SparkQA commented Jun 22, 2017

Uh oh!

SparkQA commented Jun 22, 2017

Uh oh!

jinxing64 commented Jun 22, 2017

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!

SparkQA commented Jun 22, 2017

Uh oh!

kiszk commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Jun 23, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

jinxing64 Jun 23, 2017 • 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!

cloud-fan commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 23, 2017

Uh oh!

jinxing64 commented Jun 23, 2017

Uh oh!

SparkQA commented Jun 23, 2017

Uh oh!

cloud-fan commented Jun 23, 2017

Uh oh!

kiszk commented Jun 19, 2017 •

edited

Loading

kiszk Jun 20, 2017 •

edited

Loading

kiszk Jun 23, 2017 •

edited

Loading

jinxing64 Jun 23, 2017 •

edited

Loading