Fix `from_json` function failure when input contains empty or null strings #8526

cindyyuanjiang · 2023-06-07T22:57:06Z

During testing, we found from_json function throws a java.lang.ArrayIndexOutOfBoundsException (from ai.rapids.cudf.TableWithMeta.releaseTable) when the input column contains empty or null strings. When pre-processing the input columns, we have replaced all empty string or null rows in the input column with "{}", but calling ai.rapids.cudf.Table.readJSON on the input column (containing other valid json strings) would result in tables with different number of columns, resulting in future ArrayIndexOutOfBoundsException since rows contain tables of different sizes.
=> To fix this issue, instead of replacing empty strings/nulls with "{}", we replace them with a dummy json string based on the input schema. For example, if schema is StructType([StructField("a", StringType), StructField("b", IntegerType)]), then we will replace empty strings and nulls with "{"a": "", "b": 0}". This ensures that all rows contain tables with the same number of columns after readJSON.

When testing the above solution, we encountered another issue with strip function: rapidsai/cudf#13529. The problem only occurs when the input column contains only empty strings and nulls. Our approach is to first check the data buffer for the input string column, and if it is empty, we do not call strip and just call incRefCount on the origin column.

Follow-up issue tracked: #8542

Signed-off-by: Cindy Jiang <[email protected]>

cindyyuanjiang · 2023-06-07T23:07:11Z

build

firestarman · 2023-06-08T02:18:36Z

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

