Xxhash64 supports nested types [databricks] #11859

res-life · 2024-12-11T07:55:24Z

The JNI PR: Add xxhash64 support for nested types is initially required by Hyper Log Log Plus Plus(HLLPP).

This PR is used to

verify the correctness of xxhash64 result for nested types for HLLPP
BTW: Add support for nested type.

This PR contains:

Supports nested types for xxhash64
Add fallback when the stack depth required by nested type is bigger than 8, refer to how to calculate stack depth

Signed-off-by: Chong Gao [email protected]

res-life · 2024-12-11T07:57:51Z

After compile/install the JNI with patch: NVIDIA/spark-rapids-jni#2575, all the cases for nested types passed.
@ustcfy @ttnghia We can merge JNI PR, because this PR verified the correctness.

Signed-off-by: Chong Gao <[email protected]>

res-life · 2024-12-18T07:42:49Z

build

res-life · 2024-12-18T10:15:31Z

build

res-life · 2024-12-18T10:20:37Z

Map is list of struct in cuDF.
Yan Feng mentioned a better approach to check the max stack depth without flatMap:

  /**
   * Convert map to list of struct.
   * Note: Do not support UserDefinedType and other unregular types.
   * Do not retain the nullable and other info
   */
  private def flatMap(inputType: DataType): DataType = {
    inputType match {
      case mapType: MapType =>
        ArrayType(StructType(Array(
          StructField("key", flatMap(mapType.keyType)),
          StructField("value", flatMap(mapType.valueType))
        )))
      case arrayType: ArrayType => ArrayType(flatMap(arrayType.elementType))
      case structType: StructType =>
        StructType(structType.map(f => StructField(f.name, flatMap(f.dataType))).toArray)
      case nullType: NullType => nullType
      case atomicType: AtomicType => atomicType
      case other => throw new RuntimeException(s"Unsupported type: $other")
    }
  }

Yan Feng will file a follow-up PR to do the improvement.

ustcfy · 2024-12-18T10:26:16Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/HashFunctions.scala

+      case ArrayType(c: DataType, _) => computeMaxStackSizeForFlatten(c)
+      case st: StructType =>
+        1 + st.map(f => computeMaxStackSizeForFlatten(f.dataType)).max
+      case _ => 1 // primitive types


Perhaps we don't need the flatMap function and can directly compute the depth of the map type using

case mt: MapType => 2 + math.max(computeMaxStackSizeForFlatten(mt.keyType), computeMaxStackSizeForFlatten(mt.valueType))

+1 for not rewriting the data type to check the depth.

Thanks, let's post a follow-up PR to do the improvement

@res-life why? It takes 5 mins and I want the fallback message updated too?

Updated. commit

revans2 · 2024-12-18T13:53:54Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/HashFunctions.scala

+      case ArrayType(c: DataType, _) => computeMaxStackSizeForFlatten(c)
+      case st: StructType =>
+        1 + st.map(f => computeMaxStackSizeForFlatten(f.dataType)).max
+      case _ => 1 // primitive types


+1 for not rewriting the data type to check the depth.

revans2 · 2024-12-18T13:55:35Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

+          val maxDepth = a.children.map(
+            c => XxHash64Utils.computeMaxStackSize(c.dataType)).max
+          if (maxDepth > Hash.MAX_STACK_DEPTH) {
+            willNotWorkOnGpu(s"The data type requires a stack size of $maxDepth, " +


This is not very clear because Maps count as a depth of 2. Users who see this will be confused, because they will try to add things up manually and it will not work out. At a minimum we need to mention that Maps count as a depth of 2 here.

Updated. commit

res-life · 2024-12-19T03:13:48Z

build

ttnghia · 2024-12-19T03:57:47Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/HashFunctions.scala

+      case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)
+      case ArrayType(c: DataType, _) => computeMaxStackSize(c)


Can this be combined like this?

Suggested change

case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)

case ArrayType(c: DataType, _) => computeMaxStackSize(c)

case ArrayType(c: _, _) => computeMaxStackSize(c)

No, in JNI, list(other type) is optmized to use less stack depth, only use depthOf(other).
But for list(struct), should be 1 + depthOf(struct).

NIT:

Suggested change

case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)

case ArrayType(c: DataType, _) => computeMaxStackSize(c)

case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)

case ArrayType(c, _) => computeMaxStackSize(c)

integration_tests/src/main/python/hashing_test.py

firestarman · 2024-12-19T07:41:26Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/HashFunctions.scala

+      case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)
+      case ArrayType(c: DataType, _) => computeMaxStackSize(c)


