Skip to content

test: add expression fallback-invariance suite - #5329

Open
4ktLuffy wants to merge 1 commit into
apache:mainfrom
4ktLuffy:fallback-invariance-suite
Open

test: add expression fallback-invariance suite#5329
4ktLuffy wants to merge 1 commit into
apache:mainfrom
4ktLuffy:fallback-invariance-suite

Conversation

@4ktLuffy

Copy link
Copy Markdown

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=false must not change a query's outcome — neither its rows nor whether and what it throws. The invariant is free: the config already ships, and QueryPlanSerde already 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) and CometExecRuleSuite (29/29), while this suite names it:

FAIL-OUTCOME [errors] AddMonths default=VALUE(2024-02-01) forced=THREW(CAST_INVALID_INPUT)

To be clear about what that is and isn't: AddMonths is in the corpus because #5218 named it, so this is a validation replay rather than an independent find, and CometCodegenSuite already guards that exact query end-to-end. The generic-detector misses are the load-bearing part, and the comparator is not specific to add_months — it flags this shape for any corpus expression.

What changes are included in this PR?

One new suite, CometFallbackInvarianceSuite, extending CometFuzzTestBase. No product code.

  • Fixed query corpus — 25 expressions over the existing seeded fuzz fixture, no new fixtures. A value-parity section with ANSI off, and an error-parity section with ANSI set explicitly on so it behaves identically on every Spark profile.
  • Bind gate, mandatory. Before any comparison, the executed plan must prove the flip moved execution: default leg shows Comet operators and zero Spark ProjectExec; forced leg shows a Spark ProjectExec. A plan showing neither is SKIPPED-VACUOUS and reported — never a pass. Plan presence is captured whether or not collect() succeeded, so a leg that throws is gated too.
  • Three-valued outcome comparison (Rows vs Threw(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.
  • Verdicts — compatible-rated divergence FAIL · value-vs-throw FAIL · differing error class FAIL · length mismatch FAIL · incompatible-rated divergence EXCUSED, 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:

FALLBACK-INVARIANCE-RESULT pass=24 fail=0 excused=0 vacuous=1
FALLBACK-INVARIANCE SKIPPED-VACUOUS [values] StringTranslate not-native-by-default

24 pass, 0 fail, 1 skipped-vacuous per variant × 3 variants, ~10 s. The one skip is honest and expected: StringTranslate is incompatible-rated, so it already runs on Spark by default and there is no native leg to compare against. Verified under spark-3.4, spark-3.5 and spark-4.1.

Validated by injected divergence twice: a deliberately broken spark_decimal_div as a positive control (also caught by CometFuzzMathSuite, as it should be), and the #5218 pre-fix replay. The probe run reports pass=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 BY for determinism, which introduces a shuffle and roots the executed plan at AdaptiveSparkPlanExec — whose children is empty, so plan inspection silently read zero for every query in that section and the gate stopped working entirely. Comet's own spark.comet.expression.* tests sidestep all of this by using simple queries with no shuffle; this suite now does the same, avoiding ORDER BY and 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a fallback-invariance suite: query outcome should not change when a compatible expression is forced from native to Spark

1 participant