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: [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]
