chore: sync upstream, and stop CLAUDE.md going stale - #54
Merged
Merged
Conversation
…-contrib#521) Some queries return rows that do not match their `WHERE` clause. ```sql SELECT s FROM t WHERE NOT (s = s) ``` `s = s` is NULL wherever `s` is NULL, so `NOT (s = s)` is never true and this should return nothing. On `main` it returns every row whose `s` is NULL. ## Why `build_row_filter` breaks a conjunction into separate predicates (`split_conjunction`), then keeps only the conjuncts that reference a column: ```rust if required_indices_into_file_schema.is_empty() { return Ok(None); // conjunct dropped } ``` Simplification produces conjuncts that reference no column. `NOT (s = s)` becomes `s IS NULL AND NULL` — a column conjunct, and a literal `Boolean(NULL)` that reads nothing. The literal hit that branch and was dropped. Dropping it would be harmless if something else still applied it, but nothing does: DataFusion removes the `FilterExec` once it believes the predicate is fully pushed down, so the scan is the only place the predicate is applied. A dropped conjunct makes the filter strictly weaker than the one the user wrote, and rows that cannot match come back. ## The fix Keep the conjunct. A column-free candidate builds with an empty projection mask, and the cached path evaluates it against a batch carrying only the row count the selection implies — `RecordBatch` needs that stated explicitly, since there is no array present to imply it. ## Reproducing By hand: write a parquet file with a nullable `Utf8` column `s` where some rows are NULL, register it through `LiquidCacheLocalBuilder`, and run `SELECT s FROM t1 WHERE NOT (s = s)`. Expect no rows; on `main` every NULL-`s` row comes back. With the tests in this PR applied to `main` and the `row_filter.rs` change reverted: ``` $ cargo test -p liquid-cache-datafusion-local constant_conjunct test constant_null_conjunct_is_still_applied ... FAILED test three_way_partition_reconstructs_the_scan ... FAILED ---- constant_null_conjunct_is_still_applied ---- left: ["<null>", "<null>", ...] # 4000 rows returned right: [] # none expected ---- three_way_partition_reconstructs_the_scan ---- left: 14664 # rows across the three partitions right: 12000 # rows in the table test result: FAILED. 0 passed; 2 failed ``` ## The tests `src/datafusion-local/src/tests/constant_conjunct.rs`, both cold and warm cache: - **`constant_null_conjunct_is_still_applied`** — the direct case above. The fixture is 12000 rows with every third `s` NULL, giving the 4000. - **`three_way_partition_reconstructs_the_scan`** — the general property, and the more useful regression guard: for any predicate `P`, `WHERE P`, `WHERE NOT P` and `WHERE P IS NULL` must together return each row exactly once. Here rows whose `P` was NULL came back from both `WHERE NOT P` and `WHERE P IS NULL`. Found by a randomized query fuzzer using that three-way partition as its oracle.
…ache into sync/upstream-2026-09-28 # Conflicts: # src/datafusion-local/src/tests/mod.rs
The file pinned upstream at a commit id. That was true the hour it was written and wrong three days later when our own PR merged upstream, and it would have gone on rotting on every upstream merge. A file whose job is telling agents what is true cannot hold facts that expire. It now says `main` contains upstream's history in full, which stays true, and gives the commands that answer where either side actually is. It also said "the remote is `origin` in a fresh clone", which reads as "origin is ours". In this working copy `origin` is *upstream* and the fork is a second remote named `fork` — the reverse. A session following this file wrote `git merge origin/main` meaning merge upstream, correctly for this clone, and it read as merging our own main into itself. There is now a Remotes section saying to run `git remote -v` first and warning that the names are not what you would guess, and every command names repositories by URL. Adds a Syncing section, because the omission is what sent that session looking. It says to merge upstream into a branch and raise a PR rather than using GitHub's "Sync fork" button, which offers to discard our commits when the merge is not a fast-forward, and it describes the conflict our own upstreamed changes cause on the way back: the content matches but the commit does not, so both sides appear to have edited the same lines. Resolve by keeping ours, and sync promptly, because alone that conflict is obvious and bundled with real upstream work it is not. Two more expiring claims removed. The shuttle note counted the failures it expects; it now says which tests fail and why. The toolchain note deferred to a `rust-toolchain.toml` that does not exist in this repo -- written from what such a repo usually has rather than from this one -- and now says there is none, which is the reason the pin is needed.
anoop-narang
requested review from
rohan-hotdata
and removed request for
a team
September 28, 2026 04:24
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…urs" Review found the Syncing section walking into the trap the Branches section warns about. Its only fetch named upstream, so local `main` was never refreshed, and the sync branch was cut from it — a stale one makes the sync PR revert fork commits merged since, with nothing to say so. It now fetches this fork first and branches from what came back, and uses each `FETCH_HEAD` immediately, since it holds only the last fetch. My own sync branch escaped this only because I had refreshed local `main` an hour earlier for unrelated reasons. "Resolve by keeping ours" was too broad. It is right for the returned squash of our own change, but a conflict hunk can hold an unrelated upstream edit on the same lines — a module appended next to the returned one — and taking the whole hunk from our side drops it silently. Now: keep ours for the returned change, keep any other upstream edit in the same hunk, and read the hunk rather than resolving by rule. Also records that a sync PR must be merged and not squashed. A squash gives the result one parent, so upstream's history never enters `main`'s ancestry: the merge-base does not move and the same upstream commits stay missing, with no error. Simulated both onto `main` — squashed, the merge-base stayed at the old upstream commit and the sync was undone; merged, it advanced to upstream's tip. The section now says to verify `git merge-base main FETCH_HEAD` afterwards.
Review found the verify line checking neither side of what it claimed. `git merge-base main FETCH_HEAD` reads local `main`, which does not contain a merge made on GitHub, and `FETCH_HEAD` is whatever was fetched last — fetch the fork to refresh `main` and it compares the fork against itself. Run as written it returns our own tip and looks like a pass. It now captures upstream's tip before fetching the fork over it, and tests the property directly. Run against the current state, with this PR unmerged, it correctly reports not-ok; the old line reported our own main and told the reader nothing. Third time in this file that `FETCH_HEAD` holding only the last fetch has produced a wrong instruction. The sync recipe above now says so where the fetches are, rather than leaving each site to remember it.
📊 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.
Important
Merge this — do not squash it. A squash gives the result one parent, so upstream's history never enters
main's ancestry. Simulated both ontomain:Squashed, this PR silently undoes itself and nothing reports it. PR #51, the adopt merge that gave us upstream ancestry, was a 2-parent merge for the same reason.
Two fixes for problems this file caused. Another session followed
CLAUDE.md, wrote correct commands, and they read as nonsense.CLAUDE.mdstated facts that expireIt pinned upstream at a commit id. True the hour it was written; wrong three days later when our own PR merged upstream, and worse on every upstream merge after that. A file whose job is telling agents what is true cannot hold facts that rot. It now states what stays true —
maincontains upstream's history in full — and gives the commands that answer where either side actually is.Two more removed: the shuttle note counted the failures it expects, and the toolchain note deferred to a
rust-toolchain.tomlthat does not exist in this repo. I wrote that from what such a repo usually has rather than from this one.It got the remotes backwards
It said "the remote is
originin a fresh clone", which reads as origin is ours. In this working copy:A session wrote
git merge origin/mainmeaning merge upstream, correct for this clone, and through the file it read as merging our own main into itself.There is now a Remotes section saying to run
git remote -vfirst, warning the names are not what you would guess, and every command names repositories by URL instead.No Syncing section, which is what sent that session guessing
Added. Merge upstream into a branch and raise a PR; do not use GitHub's "Sync fork" button, which offers to discard our commits when the merge is not a fast-forward. It also describes the conflict our own upstreamed changes cause coming back — the content matches but the commit does not, so both sides look like they edited the same lines — and says to sync promptly, because alone that conflict is obvious and bundled with real upstream work it is not.
The sync itself
Merging
5237fdd(upstream's squash of our datafusion-contrib#521).Zero files change because upstream's only new commit is our own fix, which we already carry. The merge buys ancestry, not code — and retires the duplicate-change conflict now, while it is a one-line module list, rather than later bundled with real upstream work.
The conflict was exactly that: ours has
mod batch_size_alignment;, upstream has nothing there. Kept ours. Verified afterwards that every declared module has a file and that therow_filter.rsbail did not come back with the merge.