[azimafroozeh]

A load csv overwrite captures its whole source into one buffer before the plan exists and cuts the slices on one thread, although the planner needs only the object's size and a partition only its own bytes: 34 s of capture and 15 s of serial cutting in SF30's 155 s load after #1147. The uniqueness check then merges eight per-partition hash tables on one thread after the last batch, 11 s on the Comment file.

The loader now plans from the size probe, and each parse partition of an overwrite range-scans its own byte range in 8 MiB pieces through conditional GETs with an If-Match precondition on the probe's ETag. Partitions align on record boundaries by bytes, each starting at the first line end past its range start and the one before it stopping at the first record end its parse meets past that byte; the guard is that the two offsets agree, and a mismatch, like any refusal of the partitioned run, falls back to the same plan as a single partition. The uniqueness check's first holders become one sharded hash table shared by every partition, pre-sized from a cardinality estimate off the first slice, so a repeat is judged at insert and nothing merges after the drain; append and merge keep the whole-object capture.

SF30, 31 overwrite statements on a fresh graph, release builds of 7711e4a3 and this branch, two interleaved rounds, medians: load 154.9 → 110.5 s, Comment 46.4 → 18.6 s, write 45.7 → 47.3 s.

For review: the decision to challenge is the single-partition fallback, which re-reads a refused source once more rather than numbering records across partitions; a refusal the fallback does not reproduce would make it wrong, and load_csv_partitioned_refusal_names_the_record.gqt and load_csv_partitioned_quoted_line_break_at_the_boundary.gqt pin the two shapes. Least sure: the alignment reads line ends without regard to quoting, so an empty line at a boundary costs a fallback, never wrong rows; no case pins it. The boundary cases cut partitions at 4 KiB through a test-only # csv_slice_bytes header, refused on a server target. An overwrite now reads its first record from the source's first 8 MiB and refuses a longer one (docs, Rust test); a memory refusal's actual is the first piece over the limit, not the whole source. The receipt's slices gain read_ns, additive, openapi.json regenerated; the DST cost golden's load csv scenarios move from one whole-object GET to the head-range GET plus one GET per piece; release note changelog.d/load-csv-partitioned-read.performance.md.

[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	⚠️ Failed 2026-10-25T14:56:24.571659Z We couldn’t complete this review. Comment @codex review to try again.	1fd560c	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]

Request changes.

Why

A load csv overwrite reads the whole source into one buffer, then cuts the slices on one thread. On an LDBC SF30 load that buffer and that cut are a large share of the time. The planner needs the object size. Each parse partition needs its own bytes.

What

The overwrite plans from the metadata probe. Each parse partition reads its own byte range in 8 MiB pieces. Each read is a conditional GET with If-Match on the probe ETag. Append and merge still capture the object once. The uniqueness check inserts into one sharded map as the rows arrive. The first record must end inside the head range. At the default slice size that range is 8 MiB. It is never smaller than 64 KiB.

The two decisions

Single-partition fallback. A partitioned refusal sets source_refused. The loader runs the same plan as one partition. Validate holds every batch until every partition finishes. The sink stages only after that, so a mismatch or a parse error does not publish. The rerun is the one place that numbers records, and the reported record is the source record. I accept this. A counter shared across partitions would be a second numbering owner. The cost is one more read of a source the load is going to refuse. load_csv_partitioned_refusal_names_the_record.gqt and load_csv_partitioned_quoted_line_break_at_the_boundary.gqt pin the two shapes the description names.

