You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/10/01 14:07:22 UTC
[PR] [SPARK-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv… [spark]
srielau opened a new pull request, #59188:
URL: https://github.com/apache/spark/pull/59188
…, and from_xml
These functions take STRING documents. CHAR padding is part of the value after CAST to STRING, so it should not be stripped.
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'common/utils/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #59188:
URL: https://github.com/apache/spark/pull/59188#issuecomment-6042486338
@dtenedor Thanks for the approval and for catching the comma-delimiter case.
Pinned the default-comma behavior you listed: `schema_of_csv(CAST('1' AS CHAR(3)))` is `STRUCT<_c0: DOUBLE>`, `from_csv` on padded `'1,2'` / `'1,ab'` keeps the last-field pad (INT becomes null, STRING keeps `"ab "`), and `ignoreTrailingWhiteSpace` restores `{1, 2}`. Same checks are in `CsvExpressionsSuite` for interpreted and generated eval. Also added a parquet `CHAR(5)` column so the table-read path is covered, not only a folded CAST literal.
Updated the description for the comma case, `ignoreTrailingWhiteSpace`, and the `readSideCharPadding` / standard-semantics disagreement on CHAR columns.
On the smaller notes: past `maxColumns` a space-delimited wide CHAR fails the same way as a STRING of that width, so a follow-up sentence is enough. The XmlToStructs codegen/interpreted mix-up is not in the commit messages.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4157794530
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2607,37 +2605,12 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
checkAnswer(
sql("""SELECT length(CAST('{"a":1}' AS CHAR(20)))"""),
Row(20))
- checkAnswer(
- sql("""SELECT schema_of_json(CAST('{"a":1}' AS CHAR(20)))"""),
Review Comment:
**Nit (P3):** The `schema_of_json` / `schema_of_xml` CHAR checks removed here (and the `COLLATE SR_AI` trio below) don't depend on trimming. JSON and XML ignore trailing whitespace, so `schema_of_json(CAST('{"a":1}' AS CHAR(20)))` should still give `STRUCT<a: BIGINT>`. They are also the only remaining coverage for the CHAR acceptance SPARK-59274 added to `SchemaOfJson` and `SchemaOfXml`: each has its own `!child.dataType.isInstanceOf[StringType]` check (`jsonExpressions.scala:2107`, `xmlExpressions.scala:212`), separate from `SchemaOfCsv`'s. With them gone, the surviving `schema_of_csv` CHAR tests wouldn't catch a regression of the JSON/XML checks back to `!= StringType`. Could you keep the JSON/XML (and collated) cases? Update the collated `schema_of_csv` one to the new untrimmed `STRUCT<_c0: INT, _c1: STRING, _c2: STRING>` rather than dropping it. The `from_xml` CHAR(30) case would also be cheap to keep.
**Verification:**
- **Regression:** Reverting SchemaOfJson or SchemaOfXml input checking to exact StringType equality makes a restored CHAR-document assertion fail.
- **Behavior:** The restored and updated CHAR-document assertions pass under standard semantics with the PR's no-trim 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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4157269142
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,23 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
- Row(Row(1)))
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
+ sql("""SELECT from_json('{"a":1} ', 'a INT')"""))
+ // With a space delimiter, CHAR padding shows up as empty CSV fields.
checkAnswer(
- checkTextInput(
- """SELECT from_csv(
+ sql(
+ """SELECT schema_of_csv(
Review Comment:
Restored the `from_csv(CAST('1' AS CHAR(3)), '_c0 INT', map('delimiter', ' ', 'mode', 'FAILFAST'))` case and asserted `MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION`. PERMISSIVE still returns the first field (`named_struct('_c0', 1)`).
Also added an expression-level `CsvToStructs` check with a `CharType` literal via `checkErrorInExpression`, so interpreted and codegen evaluation both hit the untrimmed `child.genCode` path.
The PR description now calls out the FAILFAST `from_csv` behavior as well.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor closed pull request #59188: [SPARK-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml
URL: https://github.com/apache/spark/pull/59188
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #59188:
URL: https://github.com/apache/spark/pull/59188#issuecomment-6045410523
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6fbec03caab5776014b3bfc23e8dfb8dcaab1dd7
- merged into branch-4.x https://github.com/apache/spark/commit/c25f857a984b4abe37d2d4e4ffac61c7f2663c8c
*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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4174121725
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2607,37 +2605,12 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
checkAnswer(
sql("""SELECT length(CAST('{"a":1}' AS CHAR(20)))"""),
Row(20))
- checkAnswer(
- sql("""SELECT schema_of_json(CAST('{"a":1}' AS CHAR(20)))"""),
Review Comment:
Restored the CHAR `schema_of_json` / `schema_of_xml` checks and the `COLLATE SR_AI` trio. The collated `schema_of_csv` case now expects `STRUCT<_c0: INT, _c1: STRING, _c2: STRING>` because the pad is no longer rtrimmed. Also put back `from_xml(CAST(... AS CHAR(30)), 'a INT')`, which still parses since XML ignores trailing whitespace.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4186108571
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,47 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
Review Comment:
Agreed. The valid JSON comparison still holds with or without rtrim. Added a PERMISSIVE corrupt-record case:
```sql
from_json(CAST('{"a":' AS CHAR(12)), 'a INT, _unparsed STRING', map('columnNameOfCorruptRecord', '_unparsed'))
```
`_unparsed` is `{"a": ` (declared width 12). Restoring rtrim would drop those trailing spaces.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,47 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
+ sql("""SELECT from_json('{"a":1} ', 'a INT')"""))
+ // XML ignores trailing whitespace, so a padded CHAR document still parses.
+ checkAnswer(
+ sql(
+ """SELECT from_xml(
Review Comment:
Same issue for XML. Kept the well-formed CHAR(30) parse and added a malformed padded document:
```sql
from_xml(CAST('<ROW>' AS CHAR(10)), 'a INT, _unparsed STRING', map('columnNameOfCorruptRecord', '_unparsed'))
```
`_unparsed` is `<ROW> `. Restoring rtrim would drop the pad.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #59188:
URL: https://github.com/apache/spark/pull/59188#issuecomment-5998272101
On the wide-CHAR CSV tokenization cost: this PR treats CHAR documents as STRING after CAST, so a space delimiter on `CHAR(n)` can expand into up to `n` tokens (bounded by `maxColumns`, default 20480). That is an accepted consequence of the no-rtrim contract rather than something to special-case in the parser here. A wide-input CSV benchmark or `maxColumns` mitigation can be a follow-up if this shows up in practice.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #59188:
URL: https://github.com/apache/spark/pull/59188#issuecomment-6045394888
LGTM, 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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4185992489
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,47 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
+ sql("""SELECT from_json('{"a":1} ', 'a INT')"""))
+ // XML ignores trailing whitespace, so a padded CHAR document still parses.
+ checkAnswer(
+ sql(
+ """SELECT from_xml(
Review Comment:
**Nit (P3):** This well-formed XML case has the same result with or without the removed CHAR trim because XML ignores trailing whitespace. As written, it stays green if from_xml starts stripping the padded value again, so the new no-rtrim behavior has no regression-sensitive XML assertion.
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,47 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
Review Comment:
**Nit (P3):** This assertion does not distinguish the new no-rtrim behavior: JSON accepts trailing whitespace, so restoring CHAR right trimming would still make both queries return the same Row(1). The complete padded-value behavior for from_json can therefore regress without any focused test failure.
See **Shared repair plan 1** in the review body.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4210344356
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2635,44 +2635,89 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
Review Comment:
**Nit (P3):** This absolute statement conflicts with the `ignoreTrailingWhiteSpace=true` case below, where CSV does consume the trailing padding. It can mislead maintainers into thinking parser-level whitespace options no longer apply; the guarantee here is specifically that the document expression no longer unconditionally pre-trims CHAR input before parsing.
**Verification:**
- **Inspection:** Confirm that the explanatory wording is consistent with both default preservation of padded CHAR document input and format-specific trimming enabled by parser options.
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4176028573
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2607,37 +2605,12 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
checkAnswer(
sql("""SELECT length(CAST('{"a":1}' AS CHAR(20)))"""),
Row(20))
- checkAnswer(
- sql("""SELECT schema_of_json(CAST('{"a":1}' AS CHAR(20)))"""),
Review Comment:
Confirmed -- the restored schema_of_json / schema_of_xml CHAR checks and the COLLATE SR_AI trio (with the updated schema_of_csv result) plus the from_xml CHAR(30) case are present in CharVarcharTestSuite and cover the SchemaOfJson/SchemaOfXml CHAR acceptance. This resolves the thread. Thanks.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4157794530","thread_id":"inline:4157794530","verdict_sha256":"fe00ecae72a7b9daabf1e92fa88e0abeb3537a72c995ee09b273ed55b53f800c"} -->
--
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-59917][SQL] Do not rtrim CHAR arguments to from_json, from_csv, and from_xml [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59188:
URL: https://github.com/apache/spark/pull/59188#discussion_r4156774593
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -2444,42 +2444,23 @@ class BasicCharVarcharTestSuite extends SharedSparkSession {
test("SPARK-59274: from_json/csv/xml honor CHAR/VARCHAR under standardSemantics") {
withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
- def checkTextInput(query: String, inputType: DataType): DataFrame = {
- val df = sql(query)
- val inputTypes = df.queryExecution.analyzed.expressions.flatMap(_.collect {
- case e: SupportTrimmedCharInput => e.child.dataType
- })
- assert(inputTypes === Seq(inputType), df.queryExecution.analyzed)
- df
- }
-
+ // CHAR padding is part of the STRING value after CAST. Parsers do not rtrim it.
+ // from_json on padded CHAR must match the same document as STRING with trailing spaces.
checkAnswer(
- checkTextInput(
- """SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')""",
- CharType(12)),
- Row(Row(1)))
+ sql("""SELECT from_json(CAST('{"a":1}' AS CHAR(12)), 'a INT')"""),
+ sql("""SELECT from_json('{"a":1} ', 'a INT')"""))
+ // With a space delimiter, CHAR padding shows up as empty CSV fields.
checkAnswer(
- checkTextInput(
- """SELECT from_csv(
+ sql(
+ """SELECT schema_of_csv(
Review Comment:
**Nit (P3):** This line used to be the `from_csv(CAST('1' AS CHAR(3)), '_c0 INT', map('delimiter', ' ', 'mode', 'FAILFAST'))` check. That query is the one whose behavior changes most visibly in this PR: the padded `'1 '` now tokenizes into 3 fields against a 1-field schema, and `UnivocityParser.convert` treats a token-count mismatch as malformed, so under FAILFAST it should now raise the malformed CSV record error instead of returning `Row(Row(1))`. With this case and the `CsvToStructs` CHAR case in `CsvExpressionsSuite` both removed, nothing exercises `from_csv` with a padded CHAR document. `schema_of_csv` goes through `SchemaOfCsv`, and the `from_json` check above passes with or without trimming. Could you keep the `from_csv` case and assert the new outcome, e.g. `intercept` the malformed-record error, or check the PERMISSIVE result? An expression-level `CsvToStructs` check with a `CharType` literal would also cover the codegen path that `doGenCode` now takes via `child.genCode
`.
**Verification:**
- **Regression:** Under spark.sql.charVarchar.standardSemantics.enabled, from_csv(CAST('1' AS CHAR(3)), '_c0 INT', map('delimiter', ' ', 'mode', 'FAILFAST')) is asserted to raise the malformed CSV record error (or an equivalent untrimmed-input outcome is asserted), in both interpreted and codegen evaluation where an expression-level check is 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