You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "pratham76 (via GitHub)" <gi...@apache.org> on 2026/08/15 17:18:26 UTC
[PR] [SPARK-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
pratham76 opened a new pull request, #58023:
URL: https://github.com/apache/spark/pull/58023
<!--
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.
-->
This PR fixes a bug where null literals were incorrectly serialized as the string 'null' instead of the SQL keyword NULL when pushed down to external databases via DSv2 (JDBC aggregate/predicate pushdown).
### 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.
-->
When a null literal appears in a pushed-down DSv2 expression, it was being serialized incorrectly:
`CASE WHEN S = '' THEN 'null' ELSE S END (string literal)`
This caused wrong results because the external database executed a different predicate/grouping key than intended. For example, with NULLIF(s, '') (which rewrites to If(s='', null, s)), the empty string row was bucketed under the string "null" instead of collapsing into the real NULL group, producing 3 result rows instead of 2.
### 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'.
-->
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.
-->
Added a new test case in `JDBCV2Suite`
### 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.
-->
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho closed pull request #58023: [SPARK-58782][SQL] Fix DSv2 pushdown null literal serialization bug
URL: https://github.com/apache/spark/pull/58023
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58023:
URL: https://github.com/apache/spark/pull/58023#issuecomment-5306744676
Thank you @pratham76! Adding @viirya who is a bit more familiar with DSv2 pushdown infrastructure
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #58023:
URL: https://github.com/apache/spark/pull/58023#issuecomment-5336167428
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/01b0663a711f9ca30f79b669841132bf83e1942d
- merged into branch-4.x https://github.com/apache/spark/commit/fd3c039fa5a288daa3fd6fc8945d048b31498a68
- merged into branch-4.3 https://github.com/apache/spark/commit/1308d4c3b887bc1b897c90b0a75e978a8d68aac4
*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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "pratham76 (via GitHub)" <gi...@apache.org>.
pratham76 commented on PR #58023:
URL: https://github.com/apache/spark/pull/58023#issuecomment-5325070087
> LGTM on the fix itself, pending two things below.
>
> **This is a user-facing change.** The PR answers "No", but changing results is the entire point: before the fix, a pushed-down string null literal produced a spurious `'null'` group against the external database, which is exactly what the new `JDBCV2Suite` test asserts. A wrong-results fix should be answered "Yes", with the before/after you already wrote in "Why are the changes needed?" — otherwise it won't appear in the release notes, and this is precisely the kind of fix people scan for when deciding whether to pick up a maintenance release.
>
> The fix is also bigger than the description suggests. The old `BinaryType` branch ran `assert(value.isInstanceOf[Array[Byte]])`, which fails outright on a null, so rendering the explain string for a pushed expression containing a null binary literal threw an `AssertionError` rather than merely producing bad SQL. Worth calling out; good that the new `LiteralValueSuite` pins that case.
>
> On the tests: `test("show tables")` asserts the full `h2.test` listing with `checkAnswer`, which is an exact match, so the new table breaks it. The SQL shards were still running when I looked, which is why CI hasn't flagged it yet. Details inline.
>
> Before merge:
>
> * Add `Row("test", "null_literal", false)` to `test("show tables")`.
> * Change the user-facing-change answer to "Yes" and describe the result change.
> * Optional: mention the null-binary `AssertionError` fix in the description.
>
> Separately, I checked Apache Iceberg, since `LiteralValue.toString` is reachable from any `SupportsPushDownV2Filters` implementation. It is unaffected: `SparkV2Filters` consumes predicates structurally and never reads the rendered string, and Iceberg's scan descriptions come from its own `Spark3Util.describe` rather than Spark's. The new `NULL` surfaces only in a couple of `Preconditions` messages there, which Iceberg's tests don't assert on.
Thanks for the comments @szehon-ho !! have incorporated the changes and have update the PR description to align with the fix. PTAL if the PR could be checked in now.
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #58023:
URL: https://github.com/apache/spark/pull/58023#discussion_r3797950256
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -3174,4 +3180,16 @@ class JDBCV2Suite extends SharedSparkSession with ExplainSuiteHelper {
assertResult(expectedMetadata) { jdbcRdd.getDatabaseMetadata }
}
+ test("SPARK-58782: null literal in pushed-down aggregate should render as NULL not 'null'") {
Review Comment:
Nit: missing blank line between this and the preceding test.
Also optional: this covers only the aggregate/group-by path, while the title mentions predicates too — a `WHERE NULLIF(s, '') IS NULL` variant would cover that. `LiteralValueSuite` already pins the contract, so I don't feel strongly.
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -236,6 +236,12 @@ class JDBCV2Suite extends SharedSparkSession with ExplainSuiteHelper {
batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES ('a a a')")
batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES (null)")
+ batchStmt.addBatch(
+ "CREATE TABLE \"test\".\"null_literal\" (s TEXT(32))")
Review Comment:
This new table breaks `test("show tables")` around line 1823: it asserts the full listing of `h2.test` via `checkAnswer`, which requires exact equality, and the expected list currently ends at `strings_with_nulls` — the last table added to this suite, whose author had to extend the list for the same reason.
Two ways out:
- Add `Row("test", "null_literal", false)` to that list. My preference, since the dedicated table reads well here.
- Or drop the table and use `NULLIF(str, 'abc')` over the existing `strings_with_nulls`, which exercises the same path with no new fixture.
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "pratham76 (via GitHub)" <gi...@apache.org>.
pratham76 commented on PR #58023:
URL: https://github.com/apache/spark/pull/58023#issuecomment-5311013926
> One optional nice-to-have, non-blocking: the root cause is a LiteralValue.toString rendering contract, so a small catalyst-level unit assertion (LiteralValue(null, StringType).toString == "NULL") would pin it more directly and faster than the H2 end-to-end test. The JDBCV2Suite test is good as the user-visible regression check; the unit test would just nail the contract.
Thanks for the comments @viirya, have added a catalyst level unit test in `LiteralValueSuite` that directly tests the `LiteralValue.toString` contract:
* Verifies null literals of all types (String, Integer, Boolean, Binary) render as "NULL"
* Verifies non-null literals render correctly (quoted strings, unquoted numbers, hex binary)
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "pratham76 (via GitHub)" <gi...@apache.org>.
pratham76 commented on code in PR #58023:
URL: https://github.com/apache/spark/pull/58023#discussion_r3801773269
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -236,6 +236,12 @@ class JDBCV2Suite extends SharedSparkSession with ExplainSuiteHelper {
batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES ('a a a')")
batchStmt.addBatch("INSERT INTO \"test\".\"strings_with_nulls\" VALUES (null)")
+ batchStmt.addBatch(
+ "CREATE TABLE \"test\".\"null_literal\" (s TEXT(32))")
Review Comment:
Thanks for pointing this out, I went with the first approach!
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #58023:
URL: https://github.com/apache/spark/pull/58023#issuecomment-5318825248
also FYI @tdcmeehan who i think was also looking at this issue
--
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-58782][SQL] Fix DSv2 pushdown null literal serialization bug [spark]
Posted by "pratham76 (via GitHub)" <gi...@apache.org>.
pratham76 commented on code in PR #58023:
URL: https://github.com/apache/spark/pull/58023#discussion_r3801778467
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala:
##########
@@ -3174,4 +3180,16 @@ class JDBCV2Suite extends SharedSparkSession with ExplainSuiteHelper {
assertResult(expectedMetadata) { jdbcRdd.getDatabaseMetadata }
}
+ test("SPARK-58782: null literal in pushed-down aggregate should render as NULL not 'null'") {
Review Comment:
Incorporated!, have not test here as `LiteralValueSuite` already covers the case
--
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