[SPARK-31425][SQL][CORE] UnsafeKVExternalSorter/VariableLengthRowBasedKeyValueBatch should also respect UnsafeAlignedOffset by Ngone51 · Pull Request #28195 · apache/spark

Ngone51 · 2020-04-12T14:51:00Z

What changes were proposed in this pull request?

Make UnsafeKVExternalSorter / VariableLengthRowBasedKeyValueBatch also respect UnsafeAlignedOffset when reading the record and update some out of date comemnts.

Why are the changes needed?

Since BytesToBytesMap respects UnsafeAlignedOffset when writing the record, UnsafeKVExternalSorter should also respect UnsafeAlignedOffset when reading the record from BytesToBytesMap otherwise it will causes data correctness issue.

Unlike UnsafeKVExternalSorter may reading records from BytesToBytesMap, VariableLengthRowBasedKeyValueBatch writes and reads records by itself. Thus, similar to #22053 and comment there, fix for VariableLengthRowBasedKeyValueBatch more likely an improvement for the support of SPARC platform.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually tested HashAggregationQueryWithControlledFallbackSuite with UAO_SIZE=8 to simulate SPARC platform. And tests only pass with this fix.

SparkQA · 2020-04-12T17:25:47Z

Test build #121147 has finished for PR 28195 at commit 6943425.

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

Ngone51 · 2020-04-13T01:44:19Z

ping @cloud-fan @jiangxb1987 @kiszk Please take a look thanks!

cloud-fan · 2020-04-13T05:19:35Z

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

      // the key address instead of storing the absolute address of the value, the key and value
      // must be stored in the same memory page.
-      // (8 byte key length) (key) (value) (8 byte pointer to next value)
+      // (uao: klen + vlen + uaolen) (uao: klen) (key) (value) (8 byte pointer to next value)


what does (uao: klen + vlen + uaolen) (uao: klen) mean?

it's (total length) (key length) ...?

yes, if total length here doesn't include the first uaoSize.

How about using the similar description at lines 56-61?

It will be a little bit redundant? Maybe just, (total length) (key length) (key) (value) (8 byte pointer to next value) ?

cloud-fan · 2020-04-13T05:22:16Z

can we have a test?

kiszk · 2020-04-13T09:28:05Z

@Ngone51 Did you run other tests in the environment at uao=8. Then, did they pass?

I am curious that the following files also depends on uao=4.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/VariableLengthRowBasedKeyValueBatch.java
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/FixedLengthRowBasedKeyValueBatch.java

Ngone51 · 2020-04-13T09:33:50Z

No, I'll check them later and fix together if needed.

…e 4 and 8

Ngone51 · 2020-04-13T09:49:09Z

can we have a test?

yet, I updated HashAggregationQueryWithControlledFallbackSuite only to test both 4 and 8 uaoSize, which actually can fail with uaoSize = 8.

Seems better if we could update all those related tests to test both as well.

kiszk · 2020-04-13T09:54:09Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/AggregationQuerySuite.scala

