You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Prevent Arrow offset overflow in DiskGraph temporal output - #166
DiskGraph can panic while turning decoded rows into an Arrow batch when one string or binary column contains more than about 2 GiB of values. Arrow's ordinary arrays use 32-bit offsets, so this can happen even when the machine has plenty of free memory. Lists have the analogous limit on their child count.
This patch adds a 64-bit-offset RowPack decoder and uses it for temporal-query output. String aggregates also use 64-bit offsets. Existing callers of the original RowPack decoder keep their 32-bit Arrow types. Values, nulls, timestamps, and row/projection order are preserved.
Compatibility: temporal Arrow output now uses large_string, large_binary, and large_list, including large string children in lists. Consumers that check physical Arrow types must accept those types. This does not bound query memory or make arbitrarily large results fit in RAM.
Minimal reproduction
No external dataset or running service is needed. From packages/diskgraph, on a machine with at least 6 GiB free:
The test repeats one encoded 1 MiB string 2,049 times. It first confirms that the existing decoder panics at the offset limit, then checks every value returned by the new decoder. The test is ignored in ordinary runs because it deliberately allocates more than 2 GiB.
Local Apple Silicon Mac: 321 RowPack tests, six focused Rust temporal-query tests, and 340 Python integration tests passed.
Explicit >2 GiB regression passed on Linux before the performance-only inlining adjustment; not repeated on Mac.
Nine paired release trials per decoder case and per full-query output format. Mac testing exposed a 10–15% decoder regression in the initial refactor; inlining the generic row decoder removed it.
Full queries still measured about 1% slower by median. Wider offsets also increase memory use: nested-list allocated buffers increased 51%, with about 11% higher decoder-process peak RSS on Mac.
Remain draft: the strict no-performance-regression requirement is not fully met. These synthetic results do not establish performance neutrality on other workloads or architectures.
The first version of this patch made integer decoding 9.8% slower and binary decoding 15.5% slower than the baseline. The same slowdown occurred with 32-bit offsets, which pointed to the shared decoder refactor rather than larger offsets alone.
The fix adds #[inline(always)] to the private generic row decoder, decode_row_to_builders_with_offsets. This lets the compiler inline it into the batch loop. Retesting removed the large measured slowdown on both offset-width paths; it does not change the decoded values or Arrow types.
The final comparison used release builds on an Apple Silicon Mac with 48 GiB RAM: nine randomized paired process runs per case, identical inputs, and separate baseline/candidate builds. A time ratio below 1 means the candidate took less time.
Decoder case
Final candidate / baseline median time
Integer
0.940
8-byte string
0.973
8-byte binary
0.997
Four short strings per list
0.973
1,024-byte string
0.993
These are measurements, not established speedup claims; small differences have overlapping trial ranges. A follow-up on the original 32-bit path measured ratios of 0.941 for integers and 1.005 for binary, with overlapping binary ranges.
Full temporal queries returned 8,192 rows per query. All returned values and deterministic statistics matched across baseline and candidate. Each process ran five warm-ups and 100 timed queries.
Output format
Baseline median time for 100 queries
Candidate median time
Baseline / candidate median peak RSS
model
0.27925 s
0.28196 s
68.66 / 69.50 MiB
inspectable
0.28904 s
0.29178 s
68.94 / 69.97 MiB
Remaining costs: full-query medians are about 1% slower. inspectable was slower in eight of nine paired trials, so this cannot be described as proven performance-neutral. Wider offsets also use more memory: in the nested-list decoder case, allocated Arrow buffer capacity grew 51% and process peak RSS grew about 11%. Allocated capacity and RSS are different measurements.
Checklist
I listed all the breaking changes introduced by the PR.
I updated the relevant documentation.
I added regression tests for the changed behavior.
RowPack now supports large-offset Arrow decoding for string, binary, and list values while retaining the existing 32-bit-offset batch API. Temporal queries use the new decoder, and aggregate string outputs use large-offset arrays. The changes add decoding and temporal-query tests, benchmark examples, and a performance assessment.
Sequence Diagram(s)
sequenceDiagram
participant TemporalQuery
participant RowPackDecoder
participant ArrowBuilders
TemporalQuery->>RowPackDecoder: Decode rows with large offsets
RowPackDecoder->>ArrowBuilders: Append decoded values
ArrowBuilders-->>RowPackDecoder: Return Arrow record batch
RowPackDecoder-->>TemporalQuery: Return decoded batch
Loading
Merge Risk: Moderate · up to 07c92
The new large-offset Arrow decoding for temporal queries looks sound, but the added benchmark example fails the repository's clippy check and blocks CI. Fix that one-line lint failure before merging. The author also reports a small measured slowdown, about 1% in median time for full temporal queries, which the PR documentation says is unresolved.
Pre-merge checks: 2 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Change Map Design Review
The required conversation comment beginning with <!-- kumo-pr-change-map --> is not available. The repository contains only the review guidance, and the supplied PR description is explicitly truncat…
Provide the complete marked change-map conversation comment, or make the full PR description and linked comment available. Then review each mapped change against the repository diff and the Arrow offset failure.
Passed checks: 2 passed
Check name
Status
Explanation
Pr Size
The pull request changes 8 files. This does not exceed the 40-file limit.
Public Resource Access
The PR adds one repository link at packages/diskgraph/diskgraph-pybind/README.md:178. It resolves to packages/diskgraph/docs/arrow-offset-performance.md, which is publicly accessible on the PR bra…
Full details: Public Resource Access
Explanation
The PR adds one repository link at packages/diskgraph/diskgraph-pybind/README.md:178. It resolves to packages/diskgraph/docs/arrow-offset-performance.md, which is publicly accessible on the PR branch (HTTP 200). The added baseline commit reference at packages/diskgraph/docs/arrow-offset-performance.md:12 is also publicly accessible through the GitHub API (HTTP 200). No added external URL or package source reference was inaccessible.
Full details: Change Map Design Review
Explanation
The required conversation comment beginning with <!-- kumo-pr-change-map --> is not available. The repository contains only the review guidance, and the supplied PR description is explicitly truncated. The code diff can show implementation changes, but it cannot establish the proposed change map's per-change rationale, observed failure, abstraction-layer decisions, focused verification, or stopping condition.
Resolution
Provide the complete marked change-map conversation comment, or make the full PR description and linked comment available. Then review each mapped change against the repository diff and the Arrow offset failure.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Observed failure: the native Arrow string builder panics once its cumulative value bytes exceed the signed 32-bit offset range. The existing decoder selects ordinary Arrow builders regardless of output size. The smallest proposed change is a separate decoder using large offsets, selected by the temporal-query path. Existing narrow-decoder callers are preserved.
File
Function / scope
What changed
Why
packages/diskgraph/diskgraph-rowpack/src/lib.rs
RowPack decoding, Arrow builders, null handling
Adds a generic offset-width implementation and a public large-offset decoder; forwards timestamp clocks and inlines the generic row decoder to preserve throughput
Prevent native output offset overflow without changing the original decoder's schema
Generates small inputs and reports timing, memory, and value/statistics digests
Compare both output formats reproducibly without external data
Mac results: 321 RowPack tests, six Rust temporal-query tests, and 340 Python integration tests passed. The real >2 GiB boundary test passed on Linux before the inlining adjustment. Nine paired trials per case exposed and then removed a 10–15% decoder regression. Full queries retain approximately 1% median overhead and wider offsets increase memory use. The strict no-regression requirement remains unmet; detailed methods and limits are in the performance assessment.
Stopping condition: retain draft status until correctness and measured performance are satisfactory. Bounded-memory execution, scheduling, and other query optimizations are outside this patch.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Major · Functional Correctness · Quick win
Fix the Clippy failure in the benchmark.
The supplied pipeline log shows that cargo clippy --locked --workspace --all-targets -- -D warnings fails on &[value.clone()]. Pass std::slice::from_ref(&value) instead. This keeps value available for the later encode call. (raw.githubusercontent.com)
Proposed fix
--- "a/packages/diskgraph/diskgraph-rowpack/examples/arrow_offset_bench.rs"+++ "b/packages/diskgraph/diskgraph-rowpack/examples/arrow_offset_bench.rs"@@ -26,7 +26,7 @@
scale: None,
}];
let mut cb = RowPackContextBuilder::new(columns, RowPackConfig::default());
- cb.observe_row(&[value.clone()])?;+ cb.observe_row(std::slice::from_ref(&value))?;
cb.observe_row(&[ValueRef::Null])?;
let ctx = cb.finish()?;
let encoder = RowPackEncoder::new(ctx.clone())?;
Committable suggestion
IMPORTANT
Review the replacement and indentation, then test before committing.
Suggested change
cb.observe_row(&[value.clone()])?;
cb.observe_row(std::slice::from_ref(&value))?;
Tools
GitHub Actions: Testing and Coverage / 49_diskgraph-clippy.txt
[error] 29-29: cargo clippy --locked --workspace --all-targets -- -D warnings failed: Clippy's cloned_ref_to_slice_refs lint flags &[value.clone()] as an unnecessary clone. Use std::slice::from_ref(&value).
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
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.
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
DiskGraph can panic while turning decoded rows into an Arrow batch when one string or binary column contains more than about 2 GiB of values. Arrow's ordinary arrays use 32-bit offsets, so this can happen even when the machine has plenty of free memory. Lists have the analogous limit on their child count.
This patch adds a 64-bit-offset RowPack decoder and uses it for temporal-query output. String aggregates also use 64-bit offsets. Existing callers of the original RowPack decoder keep their 32-bit Arrow types. Values, nulls, timestamps, and row/projection order are preserved.
Compatibility: temporal Arrow output now uses
large_string,large_binary, andlarge_list, including large string children in lists. Consumers that check physical Arrow types must accept those types. This does not bound query memory or make arbitrarily large results fit in RAM.Minimal reproduction
No external dataset or running service is needed. From
packages/diskgraph, on a machine with at least 6 GiB free:cargo test -p diskgraph-rowpack --lib \ tests_arrow_large::large_offsets_cross_i32_byte_boundary \ -- --ignored --exactThe test repeats one encoded 1 MiB string 2,049 times. It first confirms that the existing decoder panics at the offset limit, then checks every value returned by the new decoder. The test is ignored in ordinary runs because it deliberately allocates more than 2 GiB.
Change map
File-by-file change map
Testing Done
Performance results and decoder improvement
The first version of this patch made integer decoding 9.8% slower and binary decoding 15.5% slower than the baseline. The same slowdown occurred with 32-bit offsets, which pointed to the shared decoder refactor rather than larger offsets alone.
The fix adds
#[inline(always)]to the private generic row decoder,decode_row_to_builders_with_offsets. This lets the compiler inline it into the batch loop. Retesting removed the large measured slowdown on both offset-width paths; it does not change the decoded values or Arrow types.The final comparison used release builds on an Apple Silicon Mac with 48 GiB RAM: nine randomized paired process runs per case, identical inputs, and separate baseline/candidate builds. A time ratio below 1 means the candidate took less time.
These are measurements, not established speedup claims; small differences have overlapping trial ranges. A follow-up on the original 32-bit path measured ratios of 0.941 for integers and 1.005 for binary, with overlapping binary ranges.
Full temporal queries returned 8,192 rows per query. All returned values and deterministic statistics matched across baseline and candidate. Each process ran five warm-ups and 100 timed queries.
modelinspectableRemaining costs: full-query medians are about 1% slower.
inspectablewas slower in eight of nine paired trials, so this cannot be described as proven performance-neutral. Wider offsets also use more memory: in the nested-list decoder case, allocated Arrow buffer capacity grew 51% and process peak RSS grew about 11%. Allocated capacity and RSS are different measurements.Checklist