[azimafroozeh]

A csv overwrite's validate sweep sizes its uniqueness check's holder tables from the source's row estimate on the first batch that has one, and the source publishes that estimate after the batch is sent: a sweep that reads first finds none, the tables stay unsized, and the query_memory_bytes refusal that follows reports their bytes less, 456 here. The GQT (dst) replay of write_max_bytes_removed_id_charge.gqt seed 0 saw both orders on main after #1158, actual 398742 against 399198, every assertion green in both attempts; #1158's seed-42 fix ordered the source's admission before its first poll and left this read after the send.

The estimate now has its own sample, the first parsed slice's row count, published once before that slice's batches are sent, so the channel's send is the happens-before edge from the sample to every batch a consumer can hold. The LoadSliceExecuted record stays after the send, since its end_ns is the moment the slice's last batch left.

the_row_estimate_precedes_the_first_batch takes a partition's first batch from the stream and asserts the estimate is set at that point; without the change the assertion is the race itself, and a 20 ms gap forced between the send and the record's push in a probe build reproduces CI's attempt 0 to the byte, actual 398742.

For review: the decision to challenge is the sample's source, the first slice any partition parses, where the record's first() was the first slice any partition finished; with several partitions either is whichever partition gets there first, so a partitioned source whose first slices differ in rows is the input that gives two table sizes, unchanged by this diff. Least sure is that no local schedule ever produced the lost order, so a green GQT (dst) on this branch is the usual order, not the proof. Nothing in the diff reads or writes storage, so the cost golden is unchanged; the estimate's value is unchanged, so no release note.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	🔄 Running since 2026-10-25T22:49:42.116033Z	4a7781a	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[ragnorc]

Recommendation

Request changes. The publication order is the right fix. The regression stays green on the old order, so this commit does not lock the bug. This head is already merged. The follow-up is the expectation. The order in this commit can stay.

Why

A CSV overwrite sizes its uniqueness holder tables from the source row estimate. The sweep reads that estimate after it holds a batch. Before this change the source stored the sample after send returned. spawn_blocking polls the producer on another thread. Tokio 1.53.3 Chan::send pushes the batch and then wakes the receiver, before send returns. The sweep can read None, leave the tables unsized, and report 456 fewer bytes. CI saw actual 398742 and actual 399198 on write_max_bytes_removed_id_charge.gqt seed 0. Both strings contain query_memory_bytes, so the case stayed green. The DST job fails only when the two replays differ.

What this commit does

first_slice_rows is set on the producer thread before that slice's batches are sent. rows_estimate reads that slot. The slice log still records end_ns after the send.

Why the order holds

One caller reads the estimate: size_unique_holders, after input.next() returns a batch. Every batch send in CsvSourceExec::execute is preceded by OnceLock::set on the same thread. A losing set still synchronizes with the winning sample. The channel push is a Release and the receive is an Acquire, so a read after next() sees Some. This file is one partition. The source is far under the 8 MiB partition size, so the sample value is fixed too. I checked the channel in the pinned tokio 1.53.3. The change does not read or write Lance.

Blocking

The new test and the .gqt case both stay green on the old order.

I moved first_slice_rows.set to after send and ran the_row_estimate_precedes_the_first_batch 100 times. All 100 passed. A 20 ms sleep between that send and the set fails the assertion. The same sleep after a set-before-send stays green for 100 iterations. The producer finishes a ready send and the following set in one poll, before a parked receiver runs. flavor = "current_thread" does not remove the second thread. blocking_owned polls the body on spawn_blocking.

cases/write_max_bytes_removed_id_charge.gqt expects the substring query_memory_bytes. At head 4a7781aa, the ordinary engine and DST seeds 0 and 42 (each seed replayed twice) all reported resource limit exceeded for query_memory_bytes: actual 399198, limit 65536. Both seeds printed replay matched. With the sample published after send and a 20 ms gap, those same five runs all reported actual 398742 and the case still passed. The 456 byte gap is one unsized holder table: 8 buckets times (size_of::<(Vec<String>, (String, u64))>() + 1) = 8 * 57. size_of on this host is 56.

Change the step to expect actual 399198, limit 65536. That string is stable on this head. The lost order reports actual 398742 and the case would fail.

Optional

The sample is still the first slice any partition parses. Partitions whose first slices differ in row count can still size the tables to different powers of two, once a shard's share reaches 8 rows. This case has one partition, so that split does not apply here. A multi-partition overwrite near a bucket boundary can still make actual differ across DST replays. The seed orders the current-thread runtime. spawn_blocking stays on OS threads. Closing that needs one chosen partition, and a rule that no batch is visible before that partition's sample is set. Keep that out of this fix.

Liability

The fix removes a schedule-dependent refusal for a one-partition overwrite. It adds one OnceLock beside the slice log, because end_ns has to stay the time the last batch left. A later edit can push the log and the slot at different times, and the two can name different slices. There is no new API and no storage change. Five more fixes of this shape stay one slot per estimate.

Validation

Reviewed head: 4a7781aad90bbad79feaf053c23d44327dbe72d9. Merge base: 2af30259df186c266acf0b67cd0366787e666290.

The old-order probes kept the new OnceLock and moved set to after send. That is the old visibility point. It is not a checkout of 2af30259. For this one-row file the sample is 0 either way. Some versus None is the 456 byte charge.

• cargo test -p omnigraph-engine --lib engine::operators::csv_source::tests::the_row_estimate_precedes_the_first_batch -- --exact at head: pass.
• Same test, set after send, 100 iterations: pass in 0.03s.
• Same test, 20 ms sleep between send and set: fail, estimate missing after the first batch.
• Same test, set before send, 20 ms sleep after send, 100 iterations: pass.
• cargo test -p omnigraph-gqt --test gq_logic_tests case::write_max_bytes_removed_id_charge.gqt -- --exact from crates/omnigraph-gqt at head: pass in 2.46s. DST seeds 0 and 42 replay matched. Every attempt: actual 399198, limit 65536.
• Same case, set after send, 20 ms gap: pass in 2.80s. Replays matched. Every attempt: actual 398742, limit 65536.

CI GQT (dst) and Test Workspace passed on this head. That is one green pair of replays. It is the common order.

Temporary edits were restored. Nothing was pushed.

Limitations

I did not see the lost order under the DST scheduler without the sleep. Local runs of the old order, with no sleep, took the sized path. I did not run a multi-partition source whose first slices differ in row count.

Inline comment for crates/omnigraph/src/engine/operators/csv_source.rs line 639:

This assertion stays green when first_slice_rows.set runs after send. I ran that order 100 times and every iteration passed. A 20 ms sleep between send and set makes this assertion fail. Expect actual 399198, limit 65536 on the query_memory_bytes step in write_max_bytes_removed_id_charge.gqt. The lost order reports actual 398742, and the current substring expect accepts both.