[SPARK-24564][TEST] Add test suite for RecordBinaryComparator #21570

jiangxb1987 · 2018-06-14T22:54:36Z

What changes were proposed in this pull request?

Add a new test suite to test RecordBinaryComparator.

How was this patch tested?

New test suite.

SparkQA · 2018-06-15T03:16:56Z

Test build #91872 has finished for PR 21570 at commit 79508ec.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class RecordBinaryComparatorSuite

kiszk · 2018-06-15T05:13:27Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+  private long relativeOffset(int numFields) {
+    // All the UnsafeRows in this suite contains less than 64 columns, so the bitSetSize shall
+    // always be 8.
+    return 8 + numFields * Long.BYTES;


nit: Is the multiplication factor related to Long.BYTES? It is just 8L as computeSizeInBytes uses.

kiszk · 2018-06-15T05:34:06Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+
+    UnsafeRow row1 = new UnsafeRow(numFields);
+    byte[] data1 = new byte[100];
+    row1.pointTo(data1, computeSizeInBytes(numFields * Double.BYTES));


nit: ditto. Regardless of data type, width is always 8.

kiszk · 2018-06-15T05:35:57Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+    UnsafeRow row1 = new UnsafeRow(numFields);
+    byte[] data1 = new byte[100];
+    UTF8String str1 = UTF8String.fromString("Milk tea");
+    row1.pointTo(data1, computeSizeInBytes(numFields * Long.BYTES + str1.numBytes()));


nit: ditto. Regardless of data type, width is always 8.

jiangxb1987 · 2018-06-15T05:45:46Z

@kiszk Thanks, updated!

kiszk · 2018-06-15T06:05:26Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+  private long relativeOffset(int numFields) {
+    // All the UnsafeRows in this suite contains less than 64 columns, so the bitSetSize shall
+    // always be 8.
+    return 8 + numFields * 8;


super nit: My suggestion is to use * 8L. Although we know numFields is less than 64, it would be good to use long multiplication to avoid possible integer overflow in general.

kiszk · 2018-06-15T06:07:53Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+package test.org.apache.spark.sql.execution.sort;
+
+import org.apache.spark.SparkConf;
+import org.apache.spark.memory.TaskMemoryManager;


Since this is duplicated, it should be deleted.

cc @jiangxb1987

sorry but which duplicated line do you mention?

I am sorry. Nvm, I thought lines 21 and 23 are duplicated.

kiszk · 2018-06-15T06:08:34Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+import org.apache.spark.unsafe.memory.MemoryBlock;
+import org.apache.spark.unsafe.types.UTF8String;
+import org.apache.spark.util.collection.unsafe.sort.*;
+import org.junit.After;


nit: is it better to insert an empty line between L32 and L33?

SparkQA · 2018-06-15T07:05:01Z

Test build #91890 has finished for PR 21570 at commit 2b8df98.

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

SparkQA · 2018-06-15T07:05:02Z

Test build #91887 has finished for PR 21570 at commit 7d43900.

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

kiszk · 2018-06-15T11:10:15Z

retest this please

SparkQA · 2018-06-15T15:33:03Z

Test build #91910 has finished for PR 21570 at commit 2b8df98.

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

jiangxb1987 · 2018-06-15T17:20:42Z

cc @JoshRosen @gatorsmile

maropu · 2018-06-16T02:37:23Z

core/src/test/java/org/apache/spark/memory/TestMemoryConsumer.java

@@ -43,6 +45,12 @@ void free(long size) {
    used -= size;
    taskMemoryManager.releaseExecutionMemory(size, this);
  }
+
+  // Exposed for testing


super nit: we need this comment? This class itself is used for test use only?

How about using @VisibleForTesting annotation?

maropu · 2018-06-16T02:43:22Z

...core/src/test/java/test/org/apache/spark/sql/execution/sort/RecordBinaryComparatorSuite.java

+ */
+public class RecordBinaryComparatorSuite {
+
+  private final TaskMemoryManager memoryManager = new TaskMemoryManager(


We need to use TaskMemoryManager for this test? Is not simple tests (e.g., just comparing unsafe rows) enough?

Both way shall work. I choose to implement it in low level so the result won't be affected by the UnsafeProjection code.

SparkQA · 2018-06-19T18:28:57Z

Test build #92090 has finished for PR 21570 at commit 5be5a7a.

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

gatorsmile · 2018-06-26T04:52:54Z

ping @JoshRosen

kiszk · 2018-06-26T17:09:57Z

LGTM

cloud-fan · 2018-06-28T05:23:32Z

core/src/test/java/org/apache/spark/memory/TestMemoryConsumer.java

@@ -43,6 +47,12 @@ void free(long size) {
    used -= size;
    taskMemoryManager.releaseExecutionMemory(size, this);
  }
+
+  @VisibleForTesting


it's already in the test package, we don't need this tag.

cloud-fan · 2018-06-28T06:20:05Z

LGTM, merging to master!

## What changes were proposed in this pull request? Add a new test suite to test RecordBinaryComparator. ## How was this patch tested? New test suite. Author: Xingbo Jiang <xingbo.jiang@databricks.com> Closes apache#21570 from jiangxb1987/rbc-test.

add test suite for RecordBinaryComparator

79508ec

update

7d43900

kiszk reviewed Jun 15, 2018

View reviewed changes

update

2b8df98

kiszk reviewed Jun 15, 2018

View reviewed changes

maropu reviewed Jun 16, 2018

View reviewed changes

update

5be5a7a

cloud-fan reviewed Jun 28, 2018

View reviewed changes

asfgit closed this in 5b05966 Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-24564][TEST] Add test suite for RecordBinaryComparator #21570

[SPARK-24564][TEST] Add test suite for RecordBinaryComparator #21570

jiangxb1987 commented Jun 14, 2018

SparkQA commented Jun 15, 2018

kiszk Jun 15, 2018 •

edited

Loading

kiszk Jun 15, 2018

kiszk Jun 15, 2018

jiangxb1987 commented Jun 15, 2018

kiszk Jun 15, 2018 •

edited

Loading

kiszk Jun 15, 2018

kiszk Jun 22, 2018

jiangxb1987 Jun 26, 2018

kiszk Jun 26, 2018

kiszk Jun 15, 2018 •

edited

Loading

SparkQA commented Jun 15, 2018

SparkQA commented Jun 15, 2018

kiszk commented Jun 15, 2018

SparkQA commented Jun 15, 2018

jiangxb1987 commented Jun 15, 2018

maropu Jun 16, 2018

kiszk Jun 17, 2018

maropu Jun 16, 2018

jiangxb1987 Jun 19, 2018

SparkQA commented Jun 19, 2018

gatorsmile commented Jun 26, 2018

kiszk commented Jun 26, 2018

cloud-fan Jun 28, 2018

cloud-fan commented Jun 28, 2018

[SPARK-24564][TEST] Add test suite for RecordBinaryComparator #21570

[SPARK-24564][TEST] Add test suite for RecordBinaryComparator #21570

Conversation

jiangxb1987 commented Jun 14, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jun 15, 2018

kiszk Jun 15, 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 commented Jun 15, 2018

kiszk Jun 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

kiszk Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jun 15, 2018

SparkQA commented Jun 15, 2018

kiszk commented Jun 15, 2018

SparkQA commented Jun 15, 2018

jiangxb1987 commented Jun 15, 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 Jun 19, 2018

gatorsmile commented Jun 26, 2018

kiszk commented Jun 26, 2018

Choose a reason for hiding this comment

cloud-fan commented Jun 28, 2018

kiszk Jun 15, 2018 •

edited

Loading

kiszk Jun 15, 2018 •

edited

Loading

kiszk Jun 15, 2018 •

edited

Loading