comphead opened a new pull request, #5987:
URL: https://github.com/apache/datafusion-comet/pull/5987
## Which issue does this PR close?
Closes #5986.
## Rationale for this change
The Spark SQL matrix runs `sql/testOnly *` (`dev/ci/spark-sql-modules.py`),
so every RocksDB
state-store suite under
`sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/`
runs with Comet enabled. None of them exercise a Comet code path:
- `CometExecRule._apply` and `CometScanRule.apply` return the plan untouched
for any streaming plan
(`ShimCometStreaming.isStreamingPlan`, implemented for both 3.x and 4.x),
so the suites that drive
streaming queries run plain Spark end to end.
- The rest call `StateStore`/`RocksDB` APIs directly and never build a query
plan.
Before this PR, exactly one of the six suites was skipped, on one of the
four Spark versions:
| Suite | 3.4.3 | 3.5.9 | 4.0.4 | 4.1.3 | shard |
| --- | --- | --- | --- | --- | --- |
| `RocksDBStateStoreIntegrationSuite` | runs | runs | runs | "ignored" |
`sql_core-1` |
| `RocksDBStateStoreSuite` | runs | runs | runs | runs | `sql_core-2` |
| `RocksDBSuite` | runs | runs | runs | runs | `sql_core-3` |
| `RocksDBStateStoreCheckpointFormatV2Suite` | n/a | n/a | runs | runs |
`sql_core-1` |
| `RocksDBCheckpointFailureInjectionSuite` | n/a | n/a | n/a | runs |
`sql_core-3` |
| `RocksDBLineageSuite` | n/a | n/a | n/a | runs | `sql_core-1` |
There is precedent: `StateStoreSuite`, the
`StateStoreSuiteBase[HDFSBackedStateStoreProvider]` twin
of `RocksDBStateStoreSuite`, is already skipped under Comet on 4.0 and 4.1
with the comment "The
tests target streaming state-store internals (StateStore.get/put/commit),
not SQL execution paths,
and the `maintenance` test is flaky in CI" (#4221). The same reasoning
applies to the RocksDB
provider, which is the heavier of the two: `RocksDBSuite` is ~4000 lines and
`@SlowSQLTest`,
`RocksDBStateStoreSuite` is `@ExtendedSQLTest`, and
`AlsoTestWithRocksDBFeatures` /
`AlsoTestWithEncodingTypes` multiply each case across
changelog-checkpointing, column-family and
encoding permutations. They land in all three `sql_core` shards, the rows
already tuned for the
7 GB runner budget.
### The mixin order matters, and the existing 4.1 entry had it wrong
`RocksDBStateStoreIntegrationSuite` is quoted as "ignored" above because
that is what the patch
intends, not what it does. `dev/diffs/4.1.3.diff` mixes the trait in last:
```scala
class RocksDBStateStoreIntegrationSuite extends StreamTest
with AlsoTestWithRocksDBFeatures with IgnoreCometSuite {
```
`AlsoTestWithRocksDBFeatures` registers tests through `super.test`, and
`super` inside that trait
resolves to whatever is linearized *after* it. With `IgnoreCometSuite`
declared last it linearizes
*before* the trait, so `testWithColumnFamilies` and
`testWithChangelogCheckpointing*` reach
`SQLTestUtils.test` directly and register real tests. Only plain `test(...)`
calls were skipped --
one of that suite's nine registration sites.
Declaring `IgnoreCometSuite` before the trait puts it after in the
linearization, so every
`super.test` lands on the override. Verified against a standalone model of
the trait stack compiled
with Scala 2.13.15:
```
===== ... with AlsoTestWithRocksDBFeatures with IgnoreCometSuite =====
IGNORE plain (disabled when Comet is on)
RUN viaHelper - with colFamiliesEnabled=true (with changelog
checkpointing)
RUN viaHelper - with colFamiliesEnabled=true (without changelog
checkpointing)
RUN viaHelper - with colFamiliesEnabled=false (with changelog
checkpointing)
RUN viaHelper - with colFamiliesEnabled=false (without changelog
checkpointing)
--> registered-to-run: 4, ignored: 1
===== ... with IgnoreCometSuite with AlsoTestWithRocksDBFeatures =====
--> registered-to-run: 0, ignored: 6, duplicate names: 0
```
The same model, run against the exact shapes this PR produces
(`RocksDBSuite` with
`IgnoreCometSuite` as first parent, and `RocksDBStateStoreSuite` with the
abstract base class plus
both `AlsoTestWith*` traits), reports 0 registered-to-run and 0 duplicate
test names.
Inserting the trait only adds it between the `AlsoTestWith*` traits and
`SQLTestUtils` in the
linearization; the relative order of every other parent is unchanged, so
with `ENABLE_COMET=false`
the suites register exactly the test names they do upstream.
## What changes are included in this PR?
`dev/diffs/3.4.3.diff`, `dev/diffs/3.5.9.diff`, `dev/diffs/4.0.4.diff` and
`dev/diffs/4.1.3.diff`
gain `IgnoreCometSuite` on the RocksDB state-store suites, plus a comment on
each explaining why
the suite is skipped and why the mixin order is load-bearing. 14 suite
entries in total.
Spark 3.4's `RocksDBStateStoreSuite` does not extend `SQLTestUtils` (no
`SharedSparkSession`
mixin), so `IgnoreCometSuite` cannot be mixed in there. It gets the inline
`SparkSession.isCometEnabled` override instead, the same pattern
`StateStoreSuite` uses on 4.0/4.1.
Deliberately left running:
- `RocksDBStateStoreLockHardeningSuite` and `RocksDBStateEncoderSuite`
(4.0/4.1)
- `RocksDBSuite` on Spark 3.4 only, where it is still a bare `SparkFunSuite`
All three are plain `SparkFunSuite`s that never start a `SparkSession`, so
Comet is never loaded
and skipping them would drop upstream regression detection for no gain.
Per the contributor guide, no diff file was hand-edited: each Spark tag was
checked out, the
existing diff applied, the Spark sources modified, and the diff regenerated
with
`git diff <tag>`.
## How are these changes tested?
- Round-trip baseline: regenerating each diff from an unmodified patched
tree reproduced the
committed file byte-for-byte, before any edit, so the delta below is only
the intended change.
- Each regenerated diff applies cleanly to a pristine checkout of its tag
(`v3.4.3`, `v3.5.9`,
`v4.0.4`, `v4.1.3`) and round-trips byte-identically afterwards.
- The `git diff` against the previous diff files adds only RocksDB hunks;
the single pair of
removed lines is the reworked 4.1.3 `RocksDBStateStoreIntegrationSuite`
hunk header.
- Trait linearization and test-name uniqueness verified with the compiled
Scala model described
above.
- Every added line is within Spark's 100-character scalastyle limit.
Not run locally: the Spark SQL suites themselves. Confirming the suites now
report as ignored
needs a CI run, so this PR should carry the `run-spark-3.5-tests`,
`run-spark-4.0-tests` and
`run-spark-3.4-tests` labels (4.1 runs in the merge queue).
--
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]
