test: check Iceberg DPP pruning by planned file tasks - #6280
dwsmith1983 wants to merge 4 commits into
Conversation
Two Iceberg DPP tests used numPartitions == 1 as evidence of pruning, but Iceberg packs their small files into one Spark partition whether or not DPP prunes, so the checks passed with DPP off. Count the file tasks the scan planned instead, and the splits read where the scan runs once.
github-actions
Bot
added
enhancement
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Two Iceberg DPP tests asserted one Spark partition, which can contain all three unpruned files.
- Design approach: Count planned file tasks to distinguish successful pruning from small-file packing.
- Correctness / compatibility analysis:
perPartitionDataserializes Spark’s DPP-filtered input partitions. The nativenum_splitscounter counts executed file tasks. Checked the relevant Spark sources for 3.4.3, 3.5.9, 4.0.4 and 4.1.3, plus Iceberg’s filtering and task grouping. Spark 4.2 already skips this suite because its Iceberg runtime is incompatible. - Key design decisions: Both tests inspect planned tasks. Only the query without
ORDER BYassertsnum_splits == 1, avoiding the additional scan execution caused by range-bound sampling. - Implementation sketch: Parse each partition’s existing
IcebergScanprotobuf and sumgetFileScanTasksCount. The existing Spark-result and broadcast-reuse checks remain intact. - Behavioral changes worth calling out: Stronger test assertions only. Parsing is confined to the small fixtures, with no production overhead or new abstraction.
- Suggested improvements: No introduced P1/P2 issues found within this review.
Reviewed the entire diff from 605051ad239ef704f5f25d67910a446a6b6d7c70 to c8d70873f282ee672e1211af6781f9b1065ddb88: one commit and one test file. The PR is not a draft. Existing reviews, issue comments, inline comments and review threads were empty. Routed skill: review-comet-pr; no sibling skill applies to this test-only scope.
Exact-head CI: Comet CI, Check PR Title and CodeQL report action_required. Only the labeling workflow passed. There is no build/test verdict.
Validation limits: git diff --check passed. A bounded make core attempt failed with No space left on device, preventing focused Scala execution. The author’s reported cross-version test results were not independently reproduced. No tracked files were changed.
|
@andygrove could you approve a CI run on |
2 participants
Which issue does this PR close?
No issue. This only changes tests.
Rationale for this change
Two Iceberg DPP tests use the native scan's
numPartitions == 1as evidence that DPP pruned the fact table:Iceberg packs the tests' small files into one Spark partition whether or not DPP prunes, so both checks pass with DPP off. Running each query on main with
spark.sql.optimizer.dynamicPartitionPruning.enabled=falsestill gives one partition, but the scan plans 3 file tasks instead of 1.What changes are included in this PR?
Both tests now count the Iceberg file tasks the scan planned (the sum of
getFileScanTasksCountoverperPartitionData) and expect 1 of the 3 files. The AQE test also expects thenum_splitsmetric to be 1. The join test does not checknum_splits, because itsORDER BYruns the scan once to sample range bounds and again for the shuffle, so each split is read twice.The other two Iceberg DPP tests that check
numPartitionsare left alone. Their unpruned scans have 8 and 2 partitions, so the check already fails without pruning.How are these changes tested?
The two tests pass on Spark 3.4, 3.5, 4.0 and 4.1. With DPP turned off for the query, the old
numPartitionschecks still pass and the new checks fail, planning 3 tasks where 1 is expected.