Boundary alignment. Partition 0 starts at the records. A later partition starts after the first raw \n or \r at or past its range start. The partition before it stops at the first csv_core record end at or past its range end. csv_core 0.1.13 skips empty lines: its own test parses \n\n\n\n as no records. The raw scan treats that \n as a line end. An empty line at the boundary makes the two offsets differ. The last successful partition compares them and refuses, and the fallback runs. I checked every byte boundary of four small files that contain blank lines. Where the offsets agree, the csv_core records are the records of the whole range. Where they differ, the guard sees the difference. A 4107-byte file with the blank line at byte 2058 (# csv_slice_bytes: 4096, two partitions, the next record SPLITROW) loads 128 rows, SPLITROW once, and the receipt shows 1 partition. The alignment does not publish wrong rows. The corpus does not pin the empty line. Please add that case. The comment on RangeScanner names only a quoted field.

Blocking

1. The DST storage-cost golden fails on this commit. dst_bench_cost_count_golden failed in run 40204896850. Five lines move, and crates/omnigraph-dst/cost_table.txt does not:

   • OverwriteRemovalScan l.get 18 to 19
   • OverwriteRemovalNone l.get 10 to 11
   • OverwriteRemovalNoneEmptyTarget l.get 10 to 11
   • EndpointProbeSwept l.get 15 to 16
   • EndpointProbeJudged l.get 13 to 14

   Each labeled load csv overwrite gained one l.get. A file smaller than one slice was one whole-object GET. It is now the header range GET plus the records range GET. The DST decorator wraps every store in Lance's object-store registry and counts get_opts as l.get (crates/omnigraph-dst/src/lance_faults.rs). load_csv_statement_probes_and_reads_its_source_once already expects 2 payload reads for overwrite and 1 for append and merge. Update those five lines, and say in this pull request why each one moved. The description says the golden moves. The file does not. No other golden line moved.

2. The operator pages still describe the old read. docs/user/queries/settings.md (query_memory_bytes) still says one load csv statement retains the source, captured once and reused on every reprepare. docs/user/mutations/index.md (Limits and conflicts) still says the loader reads the object once per statement, and that a stream claims the next 8 MiB slice as it finishes one. docs/user/queries/explain-load.md still says the partition claimed the slice. docs/dev/blob.md still says a load csv source is a bounded whole-object GET. Overwrite now probes, then each partition reads its range. The source memory is one piece per partition. A refusal reads the ranges again. Append and merge still capture once. CSV_SOURCE_READ_DEADLINE wraps each range read, so one stalled piece still fails at 120 seconds, and a large object is not capped at 120 seconds for the whole load. Please make those pages say this. The new sentence about the 8 MiB first record is accurate for the default slice.

Optional

1. Add a GQT case for a blank line on the partition boundary, on the shape above, and name empty lines in the RangeScanner comment beside quoted fields.
2. The SF30 figures (154.9 s to 110.5 s, Comment 46.4 s to 18.6 s) are this pull request's measurements. I did not reproduce them. CI does not run that benchmark.
3. # csv_slice_bytes is test-only. admit_served refuses it on a server target. The default slice is SOURCE_PARTITION_BYTES (8 MiB).

Tradeoff

This removes the whole-object buffer, the serial cut, and the post-drain merge of per-partition uniqueness maps. It adds range reads, a boundary guard, a one-shot fallback, and one shared sharded map sized from the first slice. The fallback keeps one numbering owner. That is the right trade on a path that runs only when the load fails. SharedFirstHolders is the right owner for the earliest repeat: a key's two earliest positions meet whichever partition inserts first, and the reported repeat is the one with the smaller later position. load_csv_partitioned_unique_repeat_names_the_earliest.gqt pins that the refusal names KB, the earlier repeat, across the boundary with KA.

If-Match is the probe ETag on every range (PreflightedExternalBlob::read_range_pinned). object_store 0.14.2, which lance-io 13.0.0 uses, sends that header on S3, Azure, and GCP (HttpRequestBuilder::with_get_options) and checks it on the local store (GetOptions::check_preconditions). On a ranged response, meta.size is the full object size (get_range_meta copies it from Content-Range). The read refuses the load when that size or the ETag differs from the probe. A later object_store that stores the range length in meta.size would refuse every overwrite as a changed object. The pin holds that behavior today.

Open pull requests: #1150 and #1130 both edit openapi.json, and #1150 also edits table_store.rs. read_ns is additive and decodes as 0 when absent. I see no design conflict. #996 is a draft that prices request waves on query plans. This change is the load path. Parallel range reads are the shape that draft describes for object storage. The load planner still chooses the partition count as source bytes divided by 8 MiB, capped by stage_write_concurrency.

Validation

Reviewed head 799899984fca9b488d1bffb48431777da5872d0d. Merge base acb1f8c3.

Local, isolated checkout of that head:

• CARGO_TARGET_DIR=/workspace/target cargo test -p omnigraph-gqt --locked --test gq_logic_tests load_csv_partitioned — 4 passed in 0.67 s. The three committed cases passed. The fourth was a temporary empty-line case, not in this pull request. I removed it and restored the scanner file.
• Temporary RangeScanner check, also removed: at boundary 17 of key,score\n0,1.5\n\n10,1.5\n... the previous partition ends at byte 24 (cuts 0,1.5\n and \n10,1.5\n, and the second cut counts as one record) and the next partition starts at byte 17. Where the offsets are equal, the csv_core records match the whole range.

CI on this commit: GQT (ordinary) passed, so the three committed cases passed there. Test Workspace passed, which runs load_csv_refuses_a_source_replaced_between_probe_and_read, load_csv_overwrite_refuses_a_first_record_longer_than_the_head_range, and load_csv_statement_probes_and_reads_its_source_once. DST pinned suite failed, as above. I did not re-run the DST suite locally.

The three GQT files do not execute on acb1f8c3. That runner's header keys are issue, red_on, notes, and traversal, so # csv_slice_bytes fails as an unknown header before any load. Main captures the object and cuts it with csv_core, so a blank line does not split two partitions. The disagreement is new on this branch.

I did not reproduce the SF30 timings.

Inline comments

changelog.d/load-csv-partitioned-read.performance.md:5

The DST golden does not match this commit. dst_bench_cost_count_golden failed in run 40204896850. These l.get counts move, and crates/omnigraph-dst/cost_table.txt still has the old numbers: OverwriteRemovalScan 18 to 19, OverwriteRemovalNone 10 to 11, OverwriteRemovalNoneEmptyTarget 10 to 11, EndpointProbeSwept 15 to 16, EndpointProbeJudged 13 to 14. A small overwrite is now the header range GET plus the records GET, and the registry decorator counts both as l.get. Please update those five lines and explain them here. No other golden line moved.

docs/user/mutations/index.md:168

This sentence is right for the default slice. The Limits section below still says the loader reads the object once per statement and reuses that capture on every reprepare, and that a stream claims the next 8 MiB slice. Overwrite probes once, then each partition reads its range. A refusal reads the ranges again. Append and merge still capture once. query_memory_bytes in docs/user/queries/settings.md and the whole-object GET sentence in docs/dev/blob.md say the same old contract. Please update them in this change. The 120-second deadline now wraps each range read, not the whole object.

docs/user/queries/explain-load.md:87

A partition no longer claims the next slice. Each partition owns a byte range and reads that range in pieces. partition on a slice is the range that held those records. The same wording is in the Limits section of docs/user/mutations/index.md.

crates/omnigraph/src/loader/csv.rs:538

Empty lines disagree too. csv_core 0.1.13 skips them (\n\n\n\n is no records). align treats the raw \n as a line end, so the next partition can start before the previous partition's record end. The guard catches that and the fallback loads the right rows: a temporary case with the blank line at byte 2058 (csv_slice_bytes 4096) passed here with 1 partition, 128 rows, and SPLITROW once. Please add that case. The comment should name empty lines beside the quoted field.

[cursor]

Request changes.

Delta

Head 90e7094286bf5ce1f93d1515ea36af0e5fdc8776 adds one commit, fix, on 799899984fca9b488d1bffb48431777da5872d0d. The diff is crates/omnigraph-dst/cost_table.txt only. Five l.get lines each gain one call. No other line in that file moves.

Blocking

1. Resolved. The DST storage-cost golden now matches a live run. The five labels are the five load csv overwrite scenarios in overwrite_removal_cost_rows and endpoint_probe_cost_rows. Each source is a few dozen bytes, so the plan has one partition. The probe is a HEAD. The decorator counts get_opts as l.get and does not count HEAD. The overwrite then issues two get_opts calls on the file store:

   • the header read 0..head_end in Parse::resolved (crates/omnigraph/src/engine/operators/csv_source.rs). head_end is min(size, max(slice, 64 KiB)). For these files that range is the whole object.
   • the records read from records_start through one RangeScanner::fetch. The file is smaller than the 8 MiB slice (SOURCE_PARTITION_BYTES), so that read is one piece.

   A whole-object capture was one get_opts. The records read is the extra call. l.list and l.put on these labels stay put. The JSONL labels OverwriteWriter128KiB and OverwriteWriter32MiB stay put. The pull request text already states this move: one whole-object GET becomes the head-range GET plus one GET per piece. These five files are one piece. The golden records that fact. It does not hide a second change.

2. Open. The operator pages still describe one capture of the whole object.

   • docs/user/queries/settings.md:80 (query_memory_bytes) still says the retained source bytes are captured once per statement and reused by every reprepare attempt.
   • docs/user/mutations/index.md:275 still says the loader reads the object once per statement and every reprepare attempt reuses those bytes.
   • docs/user/mutations/index.md:318 still says a stream claims the next 8 MiB slice as it finishes one.
   • docs/user/queries/explain-load.md:87 still says the partition that claimed the slice. The same page now names read_ns. The claim wording is the old contract. docs/user/queries/explain-load.md:92 still says "the claiming partition".
   • docs/dev/blob.md:51 still says a load csv source is a bounded whole-object GET, and that the complete response is checked while it streams.

   An overwrite probes, then each partition reads its own range in 8 MiB pieces. The source memory is one piece per partition. A refusal reads the ranges again. Append and merge still capture once. CSV_SOURCE_READ_DEADLINE (120 seconds) wraps each range read in source_read_deadline. The sentence at docs/user/mutations/index.md:167 is right for the default slice: the first record is read from the first 8 MiB, and the head range is never smaller than 64 KiB (CSV_HEAD_MIN_BYTES).

Optional

1. Open. No GQT case puts a blank line on a partition boundary. The RangeScanner comment (crates/omnigraph/src/loader/csv.rs:533) still names only a quoted field. csv_core 0.1.13 skips empty lines. align treats a raw newline as a line end. The guard falls back when those offsets differ. Please add the case and name empty lines in that comment.
2. Open. The SF30 figures in the pull request (154.9 s to 110.5 s, Comment 46.4 s to 18.6 s) remain this pull request's measurements. I did not reproduce them. CI does not run that benchmark.
3. Unchanged. # csv_slice_bytes is test-only. This is not a request.

Other checks on this head

DST pinned suite passed, including dst_bench_cost_count_golden (run 40218010994, job 121243297473). Test Workspace passed. GQT (ordinary) passed.

GQT (dst) failed in run 40218010958, job 121243301456. The failures are replay_mismatch on load_csv_integer_endpoint_memory.gqt (seed 0) and write_max_bytes_removed_id_charge.gqt (seed 42). GQ Logic Tests failed because that job failed. GQT (served) was cancelled. The same GQT (dst) job passed on 79989998 (run 40204896872). This commit does not change the engine or those cases. I did not rerun them. I do not add a finding for them.

This commit does not touch files the other open pull requests edit.

Tradeoff

The golden update records the extra GET and removes the false red on the cost contract. It adds no mechanism. The published operator text still states the old read, so the guides and the code disagree. That is the remaining liability.

Validation

Reviewed head 90e7094286bf5ce1f93d1515ea36af0e5fdc8776. Merge base acb1f8c3f5d6034577a55146194881128300b932.

Local, isolated checkout of that head, from crates/omnigraph-dst:

CARGO_TARGET_DIR=/workspace/target cargo test --locked --test scenarios dst_bench_cost_count_golden -- --exact

Result: test dst_bench_cost_count_golden ... ok, 1 passed, 7.60 s. The test replays the table twice, requires the two passes to match, and then compares the table with cost_table.txt. DST_REGEN_COSTS was unset.

I did not reproduce the SF30 timings. I did not rerun the two GQT (dst) mismatches.