andygrove opened a new issue, #6615:
URL: https://github.com/apache/datafusion-comet/issues/6615
## 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 3bc2faa934. 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 | | 40 | 7 | 25 | 5 | 0 | 3 |
| Decimal arithmetic | | 21 | 0 | 10 | 2 | 0 | 9 |
| Cast from boolean, integral, floating-point, decimal | | 118 | 1 | 112 |
1 | 0 | 4 |
| Cast from string | | 48 | 0 | 47 | 1 | 0 | 0 |
| Cast from date, timestamp, binary, complex types | | 44 | 0 | 43 | 0 | 0
| 1 |
| Datetime | | 57 | 6 | 42 | 1 | 0 | 8 |
| String | | 56 | 18 | 36 | 1 | 0 | 1 |
| Regex | | 95 | 14 | 58 | 8 | 0 | 15 |
| Arrays and higher-order functions | | 81 | 3 | 43 | 23 | 8 | 4 |
| Maps and structs | | 45 | 1 | 24 | 17 | 2 | 1 |
| Generators (explode, posexplode) | | 43 | 1 | 40 | 0 | 0 | 2 |
| Aggregates | | 127 | 0 | 90 | 4 | 8 | 25 |
| Window functions and float sort keys | | 64 | 25 | 35 | 0 | 2 | 2 |
| Collation (Spark 4) | | 73 | 6 | 49 | 2 | 4 | 12 |
| Hash and bitwise | | 57 | 10 | 45 | 1 | 0 | 1 |
| Conditional, JSON, CSV, Variant, misc | | 93 | 5 | 38 | 3 | 4 | 43 |
| Float semantics sweep (412 runtime cases) | | 5 | 0 | 1 | 0 | 4 | 0 |
| Codegen dispatcher | | 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 #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)
- [ ] (to be filed) test: `query tolerance=` in Comet SQL tests passes when
either side is NaN
- [ ] (to be filed) test: add a Comet SQL test error mode that checks native
execution and error-class parity
- [ ] (to be filed) test: let Comet SQL tests keep ConstantFolding, exclude
optimizer rules and change configs mid-file
- [ ] (to be filed) docs: document the ways a Comet SQL test can silently
miss Comet
### By area
- [ ] (to be filed) test: move math and arithmetic coverage from Scala
suites to Comet SQL tests
- [ ] (to be filed) test: move decimal arithmetic coverage from Scala suites
to Comet SQL tests
- [ ] (to be filed) test: move cast coverage for boolean, integral,
floating-point and decimal sources to Comet SQL tests
- [ ] (to be filed) test: move cast coverage for string sources to Comet SQL
tests
- [ ] (to be filed) test: move cast coverage for date, timestamp, binary and
complex-type sources to Comet SQL tests
- [ ] (to be filed) test: move datetime expression coverage from Scala
suites to Comet SQL tests
- [ ] (to be filed) test: move string expression coverage from Scala suites
to Comet SQL tests
- [ ] (to be filed) test: move regex coverage to Comet SQL tests and fix
fixtures whose patterns lose their backslashes
- [ ] (to be filed) test: move array expression coverage from Scala suites
to Comet SQL tests
- [ ] (to be filed) test: move map and struct expression coverage from Scala
suites to Comet SQL tests
- [ ] (to be filed) test: move explode/posexplode coverage from
CometGenerateExecSuite to Comet SQL tests
- [ ] (to be filed) test: move aggregate function coverage from
CometAggregateSuite to Comet SQL tests
- [ ] (to be filed) test: move window function coverage from
CometWindowExecSuite to Comet SQL tests
- [ ] (to be filed) test: move the Spark 4 collation suites to Comet SQL
tests
- [ ] (to be filed) test: move hash and bitwise expression coverage from
Scala suites to Comet SQL tests
- [ ] (to be filed) test: move conditional, JSON, CSV, Variant and other
expression coverage to Comet SQL tests
- [ ] (to be filed) test: move CometFloatSemanticsSuite's matching cases to
Comet SQL tests
- [ ] (to be filed) test: move expression-level codegen dispatcher tests to
Comet SQL tests
Related:
- #4616: testing dual-implementation expressions across the routing matrix.
The `routing_*` fixtures are its home.
- #3406: point "Adding a New Expression" at SQL tests.
- #4770: collation suite shim. Most of its duplication goes away once those
tests are fixtures gated on 4.0.
- #5968: float rendering through SQL tests.
- #4765: an earlier, closed attempt at four temporal tests.
--
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]
