Repository navigation
fix(load): admit a csv overwrite's memory once, one share per partition - #1187
Merged
azimafroozeh merged 4 commits intoOct 28, 2026
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Closes #1160. A csv overwrite's parse partitions charge one shared pool as they decode, so which charge crosses
query_memory_bytesand theactualthe refusal reports depend on the order the partition threads reached the pool: the same case and seed reportedactual: 26677on one machine and26285on a loaded runner, which the DST can only call a replay mismatch, and after #1159 pinned one pair of charges into a fixed order and #1182 admitted the key sets before any partition starts, a busier runner finds the next pair.ValidateExecnow admits the load's memory once, after the key sets are charged and before any partition is polled: the room the limit leaves past the pool's total is carved into one share per parse partition and one for the unique holders, and every charge of a partition (its source pieces, its decoding, each decoded batch, the copy of that batch the fragment sink's chunker will hold, and the batch's validation facts, the last two charged once the slice's decode scratch is released) is made by the partition's own reader in source order against its share, so a refusal and itsactual, the smallest limit whose share would have admitted the charge, follow that thread's own sequence and never another's progress; the fragment sink's own holds, lease metadata and the registry's growth are counted on the pool and never refused. The guard is the admission floor: a share under one partition's source admission plus one slice refuses at the carve, before anything is read, and a refused run of several partitions reruns as one partition holding the whole partition share, that run's refusal the one reported.Two operator tests pin the mechanism: a two-partition overwrite charges the same consumers the same bytes on two runs and nothing outside the shared consumer set charges between the carve and the drain's end, and a share under the admission floor refuses with the smallest admitting limit before any partition is polled; the admission
.gqtcase pins that refusal and the load a wider limit admits, andload_csv_keyed_overwrite_admits_at_the_smallest_limitpins the contract at the edge, a one-partition keyed overwrite refused at 72,524 withactual 72525and loaded at 72,525, on the local engine and two DST seeds, where the same load admitted at 59,462 before this change with a figure that moved between runs. At default settings a keyed one-column overwrite loads to 256,000 rows and refuses at 512,000, where the base loads 512,000; the smallest admitting limit for 256,000 rows is 97.4 MB against the base's 70.8 MB. The SF30 overwrite suite, under a limit no share fills, loads in 107.85 s on this branch against 108.96 s on the base over two interleaved rounds whose run walls overlap, so the share charge, one atomic per charge, is off the hot path.For review: the decision to challenge is the accepted cost, that a keyed overwrite near the limit needs more
query_memory_bytesthan before: a one-partition share is 60 of 64 parts of the room by design, and the measured excess is larger, 1.22× at 30 rows and 1.29× per row at 128,000 to 256,000 rows, the rest being what the share keeps charged at the decode peak (the slice's decoded batches and their copies under the decode transient) where the pool's total at that instant was lower; the release note says the threshold moved and the settings guide says why. The input that exposes the fixed fractions is a many-partition overwrite with one partition's rows far wider than the rest, which cannot borrow another's unused room. Least sure is the uncharged part: the fragment sink's open chunks, a share's lease metadata and the registry's growth are counted on the pool but never refused, so a load can end past the limit by those bytes, within the four uncharged 64ths on the corpus's cases and past them at a limit of tens of KiB with many batches. The storage-cost golden is unchanged, since the shares decide when a load refuses and not what it reads or writes; the execution guide states the carve, the settings and mutations guides the contract, andchangelog.d/load-memory-admitted-once.changed.mdthe behaviour change: a refusal now comes before anything is read,actualis the smallest admitting limit where it was the pool's total at the refused grow, and a keyed overwrite near the limit may need a higher one.