You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2026/09/26 16:06:18 UTC
[PR] [SPARK-59801][SQL] Fix internal error for non-foldable options maps [spark]
cloud-fan opened a new pull request, #59072:
URL: https://github.com/apache/spark/pull/59072
### What changes were proposed in this pull request?
Check whether SQL function option maps are foldable before evaluating them in
`ExprUtils.convertToMapData`. Non-foldable maps now raise the new structured error
`INVALID_OPTIONS.NON_FOLDABLE`.
This also adds a regression test for `to_csv` with an options map containing an input column.
### Why are the changes needed?
SQL functions require their options maps to be foldable. Previously,
`ExprUtils.convertToMapData` evaluated a non-foldable `CreateMap` during analysis. If the map
contained an `AttributeReference`, evaluating it raised `INTERNAL_ERROR` instead of reporting an
invalid query.
### Does this PR introduce _any_ user-facing change?
Yes. A SQL function called with a non-foldable options map now returns the analysis error
`INVALID_OPTIONS.NON_FOLDABLE` instead of `INTERNAL_ERROR`. Successfully analyzed queries are
unchanged.
### How was this patch tested?
Added a regression test to `CsvFunctionsSuite` and ran:
```
build/mvn -pl sql/core -am -Dtest=none \
-DwildcardSuites=org.apache.spark.sql.CsvFunctionsSuite test
```
All 61 tests passed.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59072:
URL: https://github.com/apache/spark/pull/59072#issuecomment-5885794576
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/b03b53dcacb42c9c31c0cd2b622d495d60c3f59f
- merged into branch-4.x https://github.com/apache/spark/commit/a5dbf9560ccf4d0cc1fb59d1fc20d42b122f8237
*Posted by `merge_spark_pr.py`*
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4115778971
##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -439,6 +439,21 @@ class CsvFunctionsSuite extends SharedSparkSession {
checkAnswer(df1.selectExpr("to_csv(a)"), Row("1") :: Nil)
}
+ test("to_csv with non-foldable options") {
Review Comment:
Added JSON and XML coverage and a lambda-variable regression test. I am targeting `master` and `branch-4.x` only, so I am not planning a `branch-4.2` backport.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -4841,6 +4841,11 @@
"Invalid options:"
],
"subClass" : {
+ "NON_FOLDABLE" : {
+ "message" : [
+ "Must use a foldable `map()` function for options."
Review Comment:
Agreed. The updated patch reuses `NON_FOLDABLE_ARGUMENT` and `QueryCompilationErrors.nonFoldableArgumentError`, passes the function name from all nine CSV/JSON/XML call sites, and removes `INVALID_OPTIONS.NON_FOLDABLE`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -65,6 +65,9 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable) {
Review Comment:
Thanks for identifying these compatibility cases. I agree that requiring `foldable` is the consistent contract used by other functions. The updated patch enforces it by default, adds `spark.sql.legacy.allowNonFoldableOptions=true` to preserve the old analysis-time evaluation when needed, and documents the migration in the 4.4 guide. The tests cover `nvl`, `elt`, higher-order `transform`, and the guarded `raise_error` case under both the default and legacy behavior.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4117295734
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7557,6 +7557,15 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val LEGACY_ALLOW_NON_FOLDABLE_OPTIONS =
Review Comment:
This new config fails `SparkConfigBindingPolicySuite` in both `sql - other tests` and `hive - other tests` CI jobs.
```
[info] - Config enforcement for bindingPolicy *** FAILED ***
The following configs do not have bindingPolicy field set. ...
spark.sql.legacy.allowNonFoldableOptions
```
Could you add `.withBindingPolicy(ConfigBindingPolicy.SESSION)`? `PERSISTED` would not work for this legacy config because the existing views created by Spark 4.3 or older don't have a captured value of it. Those views would always use the default value `false`, so users couldn't use this config to read them.
##########
AGENTS.md:
##########
@@ -101,7 +101,7 @@ These are combined with a base above rather than used on their own:
## Build and Test
-Build and tests can take a long time. If the user explicitly asked to run tests, run them. Otherwise (you are running tests on your own to verify a change), first ask the user if they have more changes to make.
+Build and tests can take a long time, so prefer the narrowest relevant build or test command.
Review Comment:
This looks unrelated to SPARK-59801. Could you spin off this `AGENTS.md` change to a separate PR?
##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -439,6 +439,66 @@ class CsvFunctionsSuite extends SharedSparkSession {
checkAnswer(df1.selectExpr("to_csv(a)"), Row("1") :: Nil)
}
+ test("to_csv with non-foldable options") {
Review Comment:
Could you add the JIRA ID to the test name, e.g., `SPARK-59801: to_csv with non-foldable options`?
In addition, this PR changes 9 call sites, and each has its own hard-coded function name. Only `to_csv`, `to_json` and `to_xml` are covered now. Could you add test cases for `from_csv`, `schema_of_csv`, `from_json`, `schema_of_json`, `from_xml` and `schema_of_xml` too? `sql-tests/inputs/csv-functions.sql` and `json-functions.sql` already have the other `INVALID_OPTIONS` cases, so they could be a good place as well.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
Review Comment:
`RuntimeReplaceable` expressions like `nvl`, `nvl2` and `left` are non-foldable before `ReplaceExpressions`. So, constant options like `map('delimiter', nvl(NULL, '|'))` worked before but fail by default now. `ExprUtils` already extends `EvalHelper`, so what about checking `prepareForEval(m).foldable` instead? `nvl(NULL, '|')` becomes `Coalesce(NULL, '|')`, which is foldable. This would make the breaking change smaller.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
+ !SQLConf.get.getConf(SQLConf.LEGACY_ALLOW_NON_FOLDABLE_OPTIONS)) {
+ throw QueryCompilationErrors.nonFoldableArgumentError(
+ functionName, "options", m.dataType)
Review Comment:
nit. The other errors from this method use `INVALID_OPTIONS.*` (SQLSTATE `42K06`), while this one uses `NON_FOLDABLE_ARGUMENT` (`42K08`). Reusing the existing condition is fine with me. I'm only mentioning it in case you want all option-map errors under `INVALID_OPTIONS`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
+ !SQLConf.get.getConf(SQLConf.LEGACY_ALLOW_NON_FOLDABLE_OPTIONS)) {
Review Comment:
When `spark.sql.legacy.allowNonFoldableOptions=true`, this falls back to `m.eval()` for row-dependent maps too. So, the legacy mode brings back the original bug.
- A column reference, e.g., `to_csv(a, map('delimiter', delimiter))`, hits `INTERNAL_ERROR` again.
- A lambda variable is silently evaluated as `null` (`NamedLambdaVariable.value` is not bound yet), e.g., `transform(array('|'), x -> to_csv(named_struct('a', 1, 'b', 2), map('delimiter', x)))` uses the string `null` as the delimiter.
Shall we keep rejecting the maps with `m.references.nonEmpty` (it covers both `AttributeReference` and `NamedLambdaVariable`) even in the legacy mode, and relax only the evaluable constant cases?
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118948714
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
Review Comment:
Updated in f75b84894ba. Constant RuntimeReplaceable options are normalized recursively and accepted by default. Other deterministic, row-independent non-foldable constants remain behind the legacy config, while references, nondeterminism, and unevaluable nodes are rejected in both modes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -65,6 +65,9 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable) {
Review Comment:
Follow-up: I revised the patch so RuntimeReplaceable expressions are recursively normalized before validation and evaluation. The default path checks foldability after normalization, while the session legacy flag covers only safe deterministic, row-independent non-foldable expressions. The migration guide and PR body are updated, and tests cover nvl, nested decode, elt, transform, and guarded raise_error.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118947122
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
+ !SQLConf.get.getConf(SQLConf.LEGACY_ALLOW_NON_FOLDABLE_OPTIONS)) {
Review Comment:
Agreed. The legacy switch now relaxes only foldability: before evaluation, the normalized map must have no references, be deterministic, and contain no Unevaluable nodes. Tests cover input columns, lambda variables, outer references, and uuid() in both default and legacy modes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
Review Comment:
Updated in f75b84894ba. I used ReplaceExpressions.replace recursively rather than prepareForEval so nested RuntimeReplaceable expressions such as decode(encode(...)) are fully normalized. Foldability and evaluation both use the normalized expression; nvl and nested decode tests pass by default.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4115894147
##########
AGENTS.md:
##########
@@ -101,7 +101,7 @@ These are combined with a base above rather than used on their own:
## Build and Test
-Build and tests can take a long time. If the user explicitly asked to run tests, run them. Otherwise (you are running tests on your own to verify a change), first ask the user if they have more changes to make.
+Build and tests can take a long time, so prefer the narrowest relevant build or test command.
Review Comment:
not very related here but I hit the interruption during this PR and think it's unnecessary. The AI agent is very smart today and running test has no actual harm so no need to wait here.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118945372
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7557,6 +7557,15 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val LEGACY_ALLOW_NON_FOLDABLE_OPTIONS =
Review Comment:
Done in f75b84894ba. The config now declares ConfigBindingPolicy.SESSION, and SparkConfigBindingPolicySuite passes (3/3).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7557,6 +7557,15 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val LEGACY_ALLOW_NON_FOLDABLE_OPTIONS =
Review Comment:
Added ConfigBindingPolicy.SESSION in f75b84894ba. SparkConfigBindingPolicySuite now passes all 3 tests.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118967245
##########
AGENTS.md:
##########
@@ -101,7 +101,7 @@ These are combined with a base above rather than used on their own:
## Build and Test
-Build and tests can take a long time. If the user explicitly asked to run tests, run them. Otherwise (you are running tests on your own to verify a change), first ask the user if they have more changes to make.
+Build and tests can take a long time, so prefer the narrowest relevant build or test command.
Review Comment:
I explained in https://github.com/apache/spark/pull/59072#discussion_r4115894147 . The practice is to include small AGENTS.md edits in the PR that needed 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] [SPARK-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #59072: [SPARK-59801][SQL] Fix internal error for non-foldable options maps
URL: https://github.com/apache/spark/pull/59072
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118155254
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
Review Comment:
`!m.foldable` is broader than the failure being fixed. `RuntimeReplaceable` (e.g. `nvl`, `ifnull`) doesn't override `foldable`, so `map('delimiter', nvl(NULL, '|'))` is now rejected even though `m.eval()` evaluated it fine in 4.3. Would a check aimed at what `eval()` can't handle at analysis time (attribute, lambda-variable or outer references, or nondeterminism) fix the `INTERNAL_ERROR` without needing the legacy flag for constant options?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7557,6 +7557,15 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val LEGACY_ALLOW_NON_FOLDABLE_OPTIONS =
Review Comment:
This entry needs `.withBindingPolicy(...)`, or `SparkConfigBindingPolicySuite` fails. It's currently failing on this head with "The following configs do not have bindingPolicy field set". The suite's exceptions file is frozen, so the policy has to be declared here. Since this changes analysis behavior per session, `ConfigBindingPolicy.SESSION` seems like the natural choice.
##########
AGENTS.md:
##########
@@ -101,7 +101,7 @@ These are combined with a base above rather than used on their own:
## Build and Test
-Build and tests can take a long time. If the user explicitly asked to run tests, run them. Otherwise (you are running tests on your own to verify a change), first ask the user if they have more changes to make.
+Build and tests can take a long time, so prefer the narrowest relevant build or test command.
Review Comment:
This comes from the separate `[MINOR][INFRA]` commit and isn't related to SPARK-59801. Could it be split into its own PR?
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4112240783
##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -439,6 +439,21 @@ class CsvFunctionsSuite extends SharedSparkSession {
checkAnswer(df1.selectExpr("to_csv(a)"), Row("1") :: Nil)
}
+ test("to_csv with non-foldable options") {
Review Comment:
Since `ExprUtils.convertToMapData` is shared by 9 functions (`from_csv`, `to_csv`, `schema_of_csv`, `from_json`, `to_json`, `schema_of_json`, `from_xml`, `to_xml`, `schema_of_xml`), shall we add test coverage for JSON and XML too? The existing `csv-functions.sql`, `json-functions.sql`, and `xml-functions.sql` already have `INVALID_OPTIONS.NON_MAP_FUNCTION`/`NON_STRING_TYPE` cases and temp views with a string column (e.g. `jsonTable(jsonField, a)`), which we can reuse for `NON_FOLDABLE`.
In addition, it would be great to have a test case for a lambda variable. IIUC, since SPARK-55138 (4.2.0), the following silently uses the string `"null"` as the delimiter (`["1null2"]`) instead of failing, and this PR fixes it. If so, this seems worth backporting to `branch-4.2` too.
```sql
SELECT transform(array('|'), x -> to_csv(named_struct('a', 1, 'b', 2), map('delimiter', x)));
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -65,6 +65,9 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable) {
Review Comment:
`foldable` is stricter than the previous behavior (which only required `m.eval()` to succeed), so this starts to reject some constant option values which work today. IIUC, `Successfully analyzed queries are unchanged` in the PR description is not true for the following cases.
1. `RuntimeReplaceable` expressions which don't extend `UnaryExpression`/`BinaryExpression`/... (e.g. `nvl`, `ifnull`, `nvl2`, `left`, `right`, `btrim`, `split_part`, `decode`, `url_decode`, `version()`) have the default `foldable = false`, while `RuntimeReplaceable.eval` delegates to `replacement.eval()` since SPARK-50377 (Apache Spark 4.0.0). So, the following works in 4.0.0 ~ 4.2.0, but fails with `INVALID_OPTIONS.NON_FOLDABLE` after this PR. Persisted views and SQL functions using it will fail after upgrade too.
```sql
SELECT to_csv(named_struct('a', 1, 'b', 2), map('delimiter', nvl(NULL, '|')));
```
To keep them working, we may need to check `foldable` (and evaluate) after replacing `RuntimeReplaceable` recursively (like `ReplaceExpressions.replace`, because `prepareForEval` is a single pass and leaves `decode(bin, charset)` -> `StringDecode` as-is) and `trimAliases`.
2. Other evaluable but non-foldable expressions are rejected too. I checked that the following queries return `1|2` with Spark 3.4, and the evaluation path is the same until the base of this PR. Zero-argument Scala/Java UDFs are also affected.
```sql
SELECT to_csv(named_struct('a', 1, 'b', 2), map('sep', elt(1, '|', ',')));
SELECT to_csv(named_struct('a', 1, 'b', 2), map('sep', transform(array('|'), x -> x)[0]));
SELECT to_csv(named_struct('a', 1, 'b', 2), map('sep', CASE WHEN length('|') = 1 THEN '|' ELSE raise_error('bad') END));
```
If this stricter behavior is intended, could you document it in the migration guide and update the PR description?
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -4841,6 +4841,11 @@
"Invalid options:"
],
"subClass" : {
+ "NON_FOLDABLE" : {
+ "message" : [
+ "Must use a foldable `map()` function for options."
Review Comment:
nit. We already have `NON_FOLDABLE_ARGUMENT` (SQLSTATE `42K08`) and `QueryCompilationErrors.nonFoldableArgumentError(funcName, paramName, paramType)` for the same situation in other function builders (e.g. `collate`, `ceil`/`floor`, `to_char`, `mode`), while this new sub-condition inherits `42K06` (`Invalid type for options`). If we keep `INVALID_OPTIONS.NON_FOLDABLE`, shall we add a parameter for the offending expression like `NON_STRING_TYPE` does? Currently, the message doesn't show which option value is non-foldable.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59072:
URL: https://github.com/apache/spark/pull/59072#issuecomment-5885767428
thanks for the review, merging to master/4.x
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118950433
##########
sql/core/src/test/scala/org/apache/spark/sql/CsvFunctionsSuite.scala:
##########
@@ -439,6 +439,66 @@ class CsvFunctionsSuite extends SharedSparkSession {
checkAnswer(df1.selectExpr("to_csv(a)"), Row("1") :: Nil)
}
+ test("to_csv with non-foldable options") {
Review Comment:
Done in f75b84894ba. The test names now include SPARK-59801, and the CSV, JSON, and XML suites cover all nine call sites. The three suites pass all 216 tests.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -59,12 +60,17 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
dataType.asInstanceOf[StructType]
}
- def convertToMapData(exp: Expression): Map[String, String] = exp match {
+ def convertToMapData(exp: Expression, functionName: String): Map[String, String] = exp match {
case m: CreateMap
if AbstractMapType(
StringTypeWithCollation(supportsTrimCollation = true),
StringTypeWithCollation(supportsTrimCollation = true))
.acceptsType(m.dataType) =>
+ if (!m.foldable &&
+ !SQLConf.get.getConf(SQLConf.LEGACY_ALLOW_NON_FOLDABLE_OPTIONS)) {
+ throw QueryCompilationErrors.nonFoldableArgumentError(
+ functionName, "options", m.dataType)
Review Comment:
Thanks. I kept the existing NON_FOLDABLE_ARGUMENT condition as suggested, and updated the PR description and migration guide to use it consistently.
--
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-59801][SQL] Fix internal error for non-foldable options maps [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59072:
URL: https://github.com/apache/spark/pull/59072#discussion_r4118783218
##########
AGENTS.md:
##########
@@ -101,7 +101,7 @@ These are combined with a base above rather than used on their own:
## Build and Test
-Build and tests can take a long time. If the user explicitly asked to run tests, run them. Otherwise (you are running tests on your own to verify a change), first ask the user if they have more changes to make.
+Build and tests can take a long time, so prefer the narrowest relevant build or test command.
Review Comment:
I explained in https://github.com/apache/spark/pull/59072#discussion_r4115894147 . The practice is to include small AGENTS.md edits in the PR that needed 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