fix(read): keep fragments the zonemap does not cover - #781
Conversation
dcfcdb2 to
31bc194
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Coverage must describe the same logical zonemap index that supplies the loaded stats. Combining coverage from multiple same-column indexes can mark fragments as safe to prune even though their stats came from a different, partial index.
A viable revision would either associate stats with the exact index whose fragment bitmap is used, or conservatively disable pruning when multiple zonemap indexes cover the same column.
Please mark this PR with the breaking-change label.
geruh
left a comment
There was a problem hiding this comment.
Hey @kangnan-li thanks for raising!! I did a pass over the logic let me know what you think.
31bc194 to
59770bc
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The revision now derives conservative coverage from the fragment IDs in the loaded zonemap statistics. The same-column two-index regression, existing partial-coverage cases, zonemap behavior, and sharding behavior all pass.
Please mark this PR with the breaking-change label.
59770bc to
de40f16
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The current revision retains conservative coverage derived from the fragment IDs in the loaded zonemap statistics. The follow-up only trims explanatory comments, so the verified same-column two-index, partial-coverage, zonemap, and sharding behavior is unchanged.
Please mark this PR with the breaking-change label.
geruh
left a comment
There was a problem hiding this comment.
Nice work @kangnan-li! This shape is a lot better I left a few nits for readability and correctness in the comments let me know what you think
de40f16 to
8942e62
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The revision removes an unreachable null-coverage fallback and trims explanatory comments. Production callers always provide a coverage map, so conservative zonemap pruning behavior remains unchanged.
Please mark this PR with the breaking-change label.
A zonemap index can lag the dataset: fragments appended after the last index build carry no zone entries. The pruner built its surviving set solely from fragment IDs present in ZoneStats, so those fragments were silently dropped — contradicting the class's own documented contract that pruning "will never exclude fragments that do contain matching rows". The result is a filtered scan that returns a subset of the matching rows with no exception and no warning. Coverage is derived from the loaded ZoneStats themselves, per column. Index metadata cannot be used for this: a column may carry several zonemap indexes and getZonemapStats returns the zones of only one of them, so the union of every index's fragment bitmap claims coverage for fragments the loaded zones never saw. A fragment absent from the zones actually being evaluated is unknown, so it stays in the scan. Per-column matters too, because indexes on different columns cover different fragments. Each predicate's fragment set is widened by its own columns' unindexed fragments BEFORE the per-predicate sets are intersected; widening afterwards lets one column's predicate eliminate a fragment its index never described. - pruneFragments takes Map<column, uncoveredFragments>. A null map means coverage is unknown and disables pruning. The 2-arg overload is retained for compatibility but deprecated: it asserts full coverage, which it cannot verify. - loadZonemapStats returns stats plus per-column coverage instead of setting a field as a side effect, so the two cannot fall out of step. - Stats are loaded for every requested column. Sharding detection consumes them and needs only per-fragment min/max, so storage-partitioned joins keep working on tables whose index metadata cannot be read. Only pruning is disabled. - LanceScan no longer recomputes pruning without coverage, which would reintroduce the bug exactly when the builder declined to prune. Its now-unused zonemapStats field is removed. Tests: ZonemapFragmentPrunerTest gains 6 cases covering partial coverage, a fragment covered by one column's index but not another's, widening scoped to a predicate's own columns, and unknown coverage disabling pruning. BasePartialZonemapCoverageTest drives real Spark SQL over a table whose index covers 3 of 5 dates, a two-column table whose indexes cover different fragments, and two indexes on the same column.
8942e62 to
0dfcd66
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The implementation derives per-column coverage from the loaded zones and conservatively retains fragments those zones do not describe. This revision only trims Javadocs from the previously verified implementation; executable code and tests are unchanged, and Spotless passes on the current head.
Please mark this PR with the breaking-change label.
|
Make a few changes based on comments, and resolved conversations for minor changes, and keep major discussions unresolved for reviewer to review again |
The zonemap partial-coverage bug was fixed by lance-format#781, so the documentation should not warn about it. Also trim inline comments and test Javadocs to match the code, not the PR history.
Closes #780
Problem
ZonemapFragmentPrunerbuilds its surviving-fragment set solely from fragment IDs present in the returnedZoneStats, so a fragment with no zone entry is treated as provably non-matching and pruned away.A zonemap index can legitimately cover only part of a dataset — fragments appended after the last index build carry no zone entries. Their contents are unknown, not provably non-matching. Dropping them makes a filtered scan return a subset of the matching rows with no exception and no warning, contradicting the class's own documented contract:
Partial coverage is the steady state of a date-partitioned ingestion pipeline: the index is rebuilt periodically while new data is appended continuously. Every filtered read between rebuilds is silently wrong. See #780 for the full analysis and the two other known triggers (#514's racing distributed builds, and
describe_indicesthrowing on a MemWAL system index).Changes
ZonemapFragmentPruner— new overload takingMap<column, uncoveredFragments>: for each pushed predicate, the fragment set it yields is widened by the fragments its own columns' indices do not describe, before the per-predicate sets are intersected.Coverage has to be per column, because indexes are built independently and cover different fragments. A single dataset-wide uncovered set is not sufficient: with an index on
acovering{0}and one onbcovering{0,1}, the union leaves fragment 1 looking covered, so a predicate onastill eliminates it even thougha's index never described it — the original bug, one column over. Widening per predicate before the intersection is what closes that.A
nullmap means coverage is unknown and disables pruning entirely. The 2-arg overload is retained for source compatibility but deprecated: it asserts full coverage, which it cannot verify.LanceScanBuilder—loadZonemapStatsnow returns the stats together with each column's uncovered-fragment set, rather than setting a field as a side effect, so the two cannot fall out of step. Coverage is derived from the loadedZoneStatsthemselves: a column may carry several zonemap indexes andgetZonemapStatsreturns the zones of only one of them, so index metadata would report the union of all of them and claim coverage for fragments the loaded zones never saw. A fragment absent from the zones actually being evaluated is unknown, so it stays in the scan. This also drops thedescribeIndicescall from the read path entirely.Zone stats are loaded for every requested column regardless of what index discovery reports. Sharding detection consumes the same stats and needs only per-fragment min/max, so storage-partitioned joins keep working on tables whose index metadata cannot be read — e.g. one carrying a MemWAL system index, where
describe_indicesfails withNo scalar index plugin found for name 'memwal'. Only pruning is gated on coverage.LanceScan— removed the fallback that recomputed pruning without coverage information; it would reintroduce the bug precisely when the builder had deliberately declined to prune. ThezonemapStatsfield it was the sole reader of is now gone.Net effect: unknown or partial coverage degrades to a correct full scan instead of a silently wrong result. Fully-covered indexes prune exactly as before.
Tests
Unit —
ZonemapFragmentPrunerTest37 → 43 cases: uncovered fragment survives when another fragment matches; survives when no covered fragment matches (the production shape); survives across a multi-column intersection; survives when covered by one column's index but not another's; unknown coverage disables pruning; and a guard that fully-covered stats still prune exactly, so the fix cannot degrade into "never prune".Integration —
BasePartialZonemapCoverageTestdrives real Spark SQL against a Lance table with aSTRINGdate column whose index covers 3 of 5 dates (the remaining 2 appended afterwards): filters on indexed and unindexed dates, per-date counts reconciling with a full scan, a range spanning both, and a value absent everywhere still returning 0 rows. Two further cases cover multiple indexes: one table with indexes on two different columns covering different fragments, and one with two indexes on the same column (the case from review).Reverting just the fix reproduces the failure end-to-end:
Full suite passes: 1,944 tests across
lance-spark-base_2.12andlance-spark-3.5_2.12. Spotless and checkstyle clean.Additionally verified outside the test harness by building the
4.1_2.13bundle and running the same dataset through a real Spark job with the old and new jar — old jar reproduces the failure, new jar returns correct counts while still pruning.Cost of the fix
Pruning is disabled when coverage cannot be established, so a query that previously pruned (incorrectly) now scans. Measured on dev tables with one distinct date per fragment, probing a date that lives in an uncovered fragment:
All rows return correct counts; before the fix every uncovered probe returned 0 rows instantly.
The penalty grows with fragment count, not just with the coverage ratio — at 50% coverage it is 6.9x on 300 fragments but 33x on 5000. (0% coverage is cheaper than 10%: with no stats at all the pruning path is skipped entirely and Spark plans a plain scan, whereas partial stats cost planning work that saves little.) One Spark task is scheduled per surviving fragment, so per-task overhead dominates once thousands of fragments are unpruned. On a table with hundreds of thousands of fragments, a badly stale index will push a filtered read towards full-scan cost. Keeping the index current matters.
That is the intended trade: the fragments the index does not describe genuinely have to be read. A fully-covered index is unaffected —
uncoveredFragmentIdsis empty and the addedaddAllis a no-op (testFullyCoveredStatsStillPruneExactly).LanceScanBuilderlogs covered-vs-total fragment counts whenever any are uncovered, so a degrading index is visible rather than silent.Test matrix
lance-spark-4.1_2.13includeslance-spark-3.5_2.12/src/test/javain its test sources, so the new suite runs on both Spark lines. Verified on Spark 4.1 / Scala 2.13:PartialZonemapCoverageTest(6),ZonemapIndexTest(3),BucketSpjTest(2) all pass.