You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2023/11/26 17:10:14 UTC
[PR] [WIP][SQL] Restrict charsets in `encode()` [spark]
MaxGekk opened a new pull request, #44020:
URL: https://github.com/apache/spark/pull/44020
### What changes were proposed in this pull request?
In the PR, I propose to restrict the supported charsets in the `encode()` functions by the list from [the doc](https://spark.apache.org/docs/latest/api/sql/#encode):
```
'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16'
```
and introduce the SQL config `spark.sql.legacy.javaCharsets` for restoring the previous behaviour.
### Why are the changes needed?
Currently the list of supported charsets in `encode()` fully depends on the used JDK version, and don't reflect the expression description.
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
By running new checks:
```
$ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z string-functions.sql"
```
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #44020: [SPARK-46115][SQL] Restrict charsets in `encode()`
URL: https://github.com/apache/spark/pull/44020
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44020:
URL: https://github.com/apache/spark/pull/44020#discussion_r1406966593
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2685,9 +2685,12 @@ case class StringDecode(bin: Expression, charset: Expression)
since = "1.5.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Encode(value: Expression, charset: Expression)
+case class Encode(value: Expression, charset: Expression, legacyCharsets: Boolean)
Review Comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][SQL] Restrict charsets in `encode()` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44020:
URL: https://github.com/apache/spark/pull/44020#discussion_r1405513202
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -4584,6 +4584,15 @@ object SQLConf {
.checkValue(_ > 0, "The number of stack traces in the DataFrame context must be positive.")
.createWithDefault(1)
+ val LEGACY_JAVA_CHARSETS = buildConf("spark.sql.legacy.javaCharsets")
+ .internal()
+ .doc("When set to true, the functions like `encode()` can use charsets from JDK while " +
+ "encoding or decoding string values. If it is false, such functions support only one of " +
+ "the charsets: 'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16'.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(false)
Review Comment:
should probably update migration guide
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44020:
URL: https://github.com/apache/spark/pull/44020#discussion_r1406089773
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2696,7 +2699,13 @@ case class Encode(value: Expression, charset: Expression)
protected override def nullSafeEval(input1: Any, input2: Any): Any = {
val toCharset = input2.asInstanceOf[UTF8String].toString
try {
- input1.asInstanceOf[UTF8String].toString.getBytes(toCharset)
+ if (legacyCharsets ||
+ toCharset.equalsIgnoreCase("UTF-8") || toCharset.equalsIgnoreCase("US-ASCII") ||
+ toCharset.equalsIgnoreCase("ISO-8859-1") ||
+ toCharset.equalsIgnoreCase("UTF-16") || toCharset.equalsIgnoreCase("UTF-16LE") ||
+ toCharset.equalsIgnoreCase("UTF-16BE")) {
Review Comment:
Shall we add `val supportedCharsets = Set("UTF-8", ...)`? So we can simplify these code.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2685,9 +2685,12 @@ case class StringDecode(bin: Expression, charset: Expression)
since = "1.5.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Encode(value: Expression, charset: Expression)
+case class Encode(value: Expression, charset: Expression, legacyCharsets: Boolean)
Review Comment:
Do we really need the new parameter? It seems we can use `SQLConf.get.legacyJavaCharsets` directly.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -4584,6 +4584,15 @@ object SQLConf {
.checkValue(_ > 0, "The number of stack traces in the DataFrame context must be positive.")
.createWithDefault(1)
+ val LEGACY_JAVA_CHARSETS = buildConf("spark.sql.legacy.javaCharsets")
+ .internal()
+ .doc("When set to true, the functions like `encode()` can use charsets from JDK while " +
+ "encoding or decoding string values. If it is false, such functions support only one of " +
+ "the charsets: 'US-ASCII', 'ISO-8859-1', 'UTF-8', 'UTF-16BE', 'UTF-16LE', 'UTF-16'.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(false)
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #44020:
URL: https://github.com/apache/spark/pull/44020#discussion_r1406434225
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2685,9 +2685,12 @@ case class StringDecode(bin: Expression, charset: Expression)
since = "1.5.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Encode(value: Expression, charset: Expression)
+case class Encode(value: Expression, charset: Expression, legacyCharsets: Boolean)
Review Comment:
This avoids some issues, see the motivation in the PR's description: https://github.com/apache/spark/pull/27658
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #44020:
URL: https://github.com/apache/spark/pull/44020#issuecomment-1828819178
Merged to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46115][SQL] Restrict charsets in `encode()` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #44020:
URL: https://github.com/apache/spark/pull/44020#discussion_r1406434559
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2696,7 +2699,13 @@ case class Encode(value: Expression, charset: Expression)
protected override def nullSafeEval(input1: Any, input2: Any): Any = {
val toCharset = input2.asInstanceOf[UTF8String].toString
try {
- input1.asInstanceOf[UTF8String].toString.getBytes(toCharset)
+ if (legacyCharsets ||
+ toCharset.equalsIgnoreCase("UTF-8") || toCharset.equalsIgnoreCase("US-ASCII") ||
+ toCharset.equalsIgnoreCase("ISO-8859-1") ||
+ toCharset.equalsIgnoreCase("UTF-16") || toCharset.equalsIgnoreCase("UTF-16LE") ||
+ toCharset.equalsIgnoreCase("UTF-16BE")) {
Review Comment:
Added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org