fix(storage): bound contiguous partition routing buffers - #1491
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: CurateLabs/graphforge/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
ChangesBounded PartitionRun buffering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The implementation satisfies the byte-bound routing objective in
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please focus on whether byte-bound flushes preserve record framing, partition transitions, row accounting and durable output, and whether the regression tests cover those contracts. The 1 MiB constant is provisional pending the requested 64 KiB / 256 KiB / 1 MiB quiet-host curve; no performance claim is ready yet. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD || git diff --name-status HEAD~1 HEAD
echo
echo '=== Diff stat ==='
git diff --stat "$(git merge-base HEAD origin/main)" HEAD || git diff --stat HEAD~1 HEAD
echo
echo '=== Rust outlines for changed files ==='
while IFS= read -r file; do
case "$file" in
*.rs)
echo "--- $file ---"
ast-grep outline "$file" --items all --type function,struct,impl,enum,trait 2>/dev/null || true
;;
esac
done < <(git diff --name-only "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-only HEAD~1 HEAD)Length of output: 1471 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
echo '=== Patch ==='
git diff --find-renames --find-copies --unified=80 "$base" HEAD -- \
crates/graphforge-storage/src/graph_construction/shape.rs \
crates/graphforge-storage/src/graph_construction/shape/tests.rs
echo
echo '=== shape.rs outline ==='
ast-grep outline crates/graphforge-storage/src/graph_construction/shape.rs --items all --type function,struct,impl,enum,trait || true
echo
echo '=== tests.rs outline ==='
ast-grep outline crates/graphforge-storage/src/graph_construction/shape/tests.rs --items all --type function,struct,impl,enum,trait || trueLength of output: 16052 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Relevant definitions and call sites ==='
rg -n -C 8 --glob '*.rs' \
'fn route_slice\b|fn finish_optional\b|fn balance\b|struct FixedRangePartitioner\b|fn run_record_bytes\b|fn read_run_record\b|PartitionRun::with_bound|route_fixed_run\b|route_identity_run\b' \
crates/graphforge-storage/src/graph_construction
echo
echo '=== shape.rs relevant implementation ranges ==='
rg -n --glob '*.rs' 'struct FixedRangePartitioner\b|impl.*FixedRangePartitioner|fn route_slice\b|fn finish_optional\b|fn route_fixed_run\b|fn route_identity_run\b|struct PartitionRun\b|fn run_record_bytes\b|fn read_run_record\b' \
crates/graphforge-storage/src/graph_construction/shape.rsLength of output: 26302 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== FixedRangePartitioner routing and finalization ==='
sed -n '379,680p' crates/graphforge-storage/src/graph_construction/partition_shaping.rs
echo
echo '=== Fixed spill writer implementation ==='
rg -n -C 12 --glob '*.rs' 'struct FixedSpillWriter\b|impl.*FixedSpillWriter\b|fn append\b|fn seal\b' \
crates/graphforge-storage/src/graph_construction/partition_shaping.rs
echo
echo '=== New-test context and available test helpers ==='
sed -n '1,180p' crates/graphforge-storage/src/graph_construction/shape/tests.rs
sed -n '700,810p' crates/graphforge-storage/src/graph_construction/shape/tests.rs
echo
echo '=== Existing end-to-end route test references ==='
rg -n -C 6 --glob '*.rs' \
'route_fixed_run\(|route_identity_run\(|partition_rows|source content changed|source authority changed|finish_optional\(' \
crates/graphforge-storage/src/graph_construction/shape/tests.rs \
crates/graphforge-storage/src/graph_constructionLength of output: 62470 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== FixedRangePartitioner publication completion ==='
sed -n '650,745p' crates/graphforge-storage/src/graph_construction/partition_shaping.rs
echo
echo '=== DetailCodec wire and bytes contracts ==='
rg -n -C 10 --glob '*.rs' 'enum DetailCodec\b|impl DetailCodec\b|fn wire\b|fn bytes\b|fn read\b' \
crates/graphforge-storage/src/graph_construction
echo
echo '=== Tests that cover source-routing authentication and session reopen ==='
rg -n -C 5 --glob '*.rs' \
'content changed before partitioning|partition source content changed|shape_canonical_with_cancellation.*open|GraphConstructionSession::open\(|resumed' \
crates/graphforge-storage/src/graph_construction/shape/tests.rsLength of output: 14087
I treated the 1 MiB value as provisional and did not assess performance claims. ✅ Action performedReview finished.
|
5c698b5 to
7eb882f
Compare
github-actions
Bot
added
the
documentation
1 participant
Bounds
PartitionRunat 64 KiB, flushing on byte capacity or partition change while preserving whole records, row counts and output order. Oversized individual records bypass the accumulator.Three clean rotating-order S18 comparisons selected 64 KiB: median complete-ingest wall was 28.798 s against the unbounded baseline's 29.320 s. The selected candidate passes the recorded S18/S19/S20 throughput floors; contended observations are retained and excluded. The checked-in report and JSON contain the protocol, results, limitations and binary/input hashes. This establishes a bounded buffer without a material regression under the predeclared selection rule, not a throughput-improvement claim.
Validation:
make pre-push-fast, gate-registry validation and formatting passed.Closes #1445
Note
Bound contiguous partition routing buffers in
PartitionRunto 64 KiBPartitionRunaccumulator in shape.rs, defaulting to 64 KiB in production and configurable for tests.pushpath flushes partial data before the buffer would exceed the bound and routes an individual record at or above the bound directly instead of buffering it; records are never split.PartitionRunnow retains at most the configured bound of buffered wire bytes; any caller assuming full-run buffering before flush will see earlier, partial flushes. The new bound is enforced in shape.rspush.Macroscope summarized 7eb882f.