ilicmarkodb commented on PR #55219:
URL: https://github.com/apache/spark/pull/55219#issuecomment-4343501333
> Re-review: 2 addressed, 3 remaining, 0 new.
>
> **Addressed**
>
> * Inline comment on `ParameterHandler.scala:135` (NULL "bypass" framing) —
resolved by the new comment at lines 133-134.
> * Inline comment on `CollationSuite.scala:3054` (`Row("null")` and
`IndeterminateCollation`) — resolved by the new comment at lines 3045-3046.
>
> **Remaining** (from prior review, not yet addressed in the PR description)
>
> 1. PR description still says "matching the behavior of string literals" —
string literals are classified as Default strength
(`collationStrengthBaseCases.case lit: Literal => Default`). The new behavior
actually matches `NamedExpression | SubqueryExpression | VariableReference`.
Suggested rephrasing: "matching the behavior of columns and session variables"
(or just "now have implicit collation strength").
> 2. PR description still says "Null parameters bypass the implicit wrapping
since null values have NullType children in CAST..." — for typed nulls the null
branch still produces a CAST; it just delegates to the one `Literal.sql`
already emits. Not really a bypass, just a delegation that happens to produce
the desired form.
> 3. Design consistency question still unanswered: parameters in `EXECUTE
IMMEDIATE` with explicit `COLLATE` now get Implicit strength, while parameters
passed via `spark.sql("...", Map(...))` (plan-level binding in
`NameParameterizedQuery`) still get Default. The PR's own tests (`parameterized
query vs column collation` vs. `execute immediate parameter implicit vs column
collation`) illustrate the divergence. A line in the PR description
acknowledging whether this is intentional would help future readers.
>
> Once the PR description is updated, this looks ready to merge.
done!
--
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]
