andygrove opened a new pull request, #4853:
URL: https://github.com/apache/datafusion-comet/pull/4853
## Which issue does this PR close?
Closes #4467.
## Rationale for this change
`CometCaseConversionBase` (the parent of `CometUpper` and `CometLower`)
gated its native opt-in on the bespoke `spark.comet.caseConversion.enabled`
config, while its sibling `CometInitCap` already uses the standard
per-expression `allowIncompatible` flow. This asymmetry was confusing: a user
who set the standard `spark.comet.expression.Upper.allowIncompatible=true` (or
`Lower`) saw no effect, because upper/lower were reading a different, bespoke
flag.
Since #4721, the default path for upper/lower is already Spark-compatible
via the JVM codegen dispatcher, with the native scalar function offered as a
faster opt-in. The only remaining inconsistency was which config key drives
that opt-in. This PR aligns upper/lower with initcap so both report support
level and gate the opt-in through the same standard per-expression mechanism.
## What changes are included in this PR?
- `CometCaseConversionBase` now gates on
`CometConf.isExprAllowIncompat(getExprConfigName(expr))` and surfaces the
opt-in via `getExprAllowIncompatConfigKey(...)`, matching `CometInitCap`. The
`nativeOptInConfigKeyOverride` override is dropped, so upper/lower use
`spark.comet.expression.Upper.allowIncompatible` and
`spark.comet.expression.Lower.allowIncompatible`.
- Removed the now-redundant `COMET_CASE_CONVERSION_ENABLED`
(`spark.comet.caseConversion.enabled`) config, whose original premise
(disabling upper/lower by default) no longer holds now that the default path is
Spark-compatible.
- Regenerated the `Upper` `[COMET-INFO]` opt-in hint in the affected tpcds
plan-stability goldens (q24a, q24b, q24) to reference the new config key.
- Updated the predicate-functions expression audit note and the upper opt-in
EXPLAIN test to reference the standard key.
## How are these changes tested?
- Existing `upper`/`lower` SQL file tests (`upper.sql`, `upper_enabled.sql`,
`lower.sql`, `lower_enabled.sql`). The `_enabled` fixtures were already written
against the standard `spark.comet.expression.{Upper,Lower}.allowIncompatible`
key and now actually exercise the native opt-in path.
- Updated `CometExpressionSuite` test asserting the EXPLAIN opt-in hint
references `spark.comet.expression.Upper.allowIncompatible`.
- Plan-stability goldens are validated by the per-version plan-stability CI
jobs.
--
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]
