Repository navigation
lower_enabled.sql and upper_enabled.sql set a no-op config and never test the native path #5611
Description
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:
-- Test lower() with the standard allowIncompatible opt-in (happy path)
-- Config: spark.comet.expression.Lower.allowIncompatible=trueBut CometCaseConversionBase (spark/src/main/scala/org/apache/comet/serde/strings.scala) never reports Incompatible, so allowIncompatible is never consulted:
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:
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:
- Change the
Configdirective to the switch that actually selects the native path:-- Config: spark.comet.caseConversion.enabled=true - Update the header comment, which currently describes the
allowIncompatibleopt-in. - Add
expect_native(lower)/expect_native(upper)to the queries so the fixture fails if the native path is lost again. The plainlower.sql/upper.sqlare already annotatedexpect_dispatch(...)by Test helpers to assert whether an expression ran natively or through the codegen dispatcher #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.
Activity
cestercian commentedon Sep 1, 2026
I'll take this — updating
lower_enabled.sql/upper_enabled.sqlto usespark.comet.caseConversion.enabled=trueand fixing the header comments. Leavingexpect_nativeannotations for after #5610 lands.test: enable native path in lower/upper_enabled sql fixtures (apache#…