Repository navigation
test: compare in-memory caches with independent answers - #6262
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem:
checkSparkAnswercould compare a cached query against the same cached payload and statistics, allowing incorrect values or pruning to pass. - Design approach: Replace those comparisons with explicit rows or Spark answers collected before caching. Add a differential pruning suite for native Arrow, Spark columnar, and row inputs.
- Correctness / compatibility analysis: Reviewed all three PR commits and all five changed files, including surrounding cache and test-helper code. Compared relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. The NaN comparisons, signed-zero fixture, pruning counts, and independent expected answers match the applicable semantics.
- Key design decisions: Keeping the pruning oracle in memory avoids Parquet pruning influencing signed-zero expectations. Writer-plan and batch-count assertions verify that the intended paths execute. The local helper and parameterized suite keep the design straightforward.
- Implementation sketch: Update existing cache and Kryo assertions, add 23 predicates across three writer paths, and register the new suite in both Linux and macOS workflows.
- Behavioral changes worth calling out: Production behavior is unchanged. Tests now detect cache errors independently. The three new cases took approximately 2.2 seconds combined in the verified fork CI run.
- Suggested improvements: None at P1/P2 severity. No introduced P1/P2 issues found within this review.
Reviewed full SHA: 4924c36d3e4655909349bb11238905aed1957179. Scope was the full PR diff against base 74fdec5bcdb95f209104a2b698712f668aea4ae2, using merge base f7952de7313775f78ba41eebca2020c4915c16ae. The PR was not a draft. The snapshot and live discussion contained no reviews, issue comments, inline comments, or review threads.
Routed skills: review-comet-pr, review-comet-expression-pr, and review-comet-ffi-pr.
Exact-head CI: Upstream has a successful label check but no build/test verdict. Independently inspected fork run 36267399368: Required Checks, native build, Rust tests, and all four Linux Spark 4.1/JDK 17 groups passed. Its tested merge commit has the same tree as the reviewed head. Logs confirm all 61 cache tests passed within the exec group's 1,079 successful tests.
Validation limits: Local dev/ci/check-ci-config.py and git diff --check passed. JVM/native suites were not rerun locally because this checkout lacks Maven dependencies and a built native library. macOS, nondefault Spark runtime profiles, Spark SQL, and Iceberg suites were skipped in the verified fork run. The author's cross-version runs and fault-injection results were not independently reproduced.
andygrove
left a comment
There was a problem hiding this comment.
I checked this by planting the two bugs from #6203 in ArrowCachedBatchSerializer. With both in, 10 of the 61 cache tests fail, where 3 of 58 failed before. An int corruption confined to the Spark-vectorized convert path used to pass everything and now fails the pruning suite's Spark columnar case. Each writer's corruption is caught by its own case. The three suites pass on 3.4, 3.5, 4.0, 4.1 and 4.2.
| "dec >= -1.125 AND dec < 2.125", | ||
| "ts < TIMESTAMP '1960-01-05 00:00:00'", | ||
| "b <=> true", | ||
| "id IN (1, 9, 49)", |
There was a problem hiding this comment.
No predicate here can catch an off-by-one bound on an int column. id IN (1, 9, 49) lands on the second row of each of its batches, and n only appears in n IS NULL, which prunes on the null count. If every int batch reports its upper bound one lower, this suite still passes on all three writers, and only the typed-bounds tests in CometInMemoryCacheSuite notice. Could we add n = 5? n is constant within a batch, so that one predicate sits on both bounds of batch 5.
There was a problem hiding this comment.
@andygrove Thanks for pointing this out. I'll add the n = 5 coverage in a follow-up issue.
There was a problem hiding this comment.
Opened #6378 to track adding n = 5 coverage for integer bounds.
Which issue does this PR close?
Closes #6203.
Rationale for this change
Disabling Comet does not bypass a cached relation or its static serializer. Comparing a cached query with
checkSparkAnswercan therefore accept the same corrupted values or incorrect pruning on both sides. These tests need expected answers computed independently of the cache.What changes are included in this PR?
How are these changes tested?
CometInMemoryCacheSuite,CometInMemoryCacheKryoSuite, andCometInMemoryCachePruningSuitelocally across Spark 3.4–4.2: Spark 3.4 had 56 passed / 5 version-gated cancellations; Spark 3.5 had 59 / 2; Spark 4.0, 4.1, and 4.2 had 61 passed each. Spark 4.0 also passed semantic Scalafix CHECK.4924c36d3: omitting NaN from double statistics makes the NaN test and all three differential writer tests fail (4/4). Adding 1.0 to cached doubles causes 9 failures, including non-Arrow columnar input, both Kryo storage levels, and all three new writer tests. Production source was restored afterward and all 61 Spark 4.1 cache tests passed again.Required Checks, native build, Rust tests, all four Linux Spark 4.1 / JDK 17 test groups, static checks, TPC-H, and TPC-DS under three join configurations. The exec group passed 1,079 tests with 0 failures and 0 cancellations; its logs confirm all 61 cache tests ran, including the three new writer cases. Optional workflows followed the repository's normal PR conditions.