4ktLuffy opened a new issue, #5328: URL: https://github.com/apache/datafusion-comet/issues/5328
### What is the problem the feature request solves? `QueryPlanSerde` honours `spark.comet.expression.<Name>.enabled=false` as forced fallback, and there are 293 expressions registered to it: ```bash grep -oE "classOf\[[A-Za-z0-9_]+\] ->" spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala | sort -u | wc -l ``` That gives a free invariant, with no product change needed: **for any expression Comet rates compatible, a query's outcome — its rows, or the error it raises — must be identical whether the expression is evaluated natively or forced back to Spark.** Four distinct tests exercise that lever today, each pinning one hand-picked expression: `disable expression using dynamic config` (`Add`) in `CometExpressionSuite`, plus the `ToPrettyString`, `WidthBucket` and `StringDecode` suites. All four *do* check the answer, via `checkSparkAnswerAndOperator` on the native leg and `checkSparkAnswerAndFallbackReason` on the forced leg. What is missing is a mechanical sweep across the registry, and any systematic check that the two legs agree on **which error** they raise. ### The evidence, and its limits To test whether an invariance sweep of this shape has teeth, I reapplied the pre-fix condition from #5218 — dropping the `inputOrdinals.size == 1 || rootChildrenAreLeaves(expr)` narrowing from `CometBatchKernelCodegen.canShortCircuitNulls`, so the null short-circuit again applies to a whole tree — and ran a set of detectors against that one probe on one build: | Detector | Result | |---|---| | `CometFuzzMathSuite` | 30/30 pass — miss | | `CometCodegenFuzzSuite` | 28/28 pass — miss | | `CometCodegenHOFSuite` | 5/5 pass — miss | | `org.apache.comet.rules.CometExecRuleSuite` | 29/29 pass — miss | | `CometCodegenSuite` | 1 failure — the `(#5218)` regression guard | | `CometCodegenSourceSuite` | 2 failures — both `(#5218)` guards | | proposed suite | fails: `AddMonths default=VALUE(2024-02-01) forced=THREW(CAST_INVALID_INPUT)` | **Two honest caveats on that table, stated up front.** The three suites that catch the probe are #5218's own regression tests, shipped with its fix — their failure shows the probe is genuinely behaviour-changing, and nothing more. And my corpus contains `AddMonths` *because* #5218 named it, so the last row is a validation replay, not an independent rediscovery: `CometCodegenSuite`'s guard already covers that same query end-to-end. What the table does support is narrower — four generic mechanical detectors miss this class, including a fuzzer aimed squarely at codegen, while a fallback-invariance comparison surfaces it with a named witness. The comparator is not specific to `add_months`; it would flag this shape for any expression in the corpus. The oracle also differs from the existing guard's. `CometCodegenSuite`'s test compares Comet against Spark directly. This suite compares Comet-native against Comet-with-that-expression-forced-to-Spark, which is the config lever no current test sweeps. Same build with the probe reverted: **24 pass / 0 fail / 1 skipped-vacuous** — the single delta is the witness above. ### Describe the solution A deterministic scalatest suite, no product code, over the existing `CometFuzzTestBase` fixture. Per expression it runs one fixed query twice — default config, then `.enabled=false` — and: - **gates every comparison on evidence the flip actually moved execution.** The default leg must show Comet operators and no Spark `ProjectExec`; the forced leg must show a Spark `ProjectExec`. A plan that shows neither proves nothing and is reported `SKIPPED-VACUOUS`, never counted as a pass. - **compares three-valued outcomes** (`Rows` vs `Threw(errorClass)`), so a value on one leg against an exception on the other is a failure with a witness rather than a harness error. Differing error classes are also a failure. - treats a divergence on an incompatible-rated expression as `EXCUSED` — logged, never a pass. - canonicalises NaN, `-0.0` and NULL to distinct tokens, and row order by sorting. 25 expressions × the base suite's 3 shuffle/C2R variants = 75 comparisons in **~10 s**, so it is per-PR viable. Compiles and passes under `spark-3.4`, `spark-3.5` and `spark-4.1`. Current scope is deliberately 25 of 293 expressions. Widening is mechanical, and the bind gate is what keeps the coverage claim honest as it grows — without it a sweep like this silently overstates itself, which happened three times while this one was being built. ### Related work I checked first - **#4654** (dispatcher vs incompatible native impls) is the reason the sweep reports one skip rather than a pass: `StringTranslate` is incompatible-rated, so by default it never runs natively and there is no native leg to compare against. Whether an expression is reachable natively at all is a dispatcher question, and the bind gate makes that visible instead of silently counting it as verified. - **#4825 / #4827** (partial project fallback, JVM expression detour) would change the plan shape this suite reads: it keeps an operator native by routing an unsupported subtree through the JVM, so a forced-fallback leg might no longer surface as a Spark `ProjectExec`. That work is default-off today, so there is no conflict now — but the bind gate is the part that would need updating alongside it, and it would report SKIPPED-VACUOUS rather than passing silently if the shape changed underneath it. - I did not find an existing invariance sweep over this lever; if one exists and I missed it, that is the fastest way to close this issue. ### Additional context This came out of a systematic, AI-assisted audit of Comet's verification machinery, human-verified at each step and mutation-tested with positive controls. Given #4085's note that LLM-assisted audits have outperformed fuzzing here, the suite is built to that lesson: fast, deterministic, named witnesses. Happy to share the full evidence trail, including the negative results — the audit also produced two candidate findings about the config lever that dissolved on verification, and those are not being filed. PR attached. -- 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]
