apache / apache/datafusion-comet

CometCaseConversionBase gates compat inside convert() instead of getSupportLevel

Open
#4,467 1 comment 0 reactions 0 assignees View on GitHub
area:expressions priority:medium
Dominant language
Scala
Stars
1.3k
Forks
375
Avg merge
2d 4h
Merged PRs (30d)
198

Description

## Describe the bug

`CometCaseConversionBase` (parent of `CometUpper` and `CometLower`) gates compatibility inside `convert(...)`:

```scala
override def convert(expr: T, inputs: Seq[Attribute], binding: Boolean): Option[Expr] = {
if (!CometConf.COMET_CASE_CONVERSION_ENABLED.get()) {
withInfo(expr, "Comet is not compatible with Spark for case conversion in " +
s"locale-specific cases. Set ${CometConf.COMET_CASE_CONVERSION_ENABLED.key}=true " +
"to enable it anyway.")
return None
}
super.convert(expr, inputs, binding)
}
```

It never overrides `getSupportLevel`, so the class-level support level defaults to `Compatible(None)` even though `getIncompatibleReasons()` is populated. The Step 5 consistency rule in the `audit-comet-expression` skill calls this out specifically as the canonical antipattern to avoid. Surfaced by the string-expressions audit in apache/datafusion-comet#4461.

Concrete consequences:

1. The dispatcher's `allowIncompatible` machinery is bypassed. A user who flips the standard `spark.comet.expr.allowIncompatible=true` (or the per-expression `spark.comet.expression.Upper.allowIncompatible`) still has `lower`/`upper` fall back, because the gate is the older bespoke `spark.comet.caseConversion.enabled`.
2. EXPLAIN and the auto-generated compatibility docs disagree. EXPLAIN shows the bespoke message above; the compat doc lists the populated `getIncompatibleReasons()` text instead.
3. `initcap` uses the standard `Incompatible(Some(...))` flow; `lower`/`upper` use the bespoke flow. The asymmetry is confusing for users who have just opted into all string incompatibilities.

## Expected behavior

Override `getSupportLevel` to return `Incompatible(Some(reason))` from a shared `private val`, drop the in-`convert` conf check, and either deprecate `spark.comet.caseConversion.enabled` in favor of the standard per-expression `allowIncompatible` flag or have the support level read the standard flag.

## Additional context

- Serde: `CometCaseConversionBase` in `spark/src/main/scala/org/apache/comet/serde/strings.scala`
- Conf: `CometConf.COMET_CASE_CONVERSION_ENABLED` (default `false`)
- Related: #1052 (initcap divergence, already follows the standard pattern)

Contributor guide

Open the contributing guide

Research direction

Start with spark/src/main/scala/org/apache/comet/serde/strings.scala and inspect CometCaseConversionBase alongside the initcap pattern described in #1052. Trace how getSupportLevel, getIncompatibleReasons(), and the standard allowIncompatible settings are consumed. Done means lower and upper use the standard incompatibility flow consistently in dispatch, EXPLAIN, and generated compatibility documentation.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala, spark
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.