Repository navigation
Conversation
Every keyed write checked one 32 MiB ceiling on a batch's whole Arrow memory, which includes a managed Blob value's bytes, so a value of exactly 32 MiB never fit beside its row. KeyedBytes splits that count into payload (each managed value's logical length) and framing (the rest), each under its own 32 MiB ceiling, and every keyed check uses it: staging, keyed-write preparation, the update carry scan, branch merge buffering and proven-insert chunking, and the compatibility loader's pre-decode forecast. The halves sum to the old count. A payload refusal names its resource with 'Blob payload bytes' in place of 'bytes'. Two Lance guards pin what the Blob write plan relies on: a whole-row merge-update keeps the row's stable row id, and merge-insert refuses an external reference outside the dataset's bases.
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes for the loader forecast in the inline comment. Reviewed 2c305ca41de66be39be4d3a2e648f74c109b2ee6. I use COMMENT because the authenticated account is the PR author.
A 32 MiB Blob could never fit under the old combined limit: its surrounding row also used bytes. This PR separates managed payload from framing, with a 32 MiB limit for each. The shared accounting type serves writes, carried update cells, external copies, and merge chunks. It makes the inclusive payload limit reachable through embedded mutations and compatibility loads. HTTP body and strict NDJSON line limits still apply first.
The remaining defect is a false refusal. A 24 MiB Blob beside a String containing base64: plus 12 MiB of A succeeds through mutation. Merge-load rejects the same values as 33 MiB of Blob payload. The loader must use the declared property type, as the Arrow check does. This leaves part of the new admission contract unsatisfied. The old combined limit also refused this row.
The core design addresses the cause and reduces liability. KeyedBytes replaces repeated scalar checks with shared operations over two explicit units. It adds no persisted state, public API, or storage format. Five similar write changes could reuse this type without adding five accounting rules. The remaining liability is the separate text-based rule in the loader. Its callers already have the catalog properties needed to remove that rule.
The tradeoff is a larger accounted retention envelope: up to 64 MiB per operation instead of 32 MiB. This is not an RSS bound. Computing payload lengths also visits the Blob cells. I did not benchmark CPU time or peak memory. Unused Arrow capacity still counts as framing, so no retained buffer bytes leave the accounting. See logical_blob_payload_bytes and the external-copy admission and recheck.
Local validation used Rust 1.97.1, the locked dependencies, failpoints, and local filesystem storage:
| Check | Result |
|---|---|
Clean head, writes owner |
57 passed, none skipped |
| Unchanged Blob GQT case, head and parent | One ordinary execution passed at each commit |
Same temporary GQT boundary regression at b8b631ee8f813e557805baf33efe869011f48275 |
Failed at the added insert: keyed entity bytes for node:Doc, actual 33,556,272, limit 33,554,432 |
Same regression at 2c305ca41de66be39be4d3a2e648f74c109b2ee6 |
Passed, one ordinary execution |
| Temporary mixed String/Blob assertion in the existing exact-limit Rust test | Mutation passed. Merge-load failed: keyed parsed entity Blob payload bytes for node:Document, actual 34,603,008, limit 33,554,432 |
Please include a minimal GQT boundary regression. I extended blob_update_carries_unassigned_blobs.gqt with one insert, one 32 MiB Blob parameter, and expect affected: nodes=1 edges=0. Thus, the claim that this is outside the case format is too broad. Generated loads have a 16 MiB cap, but mutation parameters do not. The temporary parameter fixture is 44.7 MB of base64 text; that fixture cost should remain explicit. Keep byte-read and pre-effect assertions in the existing Rust owner. The compatibility merge-load probe also belongs there: GQT exposes generated loads, whose cap prevents this input.
Commands (the GQT file was identical at both commits):
cargo +1.97.1 test -p omnigraph-engine --locked --features failpoints --test writes -- --test-threads=2
cargo +1.97.1 test -p omnigraph-engine --locked --features failpoints --test writes exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refused -- --exact --nocapture
cargo +1.97.1 run -p omnigraph-gqt --locked --features omnigraph/failpoints --bin omnigraph-gqt -- /tmp/review-903/blob_limit.gqt --target omnigraph-engine --storage local-filesystemI reused CARGO_TARGET_DIR=/Users/ragnor/work/omnigraph/.claude/worktrees/omnigraph-heal-race-2f5af1/target with RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'. At the parent, after rebuilding with the unchanged GQT case, I ran "$CARGO_TARGET_DIR/debug/omnigraph-gqt" /tmp/review-903/blob_limit.gqt --target omnigraph-engine --storage local-filesystem --artifacts /tmp/review-903/gqt-parent-artifacts. The final head confirmation passed again in 15.09 seconds. GQT results above cover the ordinary target only. All temporary source edits are restored. No PR changes were pushed.
I checked the complete relevant Lance guidance against crates.io Lance 11.0.0, source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. Its merge-insert writer uses default write parameters and preserves captured stable row IDs. I also checked the pinned Blob builder and external-Blob writer options against the upstream Blob guide. No Lance dependency or storage-format change is part of this PR.
Exact-head CI also passed: workspace tests and cloud/storage checks, ordinary and DST GQT, and the pinned DST suite. These are CI results, separate from the local tests above. I found no additional blocking defect or optional design change to request.
The compatibility loader's pre-decode forecast charged every `base64:` string as Blob payload, so a declared String whose text starts with `base64:` used the payload ceiling: a 24 MiB Blob beside such a 12 MiB String was refused at 33 MiB although the batch check admits it. The forecast now takes the type's Blob properties, as the Arrow check does: only a Blob property's `base64:` value is payload; every other value is framing.
|
On the GQT boundary regression from the review: the case format can express it, but only as 44.7 MB of literal |
ragnorc
left a comment
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes until the promised minimal GQT regression is included. The loader defect from my previous review is fixed. I found no further code defect. Reviewed 90b6688ad773b4195723caa3416a64f5e332b740. This is a COMMENT review because the authenticated account is the author.
This PR lets a 32 MiB Blob fit beside its row. It separates payload bytes from the surrounding row bytes, with a 32 MiB limit for each. The follow-up fixes a false refusal when an ordinary String starts with base64:. For example, a 24 MiB Blob and 12 MiB of such String text must not count as 33 MiB of Blob payload.
The fix addresses the general cause: the accepted schema owns the property type. All four node/edge loader paths now pass the existing catalog's Blob property set to the shared forecast. Only those properties receive payload accounting. Nested values stay framing. The Arrow builders use the same catalog authority.
This removes a competing type rule and reduces long-term liability. It adds private arguments, with no public API, persistent state, or storage format. Five similar loader changes can reuse this helper. It adds one set lookup per property. I did not benchmark the cost.
The full PR permits up to 64 MiB of accounted retention across its two limits. That is not an RSS bound. HTTP body and strict NDJSON line limits still apply before this embedded-write boundary.
The earlier GQT request remains open at this head. Your update proposes a compact slow case after #927. Please include that case before merge, with evidence that it fails before the fix and passes after. Keep the compatibility-load, byte-read, and unchanged-head assertions in the existing Rust owner. GQT's generated-load cap prevents the large compatibility-load probe.
The planned generated parameter would avoid the 44.7 MB literal fixture used for my proof. That generator is outside this review's scope. I found no new inline defect or optional design change to request.
Local validation used Rust 1.97.1, locked dependencies, failpoints, and local filesystem storage.
| Regression | Before | Exact reviewed head |
|---|---|---|
Mixed String/Blob load, same writes test |
At 2c305ca41de66be39be4d3a2e648f74c109b2ee6, failed with payload actual 34,603,008 and limit 33,554,432 |
One test passed, none skipped |
| Temporary GQT boundary case, identical file | At b8b631ee8f813e557805baf33efe869011f48275, failed at the added insert: keyed entity bytes actual 33,556,272, limit 33,554,432 |
One ordinary execution passed in 12.72 seconds |
For the loader check, I copied the head's test into the old checkout without changing its production code. The failure came from the predicted false refusal. A clean head baseline and the restored-head run both passed.
The GQT probe extends blob_update_carries_unassigned_blobs.gqt with one insert. Its Blob parameter contains 32 MiB of byte 0x07, and it expects one affected node. This proof uses a 60-second file timeout. The GQT proof covers one ordinary filesystem execution at each commit. Other environments were unselected. Production placement must respect the ordinary corpus's 10-second cap, or use cases_slow.
Commands, run at the commits above:
export RUSTUP_TOOLCHAIN=1.97.1
export CARGO_TARGET_DIR=/tmp/review-903-followup/target
export RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'
cargo test -p omnigraph-engine --locked --features failpoints --test writes exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refused -- --exact --nocapture
cargo run -p omnigraph-gqt --locked --features omnigraph/failpoints --bin omnigraph-gqt -- /tmp/review-903-followup/blob_limit.gqt --target omnigraph-engine --storage local-filesystem --artifacts /tmp/review-903-followup/gqt-before-artifactsThe head GQT run used gqt-after-artifacts. The documentation and agent-link checks passed. All temporary source edits are restored. No PR changes were pushed.
The exact-head CI run passed. Its writes owner passed all 57 tests, including the new mixed String/Blob assertion. The loader forecast unit test passed too. Ordinary and DST GQT and the pinned DST suite also passed. These CI results are separate from the local checks above.
I checked the relevant upstream guidance against crates.io Lance 11.0.0, source ab6b5bbe46009ed78746b444df8db59a8bc5d842. The Blob builder and merge-insert writer support the unchanged storage assumptions. Current upstream documentation includes newer features, so it was not a substitute for the pinned source. I did not run cloud storage, DST, or performance checks locally.
…udget # Conflicts: # crates/omnigraph/src/table_store.rs
1 participant
What & why
Step 1 ("3-pre") of the RFC 0033 Phase 3 plan in #900.
Every keyed write checked one 32 MiB ceiling on a batch's whole Arrow memory. That count includes a managed Blob value's bytes, so a value of exactly 32 MiB could never fit: the row around it is never empty. A one-row batch holding a 33,554,432-byte value and an
idmeasures 33,555,112 bytes. The documented 32 MiB per-value limit was therefore unreachable through keyed writes, and the plannedblob putcould not keep its "32 MiB inclusive" promise.Change
One shared accounting type.
KeyedBytesinstorage_layer.rssplits a batch's Arrow memory into two parts, each under its own 32 MiB ceiling:{data, uri}shape;The two parts always sum to the old count, so no byte leaves the accounting.
Every keyed check uses it:
stage_keyed_writeand keyed-write preparation, including the predicted external copy;account_keyed_json_row), where abase64:value counts its decoded length as payload.Resource names. Framing refusals keep their existing names. A payload refusal uses the same name with
Blob payload bytesin place ofbytes, for examplekeyed write Blob payload bytes for node:Document.Unchanged limits:
omnigraph loadand/load/ndjson) and HTTP body caps;Lance guards:
filtered_scan_tolerates_merge_update_row_id_overlapnow also asserts that a whole-row merge-update keeps the row's stable row id. The Blob write's ETag evidence will rely on that.merge_insert_refuses_an_external_blob_outside_the_dataset_basespins why carried external cells are materialized rather than re-sent as references.Behavior changes
.gqinsert or update parameter, a carried update cell, and the embeddedloadAPI.retained keyed batch Blob payload bytes per operation.Tests
staged_tests::keyed_bytes_split_blob_payload_from_framing: an exact-limit batch's split, a one-byte-over payload refusal, and an external reference counted as framing only.staged_tests::pending_scan_budget_caps_are_inclusive_and_one_over_is_typed: extended with the payload ceiling.loader::tests::parsed_forecast_charges_base64_payload_apart_from_framing.writes.rs:mutation_update_rejects_oversized_blob_before_payload_read_pre_effectnow asserts the payload refusal name, plus a new inclusive case: an external cell of exactly 32 MiB is carried and stored.exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refusedcovers:Two existing
writes.rstests pinned the old combined allowance and were adapted:external_blob_bytes_join_the_operation_payload_allowance_before_any_payload_readnow shows that a copied external payload shares the operation's payload allowance with managed payload: 12 MiB copied plus 21 MiB managed is refused before any payload read. The old case, 24 MiB copied plus 10 MiB of rows, is now accepted under the separate allowances.materialized_blob_batches_are_rechecked_against_the_operation_allowance_before_stagingkeeps its purpose. The builder's unused capacity is framing, so 13 MiB of rows per type makes the post-read re-check refuse what the prediction admitted.Local runs, all with
--features failpoints:table_storeloaderwriteslance_surface_guardsbranchingmerge_fast_forwardmerge_truth_tableend_to_endchangesAlso: engine Clippy (default and
failpoints),cargo fmt,check-docs.pyandtypos. The full workspace graph and the cloud suites are left to CI.Docs:
docs/user/blobs.md(limits table and loader note),docs/user/mutations/index.md,docs/user/cli/managed-data.md,docs/dev/writes.md,docs/dev/execution.md, and changelog fragmentblob-payload-budget.changed.md.