Skip to content

[EPIC] Move expression test coverage from Scala suites to Comet SQL tests #6615

Description

@andygrove

What / Why

Comet SQL tests (CometSqlFileTestSuite, with fixtures under spark/src/test/resources/sql-tests/) can now express most of what the Scala expression suites check:

  • per-file Config / ConfigMatrix for ANSI, time zones, dictionary encoding, shuffle mode, batch size and allowIncompatible;
  • MinSparkVersion / MaxSparkVersion gates;
  • query modes for native coverage, fallback reasons, errors, tolerance, and native versus codegen dispatch (expect_native / expect_dispatch).

Every query is compared against Spark, and constant folding is disabled so all-literal queries run in Comet.

Expression coverage is still split between those fixtures and the Scala suites. About 1,160 Scala test definitions across 45 suites are in scope here. This epic moves their coverage into SQL tests.

The goal is to lose no coverage, not to translate every test one for one.

  • A Scala test is deleted once a fixture covers what it covered: the same inputs, configs and assertion strength, or stronger.
  • Tests that genuinely need Scala stay.
  • Each sub-issue accounts for every test in its scope, so nothing is dropped silently.

Analysis

I classified every test in the expression suites by static analysis of main at 3bc2faa. That includes the expression-level tests in the aggregate, window and generate operator suites. Each sub-issue lists its tests with a verdict and a target fixture.

The verdicts come from reading the code, not from running fixtures, so treat each target as a starting point. If a conversion turns out not to preserve coverage, keep the Scala test and note why in the sub-issue.

Area Issue Tests Covered Convert Blocked Split Keep
Math and arithmetic #6620 40 7 25 5 0 3
Decimal arithmetic #6621 21 0 10 2 0 9
Cast from boolean, integral, floating-point, decimal #6622 118 1 112 1 0 4
Cast from string #6623 48 0 47 1 0 0
Cast from date, timestamp, binary, complex types #6624 44 0 43 0 0 1
Datetime #6625 57 6 42 1 0 8
String #6626 56 18 36 1 0 1
Regex #6627 95 14 58 8 0 15
Arrays and higher-order functions #6628 81 3 43 23 8 4
Maps and structs #6629 45 1 24 17 2 1
Generators (explode, posexplode) #6630 43 1 40 0 0 2
Aggregates #6631 127 0 90 4 8 25
Window functions and float sort keys #6632 64 25 35 0 2 2
Collation (Spark 4) #6633 73 6 49 2 4 12
Hash and bitwise #6634 57 10 45 1 0 1
Conditional, JSON, CSV, Variant, misc #6635 93 5 38 3 4 43
Float semantics sweep (412 runtime cases) #6636 5 0 1 0 4 0
Codegen dispatcher #6637 89 1 23 2 2 61
Total 1156 98 761 71 34 192
  • Covered: an existing fixture already covers the test.
  • Convert: expressible in SQL tests today.
  • Blocked: needs one of the framework changes below first.
  • Split: part converts, part stays.
  • Keep: stays in Scala.

Three findings shape the work.

  1. Few tests are covered today. The fixtures are thinner than the Scala tests:

    • aggregates are grouped only by string keys;
    • many fixtures use tolerance= where the Scala test compares exactly;
    • cast fixtures are non-ANSI only, with no try_cast;
    • datetime fixtures have far fewer formats, time zones and edge values;
    • most array and map fixtures run one-row batches.

    Most of the work is therefore extending fixtures, not deleting tests.

  2. Some existing tests, Scala and SQL, don't test what they claim. These need fixing as part of the move:

    • Regex escapes. Spark drops the backslash before an unknown escape, so '\d' is d. Seven regex fixtures, and the Scala rlike tests, never test \d, \w, anchors or backreferences.
    • NaN under tolerance. query tolerance= skips the comparison when either side is NaN. The NaN results of every per-function math fixture and corr.sql's NaN permutations are never compared.
    • Literal casts. CometCast folds a cast of a literal on the JVM, so literal casts in the cast fixtures never run native code.
    • NULL literals. Spark's NullPropagation folds NULL-literal arguments before Comet sees them; the contributor guide's own ascii(NULL) example is affected.
    • Comet compared with Comet. About 20 cast tests use a helper, assertDataFrameEqualsWithExceptions, that compares Comet with Comet.
    • Vacuous Scala tests, for example:
      • three "null group key" aggregate tests build their keys with null.asInstanceOf[Int], which is 0;
      • a cast test maps over a lazy Iterator and never runs;
      • try_add / try_subtract / try_multiply are constant-folded by Spark;
      • the INT96 conversion legs compare Spark with Spark.
  3. About 71 tests need framework changes first.

    • Structured error parity: expect_error checks only a message substring and never checks that Comet ran the query.
    • Optimizer-rule control: a fixture can't keep ConstantFolding, which the folded-literal tests need, or exclude NullPropagation / ConvertToLocalRelation.

Ground rules for every PR

  1. Account for every Scala test in scope. Each one is converted, cited as already covered, or kept with its reason. The table in each sub-issue is the checklist.
  2. Delete each converted Scala test in the same PR that adds its coverage. When a suite is empty, delete it and remove it from the suite lists in .github/workflows/pr_build_linux.yml and pr_build_macos.yml. A stale name there doesn't fail CI: CometExpressionCoverageSuite is still listed, although chore: remove coverage file auto generator #2854 deleted it.
  3. Make sure the fixture reaches Comet.
    • Read expression inputs from Parquet tables, not from inline VALUES, literal casts or NULL literals.
    • Use one-file inserts (INSERT ... SELECT /*+ COALESCE(1) */ ... or range(0, n, 1, 1)) when batch shape or row order matters.
    • The docs sub-issue lists all of the traps.
  4. Check special float values exactly. Put NaN, ±Infinity and signed-zero results in a plain query, and write negative zero as double('-0.0').
  5. Keep mechanism checks. Where the Scala test pinned native versus dispatch, pin it with expect_native / expect_dispatch.
  6. Random-data tests convert to curated values that include the generator's edge values. The fuzz suites keep the random coverage: CometFuzzTestSuite, CometFuzzAggregateSuite, CometFuzzMathSuite and CometCodegenFuzzSuite.
  7. Spot-check regression tests named after an issue: confirm the fixture fails with the fix reverted.
  8. Run every profile. Pull request CI runs only the default Spark profile, and fixtures carry version gates, so these PRs need the run-all-spark-profiles label.
  9. Watch CI bucket balance. Moving aggregate and window coverage shifts CI time from the exec bucket to the expressions bucket. Rebalance if expressions grows much longer than the others.

What stays in Scala

  • Plan shape: operator counts, canonicalization, exchange reuse, mixed-engine partial/final safety.
  • Metrics, and explain / COMET-INFO text.
  • Serde and proto internals, and unit tests of helpers such as the CometRegex analyzer and the time-zone ID mapping.
  • UDF registration.
  • DataFrame-only inputs: MakeDecimal, non-nullable struct fields, negative-scale decimals, an empty InSet.
  • Unevaluable expressions such as Days / Hours.
  • Raw NaN payload bits.
  • Parquet layouts SQL can't write.
  • The fuzz suites.
  • Comet-only failures that intentionally differ from Spark.

Work

Suggested order:

  1. Start with the framework and docs issues.
    • The tolerance fix is small, and it changes how the math and aggregate conversions are written.
    • The error-parity and optimizer-rule issues unblock the tests counted as blocked in the table.
  2. The area issues are independent of each other and can proceed in parallel. Window, hash/bitwise and generators are the most mechanical. Float semantics is the lowest priority.

Framework and docs (do first)

By area

Related:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions