chore: adopt upstream's history as our base - #51
Merged
Merged
Conversation
…fusion-contrib#509) Co-authored-by: Xiangpeng <xiangpeng@Xiangpengs-MacBook-Pro.local>
… warmup + tests) (datafusion-contrib#511) ### Motivation - The CI benchmark comparison showed large, order-dependent slowdowns because LiquidCache runs first and populates the OS page cache, making the baseline look artificially faster. - Single outlier warm iterations from CI pauses can make the mean-driven warm metric noisy and report false regressions. - Make minimal, targeted changes to the comparison and CI workflow so results reflect real performance differences. ### Description - Replace mean with median for warm-iteration metrics in `.github/compare_benchmarks.py` by importing `statistics` and updating `get_warm_metrics` to return median values. - Make the configured `--threshold` consistently used when highlighting regressions and computing the warm-time summary in `format_change_percentage` and the report generation. - Add a lightweight unit test file `.github/test_compare_benchmarks.py` that verifies median warm metrics and threshold-based highlighting. - Update CI in `.github/workflows/ci.yml` to build the release `in_process` binary once, run a small unmeasured DataFusion warmup to populate the filesystem page cache, and invoke `target/release/in_process` for measured runs to remove order-dependent bias. ### Testing - Ran unit tests `python3 .github/test_compare_benchmarks.py`, and they passed (2 tests, OK). - Linted and checked Python files with `ruff` and `python -m py_compile`, both passed. - Verified `cargo check -p liquid-cache-benchmarks` for the benchmark crate succeeded. - A full workspace `cargo check` encountered a pre-existing environment-specific failure due to a missing generated asset (`dev/dev-tools/assets/tailwind.css`) that is unrelated to these benchmark changes. ------ [Codex Task](https://chatgpt.com/codex/cloud/tasks/task_e_6a96f83694ec8332ae8d6fade89739ef)
…trib#512) ### Problem `LiquidStreamBuilder::build` gives `limit` and `offset` to the reader. `plan_row_group` then cuts the row selection to the first `limit + offset` physical rows **before** the pushed-down row filter runs. So the limit counts scanned rows, not matched rows. For example, a query like `SELECT id FROM t WHERE tag = 'MATCH' LIMIT 10` drops every match that comes after the first 10 physical rows of a file. If the matches are later in the file, the query returns 0 rows, but there are matches. Parquet counts the limit against post-filter matches. DataFusion depends on this when it pushes `fetch` into a scan whose filters were absorbed (`pushdown_filters = true`, which local mode sets). To reproduce on `main`: write a 20-row file where rows 15-19 have `tag = 'MATCH'`, then run `SELECT id FROM t WHERE tag = 'MATCH' LIMIT 10`. You get 0 rows. Expected: 5 rows. ### Fix Use `limit` and `offset` only when the scan has no row filter. In that case scanned rows equal emitted rows, so the cut is correct. Filtered scans still get a limit. DataFusion's `FileStream` cuts the emitted batches with `FileScanConfig::limit` after the filter. ### Tests New file `src/datafusion-local/src/tests/filter_limit.rs`. Every test puts the matches at the physical end of the data, so a cut-then-filter scan returns too few rows: * one row group, * a limit that spans row groups (this also touches row-group statistics pruning), * a scan over two files. Each test runs its queries twice, so both the cold (parquet) path and the warm (liquid cache) path are covered. All three tests fail on `main` and pass with this change. Co-authored-by: Eddie A Tejeda <669988+eddietejeda@users.noreply.github.com> Co-authored-by: Xiangpeng Hao <haoxiangpeng123@gmail.com>
switch to the new api
Doesn't have performance impact yet, just make it nicer to look at.
Found by codex audit, those are indeed correctness bugs
…nd keep lineage metadata (datafusion-contrib#516) we will still squeeze, but I'd rather rewrite than improve.
I think vortex is well maintained and this switch actually gets slightly performance gain. We'll add back some liquid cache secret sauce very soon
Latest data fusion already has metadata cache, so we don't need to do it again.
it was only working in local mode
Ports #1, #8 and #9 onto upstream's post-morsel optimizer. A parquet scan is routed through LiquidCache only when its estimated liquid footprint fits the cache; oversized scans stay on `ParquetSource` and read from the object store instead of evicting the working set to cache a scan that was never going to fit. The estimate is filter-aware and byte-accurate: per-file, the sum of the byte sizes of the columns the scan materializes (projection union predicate), over the files that survive the scan's own pruning predicate, deduped across the byte-range splits DataFusion makes of one file. A file with no stats, or a required column with an `Absent` size, falls back to the whole-file size, which over-counts in the safe direction. `Inexact` sizes are counted: they are real measurements, and rejecting them would make every scan against a catalog that labels them that way fall back to the whole file and bypass. The threshold is `memory × tolerance + disk`. Only the memory tier carries the overcommit, because LiquidCache compacts in RAM and keeps winning until roughly 5x over budget; the disk tier is counted at face value, since a scan that overflows RAM spills to it rather than thrashing and there is no evidence for extending the RAM crossover there. Computed in f64 so a budget near u64::MAX cannot wrap. The gate is a pure performance decision — admitting a scan or bypassing it returns identical rows — so estimation runs under a panic guard whose `strict` flag chooses between aborting the query (surface the bug) and caching normally (keep queries running). Every decision logs one line under `liquid_cache::admission` with the full breakdown; without it the gate is a black box and each tuning cycle costs a benchmark run. Off by default: `LocalModeOptimizer` and `LiquidCacheLocalBuilder` cache every scan unless `with_admission_gate` is called, so upstream behaviour is unchanged.
Ports the regression coverage from #20 and #40 without their fix: upstream now evaluates nested-column predicates rather than dropping them, so the rows come out right by a better route than our decline-the-scan bypass, which cost the scan its cache. Three cases upstream's own nested_filter.rs does not reach: a nested conjunct alongside a pushable one, a nested column reached through OR (one candidate for the whole predicate, so refusing it applies no filter at all), and a conjunct on a column absent from the file schema. Each is checked cold and warm, since the cached path evaluates predicates separately from the source path.
Ports #15 (issue #13). `current_batch_id` indexes stored cache chunks, and both the cache read and the parquet fallback turn that id back into rows by multiplying it by the cache batch size. The reader walked the selection in windows of `datafusion.execution.batch_size` instead, so whenever a caller set that to anything other than the cache's size, batch id N named one range of rows to the reader and a different one to everything that resolved it. The scan then either ran off the end of the row group, or — with a LIMIT stopping it first — returned rows from the wrong offsets and succeeded. The fully-cached path misaligned the same way: a selection mask sized by the session batch size was applied to a chunk sized by the cache's, and `arrow::compute::filter` only rejects a mask longer than its target, so a shorter one silently took the wrong rows. Window by the cached row group's batch size, read once at the one place it is used. The caller is unaffected: `DataSourceExec` re-splits every source stream to the session batch size, which is what makes ignoring it inside the reader safe. The invariant this replaces was a `debug_assert_eq!`, so release builds carried the misalignment silently rather than failing. Two tests here are `#[ignore]`d against a separate upstream defect they uncovered: page-index pruning plus a pushed-down predicate panics in `boolean_buffer_and_then`. It reproduces with the session and cache batch sizes equal, so it is not a batch-size bug and is not this commit's to fix.
Ports #41. The liquid read path has no notion of DataFusion's virtual columns. It carries the `TableSchema` across faithfully but never produces one, so a scan whose projection includes a virtual column reads back a batch that simply lacks it, and resolving the column then fails with an Arrow schema error naming only the file's own fields. A predicate over one fails the same way: the reader rewrites it against the logical and physical file schemas, neither of which holds the column. Decline the swap for such scans, leaving them on `ParquetSource`, which derives virtual columns from the parquet reader. The guard keys on the virtual columns the scan actually reads — projection unioned with the pushed-down filter — not on what the table declares, so a provider that puts a row-position column on every table keeps the cache for the queries that never touch it. No projection at all is the one broad case, and it is not a guess: the scan then reads the whole table schema, virtual columns included. Positional reads are what reaches here in practice. Delete filtering and row lineage both project a reader-produced physical row position, an absolute index into the file, and a plausible but shifted position would associate a delete with the wrong row. Declining is the sound answer until the liquid reader can generate positions itself.
Ports #48, and with it the index-slot half of #45. A cache key packs the file id into 16 bits, so the 65,537th distinct file a process registers aliases the first. Entries recorded nothing about where they came from and the read API took no expected identity, so an aliased lookup returned the other file's data: a panic when the column types differed, silently wrong rows when they matched. Upstream guards the narrowing with a `debug_assert!` only, so release builds truncate and carry on. Record the unnarrowed file id alongside each entry and compare it on every read. A mismatch reads as a miss, so the caller re-reads from its source and gets correct data, and it is counted. Store objects are keyed by entry id *and* identity, so a write still in flight for one owner cannot overwrite bytes a later owner's entry names. Lease file ids instead of assigning them permanently. A lease is held by the file handle and by every row group and column derived from it, and returns to a pool when the last holder drops, so the id space tracks the files being read rather than every file ever read. Entries deliberately hold no lease: an id reused while its old entries are resident leaves them unreachable rather than readable, which is what lets the release stay out of index removal, where it would deadlock against `reset`. Two kinds of write, because they differ: a caller storing its own data takes a contested key over, while maintenance rewriting an entry it read earlier — evict, hydrate, spill, flush — lands only while the key still holds the identity it read, and is dropped otherwise. Adopting whatever holds the key by then would relabel one file's data as another's, and the new owner would read those rows as a hit. Two departures from the fork's version, both forced by upstream's newer code: The pool hands out a fresh id while the key's 16-bit file field has room and recycles only once it is exhausted. Recycling eagerly is correct but costs the previous holder every entry it cached, and upstream's join lineage test caught it: the two sides of a join took the same id and overwrote each other's lineage while 65,000 ids sat unused. The index slot also carries #45's fix — the payload is taken out of the slot when the index gives an entry up, so crossbeam-epoch's deferred destruction reclaims an empty shell rather than holding multi-megabyte arrays past the budget that counts them. `leased_file_ids`, `file_ids_over_key_width` and `identity_mismatches` expose the three counters that say whether any of this is being hit.
Ports #14. Fork-local: this rides with our stack and is never offered upstream.
Found by review of the file-id commit; three defects, one cause. Scoping store keys by identity means an object stops being reachable through the index the moment its key changes hands — `release_disk` is driven by an index entry, and by then no index entry names it. Under the shared key this port replaced, the next write simply overwrote the same object, so none of this had to be handled and there was nothing to copy. Three paths stranded bytes and objects for the life of the process: A rewrite dropped as stale had already written to the store. `try_insert` refunded its memory and returned Ok, and the bytes stayed charged with no entry naming them. It now reports what it left behind and each caller — evict, flush, insert — deletes the object and releases the reservation. An `Owned` takeover replaced a disk-resident entry and dropped it. The index now hands the displaced entry back with the identity that held it, for the same treatment. Only across identities: a write under the identity that already held the key addresses the same object and its put overwrote it, so reclaiming there would delete the bytes just written — which the policy snapshots caught. `remove_disk_entry` took an identity to address the object but removed the index record unchecked, so a caller holding a stale identity deleted the current owner's record while deleting the old identity's object. That is the window fork #49 closed on the write path, left open on the removal path. Removal is identity-checked now, and re-checked after the tree removal rather than only before. Two tests cover the reclamation and the refused removal. Disabling the reclamation strands 968 bytes in the first of them.
Ports the code half of #20, which the sync dropped. `749b6ef` brought #20 and #40 across as tests only, on the finding that upstream had superseded both. That was right for #40 and wrong for #20: upstream refuses nested columns and columns outside the file schema through `try_pushdown_filters`, which is what those tests cover, but it still drops a conjunct that references no column at all. Nothing tested that case, so the gap looked closed. A literal `Boolean(NULL)` conjunct is exactly what expression simplification leaves behind: `NOT (s = s)` becomes `s IS NULL AND NULL`. `pushdown_columns` returns an empty column set for it, the `is_empty()` bail dropped it, and by then DataFusion has removed the `FilterExec` on the strength of the predicate being fully pushed down — so the scan is the only place it is applied and it applies a strictly weaker filter. `SELECT s FROM t WHERE NOT (s = s)` returned every row with a NULL `s` instead of none. Keep the conjunct. A column-free candidate now builds with an empty projection mask, and the cached path evaluates it against a batch that carries only the row count the selection implies — `RecordBatch` needs that explicitly, since no array is there to imply it. Found by runtimedb's query fuzzer as a ternary-partition violation: the union of `P`, `NOT P` and `P IS NULL` returned 14664 rows where the unfiltered scan returned 12000. The new tests cover both that identity and the direct `NOT (s = s)` case, cold and warm; both fail without this change. The defect is upstream's and predates the sync — our fork carried this fix, upstream still does not.
A read that materializes a disk entry replaces it with a memory one, and a caller overwriting an entry does the same. Neither puts anything in the store, so the object under `(entry id, identity)` and its share of `used_disk_bytes` survive a replacement that nothing else will ever release: `release_disk` is only reached from an index entry, and by then no index entry names it. `DiskResidue::displacing` treated every same-identity displacement as an overwrite of the same object. That holds only when the write is itself disk-resident — those are the paths that put bytes under that key first. A memory entry displacing a disk one strands the copy instead, so the next spill reserves the same byte count again for an object the put overwrites. Over a fixed working set the disk tally climbs with every read/spill round, and once it reaches the limit the tier evicts entries that are genuinely there. Deleting the copy rather than keeping it for the next spill to reuse: the spill path serializes and writes unconditionally, so reuse needs a record of which form of the entry is on disk, kept correct across overwrite, transcode and takeover. Releasing the reservation without deleting is worse than either — the object then outlives any entry that could ever name it, and the store grows with nothing counting it. The datafusion-local snapshots move because `disk_evictions` now counts these reclaims; no result, plan or IO count changes.
Our main had no shared history with upstream: it was re-rooted on 2026-08-28 during the DataFusion 55 adoption, so every sync since has been a hand-port rather than a merge, and git could not even compute a merge base. This commit carries the tree of sync/upstream-2026-09 — upstream datafusion-contrib/liquid-cache at 0033b15 with our patches re-applied on top — while taking the old main as a second parent, so nothing is dropped from history and upstream's commits become ancestors. A future sync is then 'git merge origin/main', not a week of re-porting. What rides on upstream's HEAD, each verified against upstream's current code rather than assumed to still apply: admission gate (#1/#8/#9), conjunct-pushdown coverage (#20/#40), cache-sized batch reads (#15), virtual-column bypass (#41), per-entry file identity with leased ids (#48, carrying #45's index-slot fix), disk reclamation for stranded objects, the column-free conjunct fix, and CODEOWNERS. Verified as no longer needed, with evidence: the page-index fix (the bug does not reproduce upstream), the store::mount wrapper (t4 0.1.9 bypasses the macOS page cache itself, so ours would disable it), and #5/#11/#37, which upstream already carries. Known gap, deliberately left for a follow-up: src/core/tests/memory_footprint.rs, our instrumented allocator harness from #45. It guards the exact class of defect this port kept hitting — the cache's reported accounting diverging from reality — and should be adapted to upstream's shape next.
anoop-narang
requested review from
shefeek-jinnah
and removed request for
a team
September 24, 2026 06:08
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Context note: the full diff was omitted from the review prompt. gh pr diff failed with HTTP 406 because the diff is over 20,000 lines. This review read these post-merge files from the checkout: src/core/src/cache/core.rs (lines 1-1462), src/core/src/cache/index.rs, src/core/src/cache/budget.rs, src/core/src/cache/builders.rs (Insert), src/core/src/cache/policies/eviction.rs, and src/datafusion/src/cache/file_id.rs. The review did not cover the remaining upstream-sourced files, the admission gate, the optimizer, or the reader changes. CI checks were still pending when the review started.
Blocking Issues
src/core/src/cache/core.rs:121:DiskResidue::displacingdrops a displaced same-identity disk entry without releasing itsdisk_bytes.insert_innerreaches this case when a batch does not fit in memory and the key is already on disk under the same identity.used_disk_bytesthen grows by the old size on every such overwrite, and nothing releases it.
Action Required
- Release the displaced entry's
disk_bytesin the in-place overwrite case. Do not remove the store object in that case. - Add a regression test: a memory budget smaller than one batch, and the same identity inserting the same key twice. Assert
charged_disk_bytes_match_the_index.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
Review found a third shape of the same defect. `DiskResidue::displacing` treated a same-identity disk write as needing no reclamation at all, on the grounds that its put landed on the very object the displaced entry named — true of the object, but not of the reservation. The superseded entry's bytes stayed counted, so one object could be charged twice. The three cases are now explicit rather than one filter: a different identity or a memory write leaves an object nothing can reach, so it is deleted and its bytes released; a same-identity disk write keeps the object it overwrote and gives back only the superseded reservation; a displaced memory entry has nothing to reclaim. Reachability: I could not construct it. Four attempts — shrinking the memory budget to 64 bytes and inserting over a disk-resident key — all ended with the entry transcoded to liquid and resident in memory, so `insert_inner` never took the spill path that would displace a disk entry with another disk entry. Every displacement I could actually produce on that path displaces a memory entry, where the old predicate was already correct. The fix is here because the case is real in the code and the correct behaviour is cheap, not because a test forced it.
Collaborator
Author
|
Fixed in
The three cases are now explicit instead of one filter:
On reachability, since you named a specific path. I could not construct it, and I tried four times: memory budget shrunk to 4096, then 256, then 64 bytes, inserting over a key already resident on disk. Every attempt ended with the new batch transcoded to liquid and resident in memory, so So the regression test I added pins the invariant ( Full suite: 249 tests, 0 failures, clippy and fmt clean. One note on your context caveat, which I think is worth carrying forward: you flagged that the diff exceeded 20,000 lines so you reviewed six core files and explicitly not the admission gate, optimizer or reader. That is the right call to surface. For this PR the tree is identical to |
There was a problem hiding this comment.
The prior blocking finding is resolved in 5450d04. DiskResidue::displacing now releases the superseded reservation for a same-identity disk overwrite and keeps the object. This re-review covers only the since-diff in src/core/src/cache/core.rs. It does not cover the rest of the 169-file merge. CI checks were still pending when this review started.
The reclamation work made `disk_evict` appear in the cache trace snapshots for the first time. dev-tools parses those snapshots and panics on any event it does not recognise, so `parse_all_snapshot_traces` failed — the event type has existed in the core's trace enum all along, it had simply never been emitted into a recorded trace before. Adds the variant, its parse arm, and the two match arms that were exhaustive. The simulator deliberately moves no I/O counter for it: the event frees disk space rather than reading or writing any. Found by CI, not locally: dev-tools does not build here without its Tailwind asset, so every local run of mine excluded it. Dropping a placeholder into dev/dev-tools/assets/ is enough to type-check and test the crate locally, which is how this was verified before pushing.
There was a problem hiding this comment.
Context disclosure: the full diff was not in the review prompt. This cycle covers the since-diff (dev/dev-tools/src/trace/parser.rs, dev/dev-tools/src/trace/simulator.rs) and the prior blocking fix in src/core/src/cache/core.rs (DiskResidue::displacing, settle). The admission gate, optimizer, and reader changes are not covered by this approval.
The in-place disk overwrite now releases the superseded reservation through DiskResidue.superseded in settle. The prior blocking finding is resolved. The two earlier nits (stray Insert doc line at core.rs:75, PROBE println at core.rs:1219-1224) remain open and are not blocking. CI checks were still pending when this review started.
Both from review. The doc line was left over from the Insert builder and had become the first line of DiskResidue's rustdoc, so the summary named the wrong type. The println and its kinds collection were debugging I left in while chasing whether the in-place overwrite path was reachable; nothing reads them.
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
3 participants
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.
Our
mainhas no shared history with upstream. It was re-rooted on 2026-08-28 during the DataFusion 55 adoption (ce81773is a root commit), sogit merge-base origin/main mainreturns nothing and every sync since has been a hand-port rather than a merge.This PR fixes that without rewriting history. It carries the tree of
sync/upstream-2026-09— upstreamdatafusion-contrib/liquid-cacheat0033b15with our patches re-applied — and takes the oldmainas a second parent. So:git merge origin/main,What is on it
Each of our patches was checked against upstream's current code rather than assumed to still apply:
Verified as no longer needed, with evidence rather than judgement: the page-index fix (the bug does not reproduce upstream — the policy is no longer enforced on that path), the
store::mountwrapper (t4 0.1.9 bypasses the macOS page cache itself, so ours would disable it), and #5/#11/#37 which upstream already carries.Verification
397 → 459 → 521 → 583 MB(+62 MB per round over a fixed working set) to flat at ~27 MB, cross-checked against the actual store file.Known gap
src/core/tests/memory_footprint.rs— our instrumented allocator harness from #45, which cross-checks the cache's reported accounting against bytes actually allocated. It guards exactly the class of defect this port kept hitting and is not yet adapted to upstream's shape. Follow-up, and the first thing I would land on this base.