MaxGekk commented on PR #56892:
URL: https://github.com/apache/spark/pull/56892#issuecomment-4845405547
I traced the change end-to-end and the core logic looks correct and
consistent with the engine's canonical `eq(StringType)` distinction (the same
one already used by `DataTypeProtoConverter`, `ApplyDefaultCollation`, and
`DataTypeUtils.isDefaultStringCharOrVarcharType`). Specifically:
`StringType("UTF8_BINARY")` returns a distinct instance (not the case object),
the default singleton overrides `typeName` to `"string"` while non-singleton
instances use `"string collate <name>"`, so for non-default collations the new
`s" collate ${st.collationName}"` is byte-for-byte identical to the old
`typeName.substring(6)`. Removing the `PlanGenerationTestSuite` shim is also
safe because the Connect serializer already omits the collation for the default
singleton (`if (!s.eq(StringType))`), so the regenerated goldens match the real
wire format.
One thing I think is worth calling out: this same `Literal.sql` change also
affects **auto-generated result-set column names**, which the "Does this PR
introduce any user-facing change?" section doesn't mention (it lists view text
/ `SHOW CREATE TABLE` / error messages). Column names are built via
`toPrettySQL -> usePrettyExpression(e).sql`, and `usePrettyExpression` doesn't
special-case `Literal`, so a string literal coerced to an explicit
`UTF8_BINARY` now contributes a `collate UTF8_BINARY` clause to the name.
This is already observable for non-binary collations in
`collations-aliases.sql.out`, e.g.:
```
struct<concat_ws(' ' collate UTF8_LCASE, utf8_lcase, utf8_lcase):string
collate UTF8_LCASE>
```
the `' '` separator literal is coerced to `UTF8_LCASE` and shows up in the
column name. With this PR, the `UTF8_BINARY` analog changes too:
- `SELECT concat_ws(' ', utf8_binary, utf8_binary) FROM t1` (where
`utf8_binary` is `STRING COLLATE UTF8_BINARY`)
- before: column name `concat_ws(' ', utf8_binary, utf8_binary)`
- after: column name `concat_ws(' ' collate UTF8_BINARY, utf8_binary,
utf8_binary)`
The same applies under an explicit `DEFAULT COLLATION UTF8_BINARY`, where
all literals become explicit `UTF8_BINARY`.
Suggestions:
1. Mention column/alias naming in the user-facing-change note.
2. Add a `collations-aliases`-style case exercising the `utf8_binary` column
(mirroring the existing `utf8_lcase` one) so the new column-name form is
pinned. The current alias suite only covers `UTF8_LCASE`.
FWIW I also checked the other goldens that contain `collate UTF8_BINARY` but
weren't in the listed run set (`charvarchar`, `identifier-clause`,
`thetasketch`/`tuplesketch`/`hll`) - those are all type/DDL renderings
(`dataType.sql`) or `UTF8_BINARY_RTRIM`-on-a-column, none of them
explicit-`UTF8_BINARY` string-literal renderings, so they're unaffected by this
change.
--
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]
