test: add expression fallback-invariance suite - #5329
Open
4ktLuffy wants to merge 1 commit into
Open
Conversation
For every expression Comet rates compatible, forcing it back to Spark via spark.comet.expression.<Name>.enabled=false must not change a query's outcome -- neither its rows nor whether and what it throws. The config already ships and QueryPlanSerde already honours it for all 293 registered expressions, so the invariant is free; it is not currently swept. The suite runs one fixed query per expression twice, gates every comparison on plan evidence that the flip actually moved execution, and compares three-valued outcomes so a value on one leg against an exception on the other is a failure with a named witness. A plan that proves nothing is reported SKIPPED-VACUOUS, never counted as a pass. 25 expressions over the existing CometFuzzTestBase fixture, ~10s, no product code. Verified under spark-3.4, spark-3.5 and spark-4.1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #5328.
Rationale for this change
For every expression Comet rates compatible, forcing it back to Spark with
spark.comet.expression.<Name>.enabled=falsemust not change a query's outcome — neither its rows nor whether and what it throws. The invariant is free: the config already ships, andQueryPlanSerdealready honours it for all 293 registered expressions.It is not currently swept. Four tests exercise the lever, each on one hand-picked expression; all four check the answer, but none checks a range of expressions mechanically and none checks that the two legs raise the same error.
Replaying #5218's pre-fix logic shows the shape has teeth: the defect is invisible to
CometFuzzMathSuite(30/30),CometCodegenFuzzSuite(28/28),CometCodegenHOFSuite(5/5) andCometExecRuleSuite(29/29), while this suite names it:To be clear about what that is and isn't:
AddMonthsis in the corpus because #5218 named it, so this is a validation replay rather than an independent find, andCometCodegenSuitealready guards that exact query end-to-end. The generic-detector misses are the load-bearing part, and the comparator is not specific toadd_months— it flags this shape for any corpus expression.What changes are included in this PR?
One new suite,
CometFallbackInvarianceSuite, extendingCometFuzzTestBase. No product code.ProjectExec; forced leg shows a SparkProjectExec. A plan showing neither isSKIPPED-VACUOUSand reported — never a pass. Plan presence is captured whether or notcollect()succeeded, so a leg that throws is gated too.RowsvsThrew(errorClass)): value-vs-throw is a failure, differing error classes are a failure, row-count mismatch is a failure. Error parity is where Codegen dispatcher: whole-tree NullIntolerant short-circuit suppresses ANSI errors, plus TIME type gaps between canHandle and the runtime dispatcher #5218 lives, so these comparator rules are load-bearing rather than incidental.FAIL· value-vs-throwFAIL· differing error classFAIL· length mismatchFAIL· incompatible-rated divergenceEXCUSED, logged but never certified · NaN /-0.0/ NULL as distinct tokens · row order canonicalised by sorting.How are these changes tested?
The suite is the test. On a clean tree:
24 pass, 0 fail, 1 skipped-vacuous per variant × 3 variants, ~10 s. The one skip is honest and expected:
StringTranslateis incompatible-rated, so it already runs on Spark by default and there is no native leg to compare against. Verified underspark-3.4,spark-3.5andspark-4.1.Validated by injected divergence twice: a deliberately broken
spark_decimal_divas a positive control (also caught byCometFuzzMathSuite, as it should be), and the #5218 pre-fix replay. The probe run reportspass=23 fail=1— one delta from clean, and it is the witness quoted above.On the bind gate. It earned its place three times over while this suite was being written. An early version reported 22 passes where the truth was 19 passes and 3 vacuous. A second pinned ANSI off and mapped throw-vs-value to a harness error, making it structurally blind to the very bug class it exists for. A third used
ORDER BYfor determinism, which introduces a shuffle and roots the executed plan atAdaptiveSparkPlanExec— whosechildrenis empty, so plan inspection silently read zero for every query in that section and the gate stopped working entirely. Comet's ownspark.comet.expression.*tests sidestep all of this by using simple queries with no shuffle; this suite now does the same, avoidingORDER BYand canonicalising row order in the comparator instead.Scope, stated plainly. 25 of 293 expressions. The suite owns one historical bug (#5218) and is not claimed to cover operator-level mixed execution, fallback-decision logic, or scan-metadata mismatches — #4813, #4051, #4789 and #2720 are different mechanisms and this invariant would not have caught any of them.
Provenance note: produced during an AI-assisted audit of Comet's verification machinery, human-verified at each gate; mutation experiments were run with positive controls and the tree restored clean afterward.