You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/01 14:28:29 UTC
[I] lower_enabled.sql and upper_enabled.sql set a no-op config and never test the native path [datafusion-comet]
andygrove opened a new issue, #5611:
URL: https://github.com/apache/datafusion-comet/issues/5611
### Describe the bug
`lower_enabled.sql` and `upper_enabled.sql` exist to cover the native case-conversion path. They do not: the config they set is a no-op, so both fixtures currently exercise the codegen dispatcher, exactly like `lower.sql` and `upper.sql` next to them.
Each opens with:
```sql
-- Test lower() with the standard allowIncompatible opt-in (happy path)
-- Config: spark.comet.expression.Lower.allowIncompatible=true
```
But `CometCaseConversionBase` (`spark/src/main/scala/org/apache/comet/serde/strings.scala`) never reports `Incompatible`, so `allowIncompatible` is never consulted:
```scala
override def getSupportLevel(expr: T): SupportLevel =
if (!CometConf.COMET_CASE_CONVERSION_ENABLED.get()) {
Compatible(nativeOptIn = Some(NativeOptIn(CometConf.COMET_CASE_CONVERSION_ENABLED.key)))
} else {
Compatible()
}
override def convert(expr: T, inputs: Seq[Attribute], binding: Boolean): Option[Expr] = {
if (CometConf.COMET_CASE_CONVERSION_ENABLED.get()) {
super.convert(expr, inputs, binding)
} else {
CometScalaUDF.emitJvmCodegenDispatch(expr, inputs, binding)
}
}
```
The switch is `spark.comet.caseConversion.enabled` (default `false`), not `spark.comet.expression.Lower.allowIncompatible`. The fixtures look like leftovers from before case conversion moved from the `Incompatible` + `allowIncompatible` shape to `Compatible` + `NativeOptIn`.
Because both mechanisms return identical results for the ASCII inputs these fixtures use, nothing failed and the drift went unnoticed.
### Steps to reproduce
With the `expect_native` mode from #5609, temporarily annotate the query in `lower_enabled.sql`:
```sql
query expect_native(lower)
SELECT lower(s) FROM test_lower_enabled
```
```
./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite lower_enabled" -Dtest=none
```
```
Expected `lower` to run as a native expression but it ran through the JVM codegen dispatcher.
Actual: native=[] codegen-dispatched=[lower]
```
Verified against `main` on Spark 4.1.
### Expected behavior
The two `_enabled` fixtures cover the native scalar function, so that path has test coverage distinct from the dispatcher path the plain fixtures already cover.
### Suggested fix
In both `spark/src/test/resources/sql-tests/expressions/string/lower_enabled.sql` and `upper_enabled.sql`:
1. Change the `Config` directive to the switch that actually selects the native path:
```sql
-- Config: spark.comet.caseConversion.enabled=true
```
2. Update the header comment, which currently describes the `allowIncompatible` opt-in.
3. Add `expect_native(lower)` / `expect_native(upper)` to the queries so the fixture fails if the native path is lost again. The plain `lower.sql` / `upper.sql` are already annotated `expect_dispatch(...)` by #5609, so the pair then pins both mechanisms.
Keep the inputs ASCII. The native scalar function deliberately does not match Spark for locale-specific characters (Turkish dotted/dotless I, German sharp s), which is why it is opt-in, and those cases belong in the dispatcher fixtures where they already live.
This depends on #5609 for step 3; steps 1 and 2 stand on their own.
### Additional context
Found while adding the mechanism-assertion test helpers in #5610. Note that `spark.comet.caseConversion.enabled` is the current opt-in only until #4467 / #4853 land.
--
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] lower_enabled.sql and upper_enabled.sql set a no-op config and never test the native path [datafusion-comet]
Posted by "cestercian (via GitHub)" <gi...@apache.org>.
cestercian commented on issue #5611:
URL: https://github.com/apache/datafusion-comet/issues/5611#issuecomment-5497778293
I'll take this — updating `lower_enabled.sql` / `upper_enabled.sql` to use `spark.comet.caseConversion.enabled=true` and fixing the header comments. Leaving `expect_native` annotations for after #5610 lands.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] lower_enabled.sql and upper_enabled.sql set a no-op config and never test the native path [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead closed issue #5611: lower_enabled.sql and upper_enabled.sql set a no-op config and never test the native path
URL: https://github.com/apache/datafusion-comet/issues/5611
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org