[SPARK-45697][BUILD] Fix `Unicode escapes in triple quoted strings are deprecated` by panbingkun · Pull Request #43603 · apache/spark

panbingkun · 2023-10-31T11:59:40Z

What changes were proposed in this pull request?

The pr aims to fix Unicode escapes in triple quoted strings are deprecated in scala 2.13.

Why are the changes needed?

Eliminate compile warnings.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

Manually test:

./build/sbt "catalyst/testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite"

Was this patch authored or co-authored using generative AI tooling?

No.

…e deprecated`

LuciferYang · 2023-10-31T15:50:42Z

project/SparkBuild.scala

        // reduce the cost of migration in subsequent versions.
-        "-Wconf:cat=deprecation&msg=it will become a keyword in Scala 3:e"
+        "-Wconf:cat=deprecation&msg=it will become a keyword in Scala 3:e",
+        "-Wconf:cat=deprecation&msg=Unicode escapes in triple quoted strings are deprecated:e"


this will be compile error in Scala 3?

Only for test in GA.
I want it to run through GA first and see if there are any other similar issues besides the above modifications.
I will delete it later.

this will nothing in Scala 3.3.1, neither a warning nor an error.

scala 3.3.1

scala 2.13.8

LuciferYang · 2023-10-31T16:13:35Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

          List.empty, List.empty, None, None, false)))

    // verify with ROW FORMAT DELIMETED
+    val collectionItemsTerminated1 = '\u0002'


'\u0002' corresponds to "STX" (Start of Text), a non-printing character used to mark the beginning of text.

'\u0003' corresponds to "ETX" (End of Text), a non-printing character used to mark the end of text.

'\u0004' corresponds to "EOT" (End of Transmission), a non-printing character used to mark the end of transmission.

'\u0005' corresponds to "ENQ" (Enquiry), a non-printing character used to inquire whether the receiving device is ready to receive data.

I'm not sure if this cases really need to test using non-printing characters as delimiters, can we use printable characters for testing, so we can replace them with the actual corresponding characters.

If indeed we need to test using non-printing characters as delimiters, I think we can add a @nowarn annotation to suppress warnings at a fine granularity like:

// verify with ROW FORMAT DELIMITED @nowarn("cat=deprecation") val sqlWithRowFormatDelimiters: String = """ |SELECT TRANSFORM(a, b, c) | ROW FORMAT DELIMITED | FIELDS TERMINATED BY '\t' | COLLECTION ITEMS TERMINATED BY '\u0002' | MAP KEYS TERMINATED BY '\u0003' | LINES TERMINATED BY '\n' | NULL DEFINED AS 'null' | USING 'cat' AS (a, b, c) | ROW FORMAT DELIMITED | FIELDS TERMINATED BY '\t' | COLLECTION ITEMS TERMINATED BY '\u0004' | MAP KEYS TERMINATED BY '\u0005' | LINES TERMINATED BY '\n' | NULL DEFINED AS 'NULL' |FROM testData """.stripMargin assertEqual( sqlWithRowFormatDelimiters,

panbingkun · 2023-11-01T02:31:31Z

…e deprecated`

LuciferYang · 2023-11-01T05:57:41Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

-        |FROM testData
-      """.stripMargin,
+    // verify with ROW FORMAT DELIMITED
+    val sqlWithRowFormatDelimited =


looks a bit ugly ...

LuciferYang · 2023-11-01T06:01:08Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

+    val sqlWithRowFormatDelimited =
+        "SELECT TRANSFORM(a, b, c)" + "\n" +
+        "  ROW FORMAT DELIMITED" + "\n" +
+      """  FIELDS TERMINATED BY '\t'""" + "\n" +


friendly ping @AngersZhuuuu , do you have any suggestions about this deprecated usage issue

panbingkun · 2023-11-06T02:38:53Z

… deprecated

panbingkun · 2023-11-06T02:56:26Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

-        |  NULL DEFINED AS 'NULL'
-        |FROM testData
-      """.stripMargin,
+      "SELECT TRANSFORM(a, b, c)\n" +


Although this writing may seem ugly, it seems better than the previous version, haha

dongjoon-hyun

May I ask the status of this PR, @panbingkun ?

panbingkun · 2023-11-16T23:16:10Z

May I ask the status of this PR, @panbingkun ?

