[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null #42434

andygrove · 2023-08-10T16:50:00Z

As noted by @cloud-fan #41432 (comment), #41432 fixed a formatting issue when casting map to string but did not fix it in the codegen case.

This PR fixes the issue and updates a unit test to test the codegen path. Without the fix, the test fails with:

- SPARK-22973 Cast map to string *** FAILED ***
  Incorrect evaluation (fallback mode = CODEGEN_ONLY): cast(map(keys: [1,2,3], values: [null,[B@5c3fd9f3,[B@70f84210]) as string), 
  actual: {1 ->null, 2 -> a, 3 -> c}, 
expected: {1 -> null, 2 -> a, 3 -> c} (ExpressionEvalHelper.scala:270)

andygrove · 2023-08-10T16:53:23Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToStringBase.scala

@@ -352,7 +352,7 @@ trait ToStringBase { self: UnaryExpression with TimeZoneAwareExpression =>
       |  $buffer.append($keyToStringFunc($getMapFirstKey));
       |  $buffer.append(" ->");
       |  if ($map.valueArray().isNullAt(0)) {
-       |    ${appendNull(buffer, isFirstElement = true)}
+       |    ${appendNull(buffer, isFirstElement = false)}


There should always be a space after -> and before the value. It does not matter if this is the first key-value pair or not.

dongjoon-hyun

Thank you, @andygrove .

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

tgravescs

+1, LGTM

cloud-fan · 2023-08-11T00:13:13Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

+      checkEvaluation(expr, catalystValue, inputRow)
+    } else {
+      checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)
+    }
    checkEvaluationWithMutableProjection(expr, catalystValue, inputRow)
    if (GenerateUnsafeProjection.canSupport(expr.dataType)) {
      checkEvaluationWithUnsafeProjection(expr, catalystValue, inputRow)


The test already checks codegen. We don't need to touch this file, just update the test to put null in the first element.

Thanks. I reverted that change and confirmed that the test still fails if I remove the fix.

tgravescs · 2023-08-14T13:28:35Z

+1. @cloud-fan, this ok with you now?

cloud-fan · 2023-08-14T13:50:26Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala

@@ -826,9 +826,9 @@ abstract class CastSuiteBase extends SparkFunSuite with ExpressionEvalHelper {
        val ret1 = cast(Literal.create(Map(1 -> "a", 2 -> "b", 3 -> "c")), StringType)
        checkEvaluation(ret1, s"${lb}1 -> a, 2 -> b, 3 -> c$rb")
        val ret2 = cast(
-          Literal.create(Map("1" -> "a".getBytes, "2" -> null, "3" -> "c".getBytes)),
+          Literal.create(Map("1" -> null, "2" -> "a".getBytes, "3" -> "c".getBytes)),


can we also have a null value in the middle? so that we can have test coverage for null in different positions.

cloud-fan · 2023-08-15T02:12:35Z

thanks, merging to master/3.5!

…rst value is null As noted by cloud-fan #41432 (comment), #41432 fixed a formatting issue when casting map to string but did not fix it in the codegen case. This PR fixes the issue and updates a unit test to test the codegen path. Without the fix, the test fails with: ``` - SPARK-22973 Cast map to string *** FAILED *** Incorrect evaluation (fallback mode = CODEGEN_ONLY): cast(map(keys: [1,2,3], values: [null,[B5c3fd9f3,[B70f84210]) as string), actual: {1 ->null, 2 -> a, 3 -> c}, expected: {1 -> null, 2 -> a, 3 -> c} (ExpressionEvalHelper.scala:270) ``` Closes #42434 from andygrove/space-before-null-cast-codegen. Authored-by: Andy Grove <andygrove73@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 7e52169) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

yaooqinn · 2023-08-15T03:50:19Z

Late LGTM

…rst value is null As noted by cloud-fan apache#41432 (comment), apache#41432 fixed a formatting issue when casting map to string but did not fix it in the codegen case. This PR fixes the issue and updates a unit test to test the codegen path. Without the fix, the test fails with: ``` - SPARK-22973 Cast map to string *** FAILED *** Incorrect evaluation (fallback mode = CODEGEN_ONLY): cast(map(keys: [1,2,3], values: [null,[B5c3fd9f3,[B70f84210]) as string), actual: {1 ->null, 2 -> a, 3 -> c}, expected: {1 -> null, 2 -> a, 3 -> c} (ExpressionEvalHelper.scala:270) ``` Closes apache#42434 from andygrove/space-before-null-cast-codegen. Authored-by: Andy Grove <andygrove73@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

always include space before null value when casting map to string

05b0f83

github-actions bot added the SQL label Aug 10, 2023

andygrove commented Aug 10, 2023

View reviewed changes

andygrove closed this Aug 10, 2023

andygrove reopened this Aug 10, 2023

This was referenced Aug 10, 2023

[WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value in codegen when first value is null #42433

Closed

[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value #41432

Closed

dongjoon-hyun reviewed Aug 10, 2023

View reviewed changes

add unit test for codegen path for map to string with first value null

99e4874

andygrove changed the title ~~[WIP][SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null~~ [SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null Aug 10, 2023

tgravescs reviewed Aug 10, 2023

View reviewed changes

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala Outdated Show resolved Hide resolved

improve test

36c43ca

tgravescs reviewed Aug 10, 2023

View reviewed changes

andygrove mentioned this pull request Aug 10, 2023

Initial support for Spark 3.5.0-rc1 [databricks] NVIDIA/spark-rapids#8969

Merged

cloud-fan reviewed Aug 11, 2023

View reviewed changes

address feedback

d78a4f4

cloud-fan reviewed Aug 14, 2023

View reviewed changes

cloud-fan approved these changes Aug 14, 2023

View reviewed changes

include null in middle of map entries

5d6ed87

cloud-fan closed this in 7e52169 Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null #42434

[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null #42434

andygrove commented Aug 10, 2023 •

edited

andygrove Aug 10, 2023

dongjoon-hyun left a comment

tgravescs left a comment

cloud-fan Aug 11, 2023 •

edited

andygrove Aug 11, 2023

tgravescs commented Aug 14, 2023

cloud-fan Aug 14, 2023

andygrove Aug 14, 2023

cloud-fan commented Aug 15, 2023

yaooqinn commented Aug 15, 2023

[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null #42434

[SPARK-43063][SQL][FOLLOWUP] Add a space between -> and value when first value is null #42434

Conversation

andygrove commented Aug 10, 2023 • edited

andygrove Aug 10, 2023

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

tgravescs left a comment

Choose a reason for hiding this comment

cloud-fan Aug 11, 2023 • edited

Choose a reason for hiding this comment

andygrove Aug 11, 2023

Choose a reason for hiding this comment

tgravescs commented Aug 14, 2023

cloud-fan Aug 14, 2023

Choose a reason for hiding this comment

andygrove Aug 14, 2023

Choose a reason for hiding this comment

cloud-fan commented Aug 15, 2023

yaooqinn commented Aug 15, 2023

andygrove commented Aug 10, 2023 •

edited

cloud-fan Aug 11, 2023 •

edited