Skip to content

fix: gate case conversion on standard per-expression allowIncompatible#4853

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:fix-4467-case-conversion-support-level
Open

fix: gate case conversion on standard per-expression allowIncompatible#4853
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:fix-4467-case-conversion-support-level

Conversation

@andygrove

Copy link
Copy Markdown
Member

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.

CometCaseConversionBase (parent of upper/lower) 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 meant users who set
spark.comet.expression.Upper.allowIncompatible=true saw no effect.

Switch upper/lower to isExprAllowIncompat/getExprAllowIncompatConfigKey
and remove the now-redundant COMET_CASE_CONVERSION_ENABLED config, so
both paths report support level and gate opt-in the same way. Regenerate
the affected COMET-INFO plan hints in the tpcds plan-stability goldens.

Closes apache#4467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CometCaseConversionBase gates compat inside convert() instead of getSupportLevel

1 participant