NIT:

Suggested change

case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)

case ArrayType(c: DataType, _) => computeMaxStackSize(c)

case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)

case ArrayType(c, _) => computeMaxStackSize(c)

res-life · 2024-12-19T10:57:53Z

build

res-life requested review from ttnghia and ustcfy December 11, 2024 07:55

res-life changed the title ~~Xxhash64 supports nested types~~ [Do not review]Xxhash64 supports nested types Dec 11, 2024

res-life mentioned this pull request Dec 11, 2024

Add xxhash64 support for nested types NVIDIA/spark-rapids-jni#2575

Merged

Xxhash64 supports nested types

5bf3c78

Signed-off-by: Chong Gao <[email protected]>

res-life force-pushed the xxhash64-support-nested-type branch from 725c25a to 5bf3c78 Compare December 11, 2024 08:39

ttnghia previously approved these changes Dec 11, 2024

View reviewed changes

res-life mentioned this pull request Dec 13, 2024

[WIP] Add support for Hyper Log Log PLus Plus(HLL++) #11638

Draft

Chong Gao added 5 commits December 18, 2024 11:07

Merge branch 'branch-25.02' into xxhash64-support-nested-type

97b8482

Check max stack size required

3e982c8

Fix compile error

f2f495a

Update test case

89f996b

Fix bug; Update test case

27bf6f7

res-life dismissed ttnghia’s stale review via 27bf6f7 December 18, 2024 07:24

res-life changed the title ~~[Do not review]Xxhash64 supports nested types~~ Xxhash64 supports nested types Dec 18, 2024

Update code comment

d47d146

res-life changed the title ~~Xxhash64 supports nested types~~ Xxhash64 supports nested types [databricks] Dec 18, 2024

res-life marked this pull request as ready for review December 18, 2024 07:42

Chong Gao added 2 commits December 18, 2024 15:45

Update License

4101fa4

Update supportedExprs.csv

51c9227

ustcfy reviewed Dec 18, 2024

View reviewed changes

res-life requested a review from ttnghia December 18, 2024 13:54

revans2 reviewed Dec 18, 2024

View reviewed changes

Address comments

253c6f4

ttnghia reviewed Dec 19, 2024

View reviewed changes

ustcfy previously approved these changes Dec 19, 2024

View reviewed changes

firestarman reviewed Dec 19, 2024

View reviewed changes

Add more edge cases; Address a comment

8270ea0

res-life dismissed ustcfy’s stale review via 8270ea0 December 19, 2024 10:57

revans2 approved these changes Dec 19, 2024

View reviewed changes

res-life merged commit adb89aa into NVIDIA:branch-25.02 Dec 19, 2024
49 of 50 checks passed

res-life deleted the xxhash64-support-nested-type branch December 19, 2024 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xxhash64 supports nested types [databricks] #11859

Xxhash64 supports nested types [databricks] #11859

res-life commented Dec 11, 2024 •

edited

Loading

res-life commented Dec 11, 2024

res-life commented Dec 18, 2024

res-life commented Dec 18, 2024

res-life commented Dec 18, 2024

ustcfy Dec 18, 2024

revans2 Dec 18, 2024

res-life Dec 18, 2024

revans2 Dec 18, 2024

res-life Dec 19, 2024 •

edited

Loading

revans2 Dec 18, 2024

revans2 Dec 18, 2024

res-life Dec 19, 2024 •

edited

Loading

res-life commented Dec 19, 2024

ttnghia Dec 19, 2024

res-life Dec 19, 2024

firestarman Dec 19, 2024

firestarman Dec 19, 2024

res-life commented Dec 19, 2024

		case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)
		case ArrayType(c: DataType, _) => computeMaxStackSize(c)

	case ArrayType(c: StructType, _) => 1 + computeMaxStackSize(c)
	case ArrayType(c: DataType, _) => computeMaxStackSize(c)
	case ArrayType(c: _, _) => computeMaxStackSize(c)

Xxhash64 supports nested types [databricks] #11859

Xxhash64 supports nested types [databricks] #11859

Conversation

res-life commented Dec 11, 2024 • edited Loading

This PR is used to

This PR contains:

res-life commented Dec 11, 2024

res-life commented Dec 18, 2024

res-life commented Dec 18, 2024

res-life commented Dec 18, 2024

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

res-life Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

res-life Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

res-life commented Dec 19, 2024

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

res-life commented Dec 19, 2024

res-life commented Dec 11, 2024 •

edited

Loading

res-life Dec 19, 2024 •

edited

Loading

res-life Dec 19, 2024 •

edited

Loading