+                       |once it has processed $fallbackStartsAt input rows).
+                       |The query is ${actual.queryExecution}
+                       |$errorMessage
                  """.stripMargin


nit: indentation issue

Let's fix it

SparkQA · 2020-04-13T12:59:12Z

Test build #121195 has finished for PR 28195 at commit 0b8ebd3.

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

Ngone51 · 2020-04-14T07:44:47Z

I fixed VariableLengthRowBasedKeyValueBatch as well. To be clarify, it won't cause data correctness issue as UnsafeKVExternalSorter does. Unlike UnsafeKVExternalSorter may reading records from BytesToBytesMap, VariableLengthRowBasedKeyValueBatch writes and reads records by itself. Thus, similar to #22053 and your comment there, it's more likely an improvement for the support of SPARC platform.

BTW, I believe we don't need to fix FixedLengthRowBasedKeyValueBatch as it doesn't need to write any length. cc @kiszk

cloud-fan · 2020-04-14T08:15:53Z

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

 * probably be using sorting instead of hashing for better cache locality.
 *
- * The key and values under the hood are stored together, in the following format:
+ * The key and values under the hood are stored together, in the following format(uaoSize = 4):


in case of uaoSize = 4

cloud-fan · 2020-04-14T08:17:36Z

...main/java/org/apache/spark/sql/catalyst/expressions/VariableLengthRowBasedKeyValueBatch.java

    long offset = keyRow.getBaseOffset();
    int klen = keyRow.getSizeInBytes();
-    int vlen = Platform.getInt(base, offset - 8) - klen - 4;
+    int vlen = UnsafeAlignedOffset.getSize(base, offset - uaoSize - uaoSize) - klen - uaoSize;


- uaoSize * 2

cloud-fan · 2020-04-14T08:18:26Z

...main/java/org/apache/spark/sql/catalyst/expressions/VariableLengthRowBasedKeyValueBatch.java

-        totalLength = Platform.getInt(base, offsetInPage) - 4;
-        currentklen = Platform.getInt(base, offsetInPage + 4);
+        int uaoSize = UnsafeAlignedOffset.getUaoSize();
+        int doubleUaoSize = uaoSize + uaoSize;


let's be consistent with https://github.com/apache/spark/pull/28195/files#diff-ed82c6d7b012a3bf89fc2974e2840deaR46

SparkQA · 2020-04-14T10:48:54Z

Test build #121262 has finished for PR 28195 at commit b1be35e.

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

SparkQA · 2020-04-14T16:04:14Z

Test build #121276 has finished for PR 28195 at commit bda5823.

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

Ngone51 · 2020-04-14T16:56:28Z

retest this please.

kiszk · 2020-04-14T18:01:30Z

Thank you for updating VariableLengthRowBasedKeyValueBatch and your clarification. Looks good.

kiszk · 2020-04-14T18:43:25Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/AggregationQuerySuite.scala

+
+                  fail(newErrorMessage)
+                case None => // Success
+              }


I am not confident, but do we need to call setUaoSize(0) at the end of this test? This is because TEST_UAO_SIZE is static.

Oh, yeah. I think we have to reset it.

SparkQA · 2020-04-14T19:40:44Z

Test build #121284 has finished for PR 28195 at commit bda5823.

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

SparkQA · 2020-04-15T05:04:14Z

Test build #121297 has finished for PR 28195 at commit d441e53.

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

jiangxb1987 · 2020-04-15T06:28:18Z

common/unsafe/src/main/java/org/apache/spark/unsafe/UnsafeAlignedOffset.java

+
  public static int getUaoSize() {
-    return UAO_SIZE;
+    return TEST_UAO_SIZE == 0 ? UAO_SIZE : TEST_UAO_SIZE;


nit: you can use Utils.isTesting instead

Other tests might not set UAO size manually, then we'll get 0 in this case.

We should handle the logic of setting/reverting the size in the test code.

then, we need to figure out all the test cases where used UnsafeAlignedOffset, right?

Why do we need to do that? The default value of TEST_UAO_SIZE should be the same as UAO_SIZE, only if you want to test other values then you need to change it.

Ok, I get your point. Let me update it later.

hmm...unfortunately, unsafe project doesn't depend on core. So we can not access the Utils.

jiangxb1987 · 2020-04-15T06:29:20Z

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

 *
- * The key and values under the hood are stored together, in the following format:
+ * The key and values under the hood are stored together, in the following format(in case of
+ * uaoSize = 4):


why not replace the number below with uaoSize?

I was supposed to do so but it requires more changes in the original content and it is not so clean compares to a concrete number. So I am not sure if we should do. Do you have strong prefer to change in uaoSize way?

The current way doesn't make it clear whether each number 4 represents the uaoSize.

Ok. I'll update later.

jiangxb1987 · 2020-04-15T06:31:20Z

...main/java/org/apache/spark/sql/catalyst/expressions/VariableLengthRowBasedKeyValueBatch.java

                             Object vbase, long voff, int vlen) {
-    final long recordLength = 8L + klen + vlen + 8;
+    int uaoSize = UnsafeAlignedOffset.getUaoSize();
+    int doubleUaoSize = 2 * uaoSize;


doubleUaoSize sounds strange, I'd keep the 2 * uaoSize in the computation...

jiangxb1987 · 2020-04-15T06:35:02Z

sql/core/src/main/java/org/apache/spark/sql/execution/UnsafeKVExternalSorter.java

        // Get encoded memory address
        // baseObject + baseOffset point to the beginning of the key data in the map, but that
-        // the KV-pair's length data is stored in the word immediately before that address
+        // the KV-pair's length data is stored at 2 * uaoSize bytes immediately before that address


sorry I don't get it here why the address is related to uaoSize?

The record format is:

(total length) (key length) (key) (value) (8 byte pointer to next value) | | uaoSize uaoSize

And we now get keyOffset, so we need back walk for 2 usao sizes to get the address of the record.

so you mean we need to add back 2 * uaoSize to cover the space of storing total length and key length? If that's the case then it makes sense.

SparkQA · 2020-04-15T15:45:32Z

Test build #121324 has finished for PR 28195 at commit 0fb6024.

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

SparkQA · 2020-04-16T17:26:30Z

Test build #121368 has finished for PR 28195 at commit c6f39b5.

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

cloud-fan · 2020-04-17T04:48:42Z

thanks, merging to master/3.0!

…dKeyValueBatch should also respect UnsafeAlignedOffset ### What changes were proposed in this pull request? Make `UnsafeKVExternalSorter` / `VariableLengthRowBasedKeyValueBatch ` also respect `UnsafeAlignedOffset` when reading the record and update some out of date comemnts. ### Why are the changes needed? Since `BytesToBytesMap` respects `UnsafeAlignedOffset` when writing the record, `UnsafeKVExternalSorter` should also respect `UnsafeAlignedOffset` when reading the record from `BytesToBytesMap` otherwise it will causes data correctness issue. Unlike `UnsafeKVExternalSorter` may reading records from `BytesToBytesMap`, `VariableLengthRowBasedKeyValueBatch` writes and reads records by itself. Thus, similar to #22053 and [comment](#22053 (comment)) there, fix for `VariableLengthRowBasedKeyValueBatch` more likely an improvement for the support of SPARC platform. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Manually tested `HashAggregationQueryWithControlledFallbackSuite` with `UAO_SIZE=8` to simulate SPARC platform. And tests only pass with this fix. Closes #28195 from Ngone51/fix_uao. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 40f9dbb) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Ngone51 · 2020-04-17T07:06:52Z

thanks all!

Ngone51 added 2 commits April 12, 2020 22:37

fix uao

eb9e5bc

minor

6943425

cloud-fan reviewed Apr 13, 2020

View reviewed changes

test HashAggregationQueryWithControlledFallbackSuite with both uaoSiz…

0b8ebd3

…e 4 and 8

probot-autolabeler bot added CORE SQL labels Apr 13, 2020

kiszk reviewed Apr 13, 2020

View reviewed changes

Ngone51 added 2 commits April 14, 2020 15:04

update comment

c70c5db

fix VariableLengthRowBasedKeyValueBatc as well

b1be35e

Ngone51 changed the title ~~[SPARK-31425][SQL][CORE] UnsafeKVExternalSorter should also respect UnsafeAlignedOffset~~ [SPARK-31425][SQL][CORE] UnsafeKVExternalSorter/VariableLengthRowBasedKeyValueBatch should also respect UnsafeAlignedOffset Apr 14, 2020

cloud-fan reviewed Apr 14, 2020

View reviewed changes

Ngone51 added 2 commits April 14, 2020 20:19

fix indent

31bd15e

address comment

bda5823

kiszk reviewed Apr 14, 2020

View reviewed changes

add method comment

5b3adba

Ngone51 added 2 commits April 15, 2020 09:56

allow size=0

0d0f97a

reset uaoSize at the end of test

d441e53

kiszk approved these changes Apr 15, 2020

View reviewed changes

jiangxb1987 reviewed Apr 15, 2020

View reviewed changes

Ngone51 added 2 commits April 15, 2020 20:35

update comment

bc93b52

revert doubleUaoSize

0fb6024

update

c6f39b5

cloud-fan closed this in 40f9dbb Apr 17, 2020

Conversation

Ngone51 commented Apr 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Apr 12, 2020

Uh oh!

Ngone51 commented Apr 13, 2020

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 commented Apr 13, 2020

Uh oh!

kiszk commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ngone51 commented Apr 13, 2020

Uh oh!

Ngone51 commented Apr 13, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 13, 2020

Uh oh!

Ngone51 commented Apr 14, 2020

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 Apr 14, 2020

Uh oh!

SparkQA commented Apr 14, 2020

Uh oh!

Ngone51 commented Apr 14, 2020

Uh oh!

kiszk commented Apr 14, 2020

Uh oh!

kiszk Apr 14, 2020 • 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 Apr 14, 2020

Uh oh!

SparkQA commented Apr 15, 2020

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!

Ngone51 commented Apr 12, 2020 •

edited

Loading

kiszk commented Apr 13, 2020 •

edited

Loading

Ngone51 commented Apr 13, 2020 •

edited

Loading

kiszk Apr 14, 2020 •

edited

Loading

jiangxb1987 Apr 15, 2020 •

edited

Loading

Ngone51 Apr 15, 2020 •

edited

Loading