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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