@dongjoon-hyun My modification principle is to eliminate the warning and maintain the original string semantics, but it is indeed ugly;while @LuciferYang 's suggestion is to eliminate the warning, but bypass it with a special annotation.
Which modification method do you think is more appropriate? We would also like to hear your suggestions, many thanks!
(PS: I actually think both are acceptable, but a judgment and decision are needed. 😄)

LuciferYang · 2023-11-17T04:18:42Z

To clarify: I am more inclined to understand the original intent of the test. If the goal is just to test different separators, we can use the literals of visible separators to eliminate this compile warning. If the original intent was to test invisible separators, then I think we can simply suppress the warning in this test.

… deprecated

panbingkun · 2023-11-17T08:52:26Z

To clarify: I am more inclined to understand the original intent of the test. If the goal is just to test different separators, we can use the literals of visible separators to eliminate this compile warning. If the original intent was to test invisible separators, then I think we can simply suppress the warning in this test.

Okay, let's suppress the warning with annotations first.
I tested it locally and it was okay.

LuciferYang · 2023-11-20T03:39:34Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala


    // verify with ROW FORMAT DELIMETED
-    assertEqual(
+    @nowarn("cat=deprecation")


fine-grained suppressing this case with @nowarn if fine to me

LuciferYang · 2023-11-21T07:13:22Z

pom.xml

                reduce the cost of migration in subsequent versions.
              -->
              <arg>-Wconf:cat=deprecation&amp;msg=it will become a keyword in Scala 3:e</arg>
+              <arg>-Wconf:cat=deprecation&amp;msg=Unicode escapes in triple quoted strings are deprecated:e</arg>


@panbingkun please revert the change of SparkBuild.scala and pom.xml

Okay, done.

… deprecated

LuciferYang · 2023-11-22T04:04:28Z

Merged into master for Spark 4.0. Thanks @panbingkun

[SPARK-45697][BUILD] Fix `Unicode escapes in triple quoted strings ar…

d5d5200

…e deprecated`

github-actions bot added SQL BUILD labels Oct 31, 2023

fix pom.xml

005a175

LuciferYang reviewed Oct 31, 2023

View reviewed changes

[SPARK-45697][BUILD] Fix `Unicode escapes in triple quoted strings ar…

c13ef9e

…e deprecated`

panbingkun requested a review from LuciferYang November 1, 2023 03:10

LuciferYang reviewed Nov 1, 2023

View reviewed changes

panbingkun marked this pull request as ready for review November 1, 2023 12:05

panbingkun added 2 commits November 6, 2023 10:52

[SPARK-45697][BUILD] Fix Unicode escapes in triple quoted strings are…

3a9d80f

… deprecated

Merge branch 'master' into SPARK-45697

8abeac5

panbingkun commented Nov 6, 2023

View reviewed changes

dongjoon-hyun reviewed Nov 16, 2023

View reviewed changes

panbingkun added 2 commits November 17, 2023 16:23

Merge branch 'master' into SPARK-45697

2a99240

[SPARK-45697][BUILD] Fix Unicode escapes in triple quoted strings are…

5e84eaf

… deprecated

panbingkun added 2 commits November 17, 2023 23:49

Merge branch 'master' into SPARK-45697

2832827

Merge branch 'master' into SPARK-45697

6956df8

LuciferYang reviewed Nov 20, 2023

View reviewed changes

LuciferYang reviewed Nov 21, 2023

View reviewed changes

panbingkun added 2 commits November 21, 2023 15:34

[SPARK-45697][BUILD] Fix Unicode escapes in triple quoted strings are…

3586bc1

… deprecated

Merge branch 'master' into SPARK-45697

c647f64

github-actions bot removed the BUILD label Nov 21, 2023

LuciferYang approved these changes Nov 21, 2023

View reviewed changes

LuciferYang closed this in 7d3f5bd Nov 22, 2023

Conversation

panbingkun commented Oct 31, 2023 • 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?

Was this patch authored or co-authored using generative AI tooling?

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!

LuciferYang Oct 31, 2023 • 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!

panbingkun commented Nov 1, 2023

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!

panbingkun commented Nov 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LuciferYang commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented Nov 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Nov 21, 2023 • 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!

LuciferYang commented Nov 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

panbingkun commented Oct 31, 2023 •

edited

Loading

LuciferYang Oct 31, 2023 •

edited

Loading

panbingkun commented Nov 16, 2023 •

edited

Loading

LuciferYang commented Nov 17, 2023 •

edited

Loading

panbingkun Nov 21, 2023 •

edited

Loading