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: [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]
