andygrove commented on issue #4573:
URL:
https://github.com/apache/datafusion-comet/issues/4573#issuecomment-4908554132
Did a fresh pass on this against current `main`. Two things: a couple of
stale items, and a correction to the "currently unreachable" rationale that I
verified empirically.
### Stale items
- **`ArrayJoin`** is already guarded (landed in #4713). It returns
`Incompatible` when `hasNonDefaultStringCollation(expr.array.dataType)` at
`arrays.scala:411`. It should move to the "Already guarded" section.
- All the cited line numbers have drifted with churn in `strings.scala` /
`arrays.scala` (e.g. `ConcatWs` is now ~`strings.scala:357`, `InitCap`
~`:133`). Cosmetic, but every reference is off.
- The "partially mitigated by `COMET_CASE_CONVERSION_ENABLED`" note on
`Upper`/`Lower` is misleading: that gate only selects native vs.
JVM-codegen-dispatch, neither path is collation-aware, so it does not mitigate
collation.
The other unguarded items (`Upper`/`Lower`, `InitCap`, `Like`, `RLike`,
`ConcatWs`, `Contains`, `StartsWith`, `EndsWith`, `StringInstr`,
`StringReplace`, and the "likely safe" group) are still genuinely unguarded.
### The "currently unreachable" reason is incomplete
The issue attributes unreachability to the scan-level guard alone. I built a
repro (plain UTF8_BINARY Parquet columns, collation introduced via `COLLATE`
clauses and collated literals) and swept `contains`, `startswith`, `endswith`,
`instr`, `=`, and `upper` with codegen dispatch on and off. Every case matched
Spark exactly, no divergence.
The physical plan shows why:
```
*(1) Project [Contains(cast(_1#2 as string collate UTF8_LCASE), A) ...] <-
Spark
+- CometNativeColumnarToRow
+- CometNativeScan parquet [_1#2]
<- Comet
```
Spark's collation type-coercion always injects a `cast(... as string collate
X)` (or a `collate(...)` node) to reconcile a plain column with an explicit
collation. Comet has no serde for a collated cast or for `Collate`, so that
node cannot convert and the enclosing expression falls back to Spark on its
own. There are effectively three independent safety nets, not one:
1. collated **scan column** -> scan-level guard forces full fallback
2. `col COLLATE x` / `CAST(... COLLATE x)` -> coercion injects a collated
cast Comet cannot convert -> expression falls back
3. collated **literal** -> the other operand still gets a collated cast,
same as (2)
So the per-expression guards listed here are defense-in-depth, not fixes for
an active divergence. Their value is a clean, explicit EXPLAIN fallback reason
plus protection against a future regression: if anyone adds a `Collate` /
collated-cast passthrough, or lets collated `StringType` be treated as plain
UTF8, these guards become the only thing preventing silently wrong
case-sensitive results. This is consistent with `priority:low`.
Suggested edits: move `ArrayJoin` to "Already guarded", refresh the line
numbers, correct the `Upper`/`Lower` note, and reword the rationale from
"unreachable because of the scan guard" to the three-mechanism picture above.
--
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]
