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