+        val res = struct.fields.foldRight ("") ((field, acc) => 
+          field.dataType match {
+            case IntegerType => "\"" + field.name + "\": 0, " + acc
+            case _ => "\"" + field.name + "\": \"\", " + acc


Do we need to take care of a nested StructType here ?

This is a really inefficient fix in terms of memory usage on the GPU. I fixed most of this in CUDF with rapidsai/cudf#13477 except for the one use case that you are testing in the python code.

Can we have a follow on issue to only do this if all of the strings are empty strings or '{}' in 23.08? We technically only need one row to have this in it and the rest can be empty, but trying to make that happen feels more expensive than just expanding this out everywhere.

@firestarman Yes, thank you! I will update this to take care of nested StructType.

@revans2 I will file a follow up issue in 23.08. Thank you for pointing this out!

integration_tests/src/main/python/json_test.py

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

integration_tests/src/main/python/json_test.py

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

integration_tests/src/main/python/json_test.py

revans2 · 2023-06-08T14:39:13Z

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

+        val res = struct.fields.foldRight ("") ((field, acc) => 
+          field.dataType match {
+            case IntegerType => "\"" + field.name + "\": 0, " + acc
+            case _ => "\"" + field.name + "\": \"\", " + acc


This is a really inefficient fix in terms of memory usage on the GPU. I fixed most of this in CUDF with rapidsai/cudf#13477 except for the one use case that you are testing in the python code.

Can we have a follow on issue to only do this if all of the strings are empty strings or '{}' in 23.08? We technically only need one row to have this in it and the rest can be empty, but trying to make that happen feels more expensive than just expanding this out everywhere.

revans2 · 2023-06-08T14:59:26Z

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

+        input.incRefCount
+      } else {
+        withResource(cudf.Scalar.fromString(" ")) { space =>
+          input.strip(space)


Can we look for other places that strip, rstrip, and lstrip are called? I did a quick look and I think we might get the same problem on

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

Line 386 in e3b9517

col.rstrip()

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

Line 306 in e3b9517

GpuColumnVector.from(column.getBase.rstrip(t), dataType)

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

Line 286 in e3b9517

GpuColumnVector.from(column.getBase.lstrip(t), dataType)

But there may be others.

At a minimum we need to test these too and fix them if they are also broken

@revans2 Thanks! I tested GpuStringTrim[Left, Right] implementation above. The same problem comes up. I added integration tests and fixes for them.

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

Line 386 in e3b9517

col.rstrip()

I didn't find tests for the above rstrip() call site, and haven't added a fix yet.

I think that is fine. That is specifically for a special case ORC char type. We should file an issue for it just so it is not missed, but it should be really rare.

Signed-off-by: Cindy Jiang <[email protected]>

cindyyuanjiang · 2023-06-09T00:30:01Z

Added fix and tests for the following strip() call site:

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

Line 266 in e3b9517

GpuColumnVector.from(column.getBase.strip(t), dataType)

Signed-off-by: Cindy Jiang <[email protected]>

cindyyuanjiang · 2023-06-09T00:47:13Z

build

integration_tests/src/main/python/json_test.py

Signed-off-by: Cindy Jiang <[email protected]>

cindyyuanjiang · 2023-06-09T01:45:58Z

build

revans2 · 2023-06-09T14:12:08Z

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

+      case struct: StructType if (struct.fields.length > 0) => {
+        val jsonFields: Array[String] = struct.fields.map { field => 
+          field.dataType match {
+            case IntegerType => s""""${field.name}": 0"""


Sorry, does Spark really return a 0 and not a null for an empty int column???

What happens if a field name has odd characters in it? Can it have a " for example?

I am not certain about Spark side. I will investigate more. For odd characters, I think it would fail at readJSON parsing step. I will follow up with this in #8542. Thank you so much!

initial fix for from_json function

1807edf

Signed-off-by: Cindy Jiang <[email protected]>

sameerz added the bug Something isn't working label Jun 7, 2023

cindyyuanjiang self-assigned this Jun 7, 2023

cindyyuanjiang linked an issue Jun 7, 2023 that may be closed by this pull request

[BUG] from_json function failed testing with input column containing empty or null string #8522

Closed

firestarman reviewed Jun 8, 2023

View reviewed changes

gerashegalov reviewed Jun 8, 2023

View reviewed changes

revans2 reviewed Jun 8, 2023

View reviewed changes

cindyyuanjiang added 4 commits June 8, 2023 15:20

cleaned up from json tests and added one test for struct of struct

f1092fb

Signed-off-by: Cindy Jiang <[email protected]>

addressed review comments

2f7e2ad

Signed-off-by: Cindy Jiang <[email protected]>

add integration tests and workaround for lstrip and rstrip call sites

5674788

Signed-off-by: Cindy Jiang <[email protected]>

add integration tests and fix for GpuStringTrim

b07674d

Signed-off-by: Cindy Jiang <[email protected]>

revert a fix for rstrip

f6583c5

Signed-off-by: Cindy Jiang <[email protected]>

gerashegalov previously approved these changes Jun 9, 2023

View reviewed changes

integration_tests/src/main/python/json_test.py Outdated Show resolved Hide resolved

addressed review comments and updated tests

9a003c3

Signed-off-by: Cindy Jiang <[email protected]>

cindyyuanjiang dismissed gerashegalov’s stale review via 9a003c3 June 9, 2023 01:45

gerashegalov approved these changes Jun 9, 2023

View reviewed changes

cindyyuanjiang merged commit 8f967e2 into NVIDIA:branch-23.06 Jun 9, 2023

cindyyuanjiang deleted the fix-gpu-from-json-func branch June 9, 2023 05:04

cindyyuanjiang mentioned this pull request Jun 9, 2023

Improve fix for from_json function for empty data input column #8542

Closed

2 tasks

revans2 reviewed Jun 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `from_json` function failure when input contains empty or null strings #8526

Fix `from_json` function failure when input contains empty or null strings #8526

cindyyuanjiang commented Jun 7, 2023 •

edited

Loading

cindyyuanjiang commented Jun 7, 2023

firestarman Jun 8, 2023

revans2 Jun 8, 2023

cindyyuanjiang Jun 8, 2023

cindyyuanjiang Jun 8, 2023

revans2 Jun 8, 2023

revans2 Jun 8, 2023

cindyyuanjiang Jun 9, 2023 •

edited

Loading

revans2 Jun 9, 2023

cindyyuanjiang commented Jun 9, 2023 •

edited

Loading

cindyyuanjiang commented Jun 9, 2023

cindyyuanjiang commented Jun 9, 2023

revans2 Jun 9, 2023

cindyyuanjiang Jun 9, 2023

Fix from_json function failure when input contains empty or null strings #8526

Fix from_json function failure when input contains empty or null strings #8526

Conversation

cindyyuanjiang commented Jun 7, 2023 • edited Loading

cindyyuanjiang commented Jun 7, 2023

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

Choose a reason for hiding this comment

cindyyuanjiang Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindyyuanjiang commented Jun 9, 2023 • edited Loading

cindyyuanjiang commented Jun 9, 2023

cindyyuanjiang commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `from_json` function failure when input contains empty or null strings #8526

Fix `from_json` function failure when input contains empty or null strings #8526

cindyyuanjiang commented Jun 7, 2023 •

edited

Loading

cindyyuanjiang Jun 9, 2023 •

edited

Loading

cindyyuanjiang commented Jun 9, 2023 •

edited

Loading