test: cover round expression routing configurations

[rich7420]

Which issue does this PR close?

Part of #4616.

Rationale for this change

The round SQL tests do not pin routing with the codegen dispatcher disabled or distinguish native and dispatched null-scale handling.

What changes are included in this PR?

Add a separate dispatcher-disabled fixture that checks native integral/decimal rounding and float/double fallback reasons. Assert native null-scale handling for decimal and integral inputs, and dispatched handling for float/double inputs.

How are these changes tested?

All three round SQL fixtures pass locally on Spark 4.1 (3 tests, no failures or skips). The tests also catch injected regressions in dispatcher routing and native null-scale handling. Cross-version CI for this revision is pending.

[andygrove]

Thanks for picking this up. The new expect_native query with the dispatcher off is a real regression guard, and the fallback reason assertions are correct. I have two concerns about how the coverage is structured, in the inline comments.

One more thing on the PR description. It says "Enabling allowIncompatible must not change either route." Since round has no Incompatible case, it might be worth rewording that so it doesn't suggest to the next reader that the flag plays a role in round routing.

[andygrove]

I don't think this matrix can observe anything. CometRound.getSupportLevel only returns Unsupported or Compatible, and the Unsupported arm in exprToProtoInternal never reads allowIncompatible. So both matrix values run the exact same code, and the whole file (including the overflow tables and every existing query) runs twice. The sql-file-tests doc asks us not to add ConfigMatrix speculatively.

Could we drop the matrix here? If the goal is to guard against float/double being reclassified as Incompatible in the future, that would be better handled by whichever PR makes that change adding the row 2 coverage at that point.

[andygrove]

This is the first fixture in sql-tests/ that changes config with a mid-file SET. It works because the Config: ...codegen.enabled=true line at the top makes withSQLConf restore the flag after the file, but nothing in the fixture says that, and the top-of-file directive now reads as if the dispatcher is on for the whole file. If someone later removes that seemingly redundant Config line, the SET leaks and every fixture that runs afterwards silently loses dispatcher coverage.

Could we follow the existing pattern instead and move these three queries into their own fixture with -- Config: spark.comet.exec.scalaUDF.codegen.enabled=false at the top? misc/codegen_dispatch_disabled_fallback.sql is the precedent. That keeps each file under a single config.

[sunchao]

Correctness

Reviewed f79d7b21 against base 3810936b. The existing fixture checked native integral/decimal rounding and dispatched floating-point rounding, but did not pin the dispatcher-disabled behavior. This change adds three queries that require integral and decimal expressions to remain native and float/double expressions to fall back with the disabled-dispatcher reason. It changes one SQL test file and no production code.

I checked RoundBase and its tests on the maintained Spark 3.5 and 4.0 branches. Both use HALF_UP rounding, preserve nulls and special floating-point values, and distinguish ANSI overflow from legacy wrapping. The fixture runs with ANSI disabled and constant folding excluded. Its existing column and literal cases retain coverage of nulls, NaN/infinities, signed zero, decimal-string rounding boundaries and legacy long overflow. This PR does not establish new ANSI coverage. The new assertions compare Spark results as well as checking native routing or the specific fallback reason. I found no new or remaining verified P1/P2 issue.

Validation

At 2026-09-12 21:37:04 UTC, CI had 41 successful and 11 skipped checks, including successful Required Checks. I verified both matrix rows in the Spark 3.5 expressions job and Spark 4.0 expressions job. Their checkout 596fc7ce has parents exactly this base and head. Its only change from the base is the reviewed fixture, with the same blob as the head. The relevant runner and routing code are unchanged across the head, merge base and current base. I did not run separate local JVM/native tests or reproduce the author's injected regressions. Maintained Spark 3.4/4.1 source coverage remains unavailable.

Performance

There is no production execution change. The matrix runs all 14 queries and seven statements twice even though CometRound returns only Compatible or Unsupported, neither of which reads this allowIncompatible flag. The second row took 1.191 seconds in the inspected Spark 3.5 job and 1.310 seconds in Spark 4.0. Those are test durations, not expression benchmarks. The existing matrix discussion already identifies this redundant work. I have no additional performance finding.

Design

The supported-type query directly guards against making native support depend on the dispatcher. Separate float and double queries make the expected fallback reason unambiguous. The mid-file SET currently stays within withSQLConf: the top-level dispatcher directive saves and restores that same key, including on failure, and each matrix case gets a fresh scope. I found no current configuration leak. The existing fixture-structure discussion already covers making this dependency clearer through a separate disabled-dispatch fixture.

Abstraction & complexity

The change uses the existing SQL fixture parser and routing assertions without adding helpers or production abstractions. expect_native(round) checks both the native expression set and absence from the dispatched set, while expect_fallback checks results and the named reason. This is an appropriate level for these routing checks. I have no additional findings beyond the existing discussion.

[andygrove]

The ConfigMatrix line has a second problem beyond the one in the other thread. CometRound does not override getExprConfigName, so the name Comet derives comes from Round.getSimpleName and the key it actually reads is spark.comet.expression.Round.allowIncompatible with a capital R. Every other fixture in sql-tests/ uses the class name, for example spark.comet.expression.Cast.allowIncompatible. SQLConf lookups are case sensitive, so the lowercase key here is never read, and both matrix rows run with allowIncompatible at its default of false. That also means the fork CI run cited in the description never exercised what the matrix claims to exercise. If the matrix stays, could you fix the casing? If it goes away as suggested in the other thread then this goes with it.

The other thing is the null scale query at line 44. It is the one case in the file whose mechanism is not pinned, and I think it pins the wrong thing. d and f are Unsupported per getSupportLevel, so they never reach CometRound.convert. What actually runs is Spark's own RoundBase.doGenCode inside the dispatcher, which has its own null scale short circuit. The case _ if scaleV == null branch in CometRound.convert is only reachable for Compatible children, and grepping the repo it is not reached by any test today. Since this PR is about pinning round routing, would it make sense to add round(dec, NULL) and round(i, NULL) as an expect_native(round) query? I would want to see that pass before calling the routing covered. That branch emits Literal(null), which is NullType, while the plan types round(dec, NULL) as decimal(6,0) and round(i, NULL) as int, so I am not sure the emitted literal matches the declared output type.