Repository navigation
🐛 Fix: the temporal clone e2e follows the async copy it exercises - #203
Draft
medazizktata25 wants to merge 2 commits into
Draft
medazizktata25 wants to merge 2 commits into
medazizktata25 wants to merge 2 commits into
Conversation
…ises
A query over an uncovered temporal window federates to the source and
enqueues a background copy of the window; the worker's copy skips the
clone_stats write by design, so rows_fetched never moves. The test still
expected the synchronous path it was written for and failed on every
run that could reach it ("temporal window hydrated (t0=0 t1=0)").
It now checks what the path does: the first query federates once and
matches the source, the queued copy lands (the table is whole-cached or
holds a cached range, with nothing left in gfs.copy_queue), and the
repeat query matches the source without federating again.
Checked against an extension image whose worker never copies: the test
fails with "the background copy of the window never landed".
The background worker's copies (temporal windows, whole-table and predicate copies) skip clone_stats, so rows_fetched never includes them. It counts what a backend copies for its own query, plus what gfs.warm copies. Say so where the column is defined, and where tpch_validate.sh reads it as the number of rows pulled.
medazizktata25
requested a deployment
to
pr-validation
October 27, 2026 15:02 — with
GitHub Actions
Waiting
medazizktata25
requested a deployment
to
pr-validation
October 27, 2026 15:02 — with
GitHub Actions
Waiting
medazizktata25
requested a deployment
to
pr-validation
October 27, 2026 15:02 — with
GitHub Actions
Waiting
This branch is waiting to be deployed
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
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.
Related Issue
None filed.
What
e2e_clone_robustness::clone_range_and_temporal_hydration_elidefails on every run that can reach it:The test is out of date, not the extension. Its temporal half was written for a synchronous fetch: it expects the query on a date window to raise
rows_fetched. The commit after it (f7a4d83) changed that path, and the test was never updated:timecopy of the window;gfs.clone_stats(DEFER_BOOKKEEPING), sorows_fetchedcannot move.The data is right: the query returns exactly the source's rows. CI did not catch this because it runs none of this file's tests (they need the
gfs-postgresimage).How
test(clone): the temporal-window case follows the async copy it exercises. The temporal half now checks what the path does:federate_calls+1) and returns exactly the source's rows;gfs.copy_queuefor the table, and the table is whole-cached or holds a cached range (waits up to 60 s);docs(clone): rows_fetched does not count the background worker's copies. A comment whereclone_stats.rows_fetchedis defined, saying what it counts: rows a backend copies for its own query, plusgfs.warm's, and not the worker's. A matching note intpch_validate.sh, whosepulled()reads that column as rows pulled.Review Guide
crates/applications/cli/tests/e2e_clone_robustness.rs: the temporal half ofclone_range_and_temporal_hydration_elide.crates/extensions/gfs/src/sql/schema.sqlandtpch_validate.sh: comments only.Testing
On Rust 1.93.1, macOS with Colima for Docker, and a
gfs-postgres:16image built from this tree.e2e_clone_robustness: 5/5 pass, and they ran (not the early return for a missing image).cargo fmt --checkclean.--comments and a shell comment, so the installed catalog is unchanged; the image was not rebuilt for it.Documentation
rows_fetchedcomment inschema.sql, and the note intpch_validate.sh.User Impact
None at runtime: one test and two comments.
Out of scope, found while checking this: the copy worker still shares locks with foreground queries, and a user query on a table being copied can fail with
deadlock detected. I reproduced it; it will be reported separately.gfs.take_token()takesgfs.budgetFOR UPDATEbefore its unlimited early return, and the worker holds that for the whole copy.clone_sourcerow, which a concurrent query holds frombump_access.Checklist
cargo fmt --check)