feat(admission): count the on-disk liquid tier toward the gate budget - #9
Merged
Conversation
The footprint admission gate sized its budget against RAM only, so with the disk tier enabled a scan whose liquid footprint exceeded memory was bypassed to the fallback mount even though it would fit on the on-disk liquid tier without thrashing. Size the gate against both tiers, weighted differently: memory keeps the compaction overcommit (tolerance, the measured RAM crossover) while the disk tier counts at face value — threshold = memory * tolerance + disk. With the disk tier off (max_disk_bytes == 0) the threshold is exactly memory * tolerance, so gate behaviour is unchanged there. Extract the threshold into a pure admission_threshold() and add unit tests pinning the formula (these run everywhere; the cache-based gate tests remain Linux-only via direct_io).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified the threshold math: memory × tolerance + disk correctly reduces to the prior memory × tolerance behavior when the disk tier is off, and the f64 computation avoids the integer-overflow bug an integer budget × tolerance would have. Callers, docs, and tests are all consistent. The migration of existing gate tests to disk = 0 correctly preserves their prior semantics (the old gate ignored disk). LGTM.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cargo doc --document-private-items with -D warnings denies public items linking to a private fn (rustdoc::private_intra_doc_links). State the formula inline in the public docs; keep the link only from private should_bypass.
There was a problem hiding this comment.
Verified the threshold math and behavioral equivalence:
memory × tolerance + diskcomputed in f64 — no overflow at large budgets.- Disk-off case reduces to the prior
footprint > memory × tolerance(andmemory == 0→footprint > 0.0), so gate behaviour is exactly preserved when the disk tier is disabled. - The
build_cache_with_budgethelper now passesdisk = 0, which correctly keeps the existing memory-only gate tests intact. - New unit tests pin the formula unambiguously (ruling out
(mem+disk)×toland disk-scaled-by-tolerance) and the disk-tier integration test is sound.
Clean, well-documented, and well-tested. LGTM.
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The footprint admission gate sized its budget against RAM only. With the disk (liquid) tier enabled, a scan whose estimated liquid footprint exceeded memory was bypassed to the fallback mount even though it would fit on the on-disk liquid tier without thrashing — the disk capacity sat idle.
This sizes the gate against both tiers, weighted differently:
tolerance, the measured RAM crossover where LiquidCache still beats the mount despite spilling).So it is
memory × tolerance + disk, not(memory + disk) × tolerance(which would admit footprints far beyond physical capacity → thrash).With the disk tier off (
max_disk_bytes == 0) the threshold is exactlymemory × tolerance, so gate behaviour is unchanged when disk is disabled.Why
When the disk tier is on, evicted-from-RAM entries spill to the on-disk liquid tier (NVMe), so a scan up to
memory + diskfits without thrashing. The gate should admit those instead of bouncing them to the mount (where every read re-decodes parquet).How
admission_threshold(memory, disk, tolerance)function.should_bypass/should_bypass_guardednow takememory_budgetanddisk_budgetseparately; the per-decision admission log emitsmemory_bytes+disk_bytes+threshold_bytes.threshold_tests: 5 pure unit tests pinning the exact formula (memory×tol+disk, disk-off = mem×tol, disk-at-1×, zero-threshold, large-memory no-overflow). These run everywhere; the cache-based gate tests remain Linux-only (direct_io).Testing
cargo fmtclean; clippy clean.threshold_testspass locally (macOS).direct_iofor the t4 mount) → validated in CI.