Add GPU version of ToPrettyString [databricks] #9221

razajafri · 2023-09-12T02:27:15Z

This PR adds a GpuToPrettyString so we don't fall back to the CPU when show() is called in REPL.

The PR was manually tested and unit tests were added

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

revans2

I did a quick pass through much of the code and it feels like there is a lot of refactoring that I have a hard time seeing any value with. I understand ToStringBase so you can override how some functions are implemented, but everything else feels unnecessary.

sql-plugin/src/main/spark350/scala/com/nvidia/spark/rapids/shims/SparkShims.scala

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

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

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

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

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

sql-plugin/src/main/spark350/scala/com/nvidia/spark/rapids/shims/GpuToPrettyString.scala

tests/src/test/spark350/scala/com/nvidia/spark/rapids/ToPrettyStringSuite.scala

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

…ttystring

razajafri · 2023-09-20T19:17:57Z

@revans2 and @andygrove thank you for reviewing.

I have addressed all of your comments I think. PTAL

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

revans2

The only non-nit I have left is what Andy showed about ANSI mode for cast.

revans2 · 2023-09-20T20:31:40Z

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

+    val cast = meta.wrapped.asInstanceOf[UnaryExpression]
+    val from = cast.child.dataType
+    val to = StringType
+    if (!gpuCanCast(from, to)) {


I don't think this is going to show up in the generated docs, even if we tried to build them with 3.5.0. We should at least have a follow on issue to figure that out.

Wondering if there is a valid need to print ToPrettyString as part of the supported_ops. It is a sub set of what GpuCast can do.

…ttystring

…options

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

…P-9171-toprettystring

…ttystring

razajafri · 2023-09-25T21:35:03Z

@andygrove @revans2 PTAL.

razajafri · 2023-09-25T21:35:08Z

build

tests/src/test/spark350/scala/com/nvidia/spark/rapids/ToPrettyStringSuite.scala

razajafri · 2023-09-27T16:22:50Z

@revans2 @andygrove I have addressed your concerns. PTAL

I have also converted this to draft until rapidsai/cudf#14205 is merged

razajafri · 2023-09-27T16:51:19Z

tests/src/test/scala/com/nvidia/spark/rapids/GpuBatchUtilsSuite.scala

@@ -25,6 +25,7 @@ import org.scalatest.funsuite.AnyFunSuite
 import org.apache.spark.sql.Row


I had to make changes to this file because of the way InternalRow accesses Arrays, Maps and Structs when using BoundReference

andygrove · 2023-09-27T19:18:12Z

@mimaomao @kevnzhao fyi, this PR is ready to go, but we are just waiting on rapidsai/cudf#14205 to be merged before we merge this one.

razajafri · 2023-09-28T16:35:38Z

build

razajafri added 4 commits September 8, 2023 09:44

poc working

3711905

Refactoring

d48ab00

enable spark 350 tests

ec8e575

cleanup

b3d238d

razajafri self-assigned this Sep 12, 2023

Signing off

223047e

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

razajafri changed the title ~~Add GPU version of ToPrettyString~~ Add GPU version of ToPrettyString [databricks] Sep 12, 2023

razajafri marked this pull request as draft September 12, 2023 04:15

revans2 reviewed Sep 12, 2023

View reviewed changes

sameerz added the Spark 3.5+ Spark 3.5+ issues label Sep 13, 2023

razajafri added 5 commits September 13, 2023 22:57

added unit tests

3708a2c

addressed review comments

e8869c2

Renamed test

adf05dd

cleanup

09aee4f

add function for getting spark conf values

4bfca7a

revans2 reviewed Sep 18, 2023

View reviewed changes

razajafri added 2 commits September 18, 2023 18:35

fixed scalastyle

f435b61

addressed review comments

57d87fe

andygrove reviewed Sep 20, 2023

View reviewed changes

tests/src/test/spark350/scala/com/nvidia/spark/rapids/ToPrettyStringSuite.scala Outdated Show resolved Hide resolved

andygrove reviewed Sep 20, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated Show resolved Hide resolved

andygrove reviewed Sep 20, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated Show resolved Hide resolved

andygrove reviewed Sep 20, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated Show resolved Hide resolved

razajafri added 2 commits September 20, 2023 11:35

added hexString to show binary to string

8781b4f

Merge remote-tracking branch 'origin/branch-23.10' into SP-9171-topre…

5b4daf8

…ttystring

razajafri marked this pull request as ready for review September 20, 2023 19:17

razajafri requested review from andygrove and revans2 September 20, 2023 19:49

andygrove reviewed Sep 20, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala Outdated Show resolved Hide resolved

revans2 reviewed Sep 20, 2023

View reviewed changes

razajafri added 2 commits September 21, 2023 11:33

Merge remote-tracking branch 'origin/branch-23.10' into SP-9171-topre…

650e25e

…ttystring

refactored cast options

79ccc0b

razajafri mentioned this pull request Sep 21, 2023

[FEA] Refactor GpuCast to take CastOptions #9284

Closed

razajafri added 10 commits September 21, 2023 13:35

Refactored GpuCast to take a CastOptions to make it easer to pass in …

8f3ae34

…options

singing off

d3c6bee

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

Merge remote-tracking branch 'private/SP-9284-add-castoptions' into S…

c1ac19f

…P-9171-toprettystring

addressed review concerns

691120e

addressed review comments

063252c

removed CastOptions to make merging easy

5a92e5c

Merge remote-tracking branch 'private/SP-9284-add-castoptions' into S…

1c765ed

…P-9171-toprettystring

added more tests for basic types

3841994

Merge remote-tracking branch 'origin/branch-23.10' into SP-9171-topre…

c889f97

…ttystring

added more unit tests

8a4ff65

andygrove reviewed Sep 25, 2023

View reviewed changes

tests/src/test/spark350/scala/com/nvidia/spark/rapids/ToPrettyStringSuite.scala Outdated Show resolved Hide resolved

Added tests for more types

8cf22e6

razajafri marked this pull request as draft September 27, 2023 16:20

call toHex() instead of fromStrings()

9ce742a

razajafri requested review from andygrove and revans2 September 27, 2023 16:34

razajafri commented Sep 27, 2023

View reviewed changes

andygrove approved these changes Sep 27, 2023

View reviewed changes

revans2 approved these changes Sep 27, 2023

View reviewed changes

razajafri marked this pull request as ready for review September 28, 2023 16:47

razajafri merged commit d8a5e5e into NVIDIA:branch-23.10 Sep 28, 2023
29 checks passed

razajafri deleted the SP-9171-toprettystring branch September 28, 2023 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPU version of ToPrettyString [databricks] #9221

Add GPU version of ToPrettyString [databricks] #9221

razajafri commented Sep 12, 2023 •

edited

Loading

revans2 left a comment

razajafri commented Sep 20, 2023

revans2 left a comment

revans2 Sep 20, 2023

razajafri Sep 25, 2023

razajafri commented Sep 25, 2023

razajafri commented Sep 25, 2023

razajafri commented Sep 27, 2023

razajafri Sep 27, 2023

andygrove commented Sep 27, 2023

razajafri commented Sep 28, 2023

		@@ -25,6 +25,7 @@ import org.scalatest.funsuite.AnyFunSuite
		import org.apache.spark.sql.Row

Add GPU version of ToPrettyString [databricks] #9221

Add GPU version of ToPrettyString [databricks] #9221

Conversation

razajafri commented Sep 12, 2023 • edited Loading

revans2 left a comment

Choose a reason for hiding this comment

razajafri commented Sep 20, 2023

revans2 left a comment

Choose a reason for hiding this comment

revans2 Sep 20, 2023

Choose a reason for hiding this comment

razajafri Sep 25, 2023

Choose a reason for hiding this comment

razajafri commented Sep 25, 2023

razajafri commented Sep 25, 2023

razajafri commented Sep 27, 2023

razajafri Sep 27, 2023

Choose a reason for hiding this comment

andygrove commented Sep 27, 2023

razajafri commented Sep 28, 2023

razajafri commented Sep 12, 2023 •

edited

Loading