stantheman0128 opened a new issue, #6232:
URL: https://github.com/apache/datafusion-comet/issues/6232
### What is the problem the feature request solves?
`CometLiteral.getSupportLevel` checks the literal's type through
`supportedDataType` with the default `allowAnyStringType = true`, so its
`StringType` case accepts every collation (`QueryPlanSerde.scala:615,
624-625`), and `convert` writes the value with `setStringVal`
(`literals.scala:98-99`). So a `Literal` of type `StringType(UTF8_LCASE)` is
accepted, and its collation is not carried into the native plan. Nothing
records that it was dropped.
A cast of a literal is one way to get such a literal.
`CometCast.getSupportLevel` returns `Compatible()` for a cast whose child is a
`Literal`, unless a variant type is involved, without calling `isSupported`
(`CometCast.scala:90-99`), so the collation guard proposed in #5302 would not
see it either. `convert` then folds the cast with `cast.eval()` into a new
literal of the target type (`:111-112`). `CAST('abc' AS STRING COLLATE
UTF8_LCASE)` therefore turns into a collated literal that `CometLiteral`
accepts. `ConstantFolding` normally removes a cast like that before Comet sees
it, but `CometSqlFileTestSuite` excludes `ConstantFolding` for every fixture
(`CometSqlFileTestSuite.scala:92`), so Comet's own harness can reach this path.
The answer is right today, since folding produces the same bytes Spark
would. The problem is the one #4489 described for `CometCast`: the behavior is
implicit and untested, so the next change to either serde can alter it without
anyone noticing. I have not built a query that returns a wrong result through
this path.
### Describe the potential solution
Two options, and I don't have a strong preference:
1. `CometLiteral.getSupportLevel` returns `Unsupported` when the literal's
type has a non-default collation. Passing `allowAnyStringType = false` to
`supportedDataType` would do it for a plain string literal, the way
`CometLocalTableScanExec` already does (`CometLocalTableScanExec.scala:146`).
`CometLiteral` also mixes in `CometTypeShim`, so `hasNonDefaultStringCollation`
is in scope for nested types. The cost is that these literals, and the
expressions around them, fall back to Spark.
2. Keep accepting them, since the bytes match, and say so in a comment in
`CometLiteral`.
Either way, a test under `CometSqlFileTestSuite` with `CAST('abc' AS STRING
COLLATE UTF8_LCASE)` would pin the choice, since that harness is the one that
can reach this path.
### Additional context
Found while working on #5302 (#4489). Discussion:
https://github.com/apache/datafusion-comet/pull/5302#pullrequestreview-5294205378
Line numbers are from `main` at `646ff181`.
--
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]
