Size a segment's head reserve without holding its records

[bfulton]

Follow-up to #4, from two pieces of feedback: write_sorted never set compression, and its slice of records is the wrong shape for a caller who will not hold them.

The shape now

// Pass one: lengths only. Holds one u32 per sixteenth key, nothing else.
let mut p = reserve::Planner::new(opts.block_size, write.inline_max);
for (key_len, run_len) in lengths { p.push(key_len, run_len); }
let reserve = p.finish().unwrap().bytes();

// Pass two: the records, streamed, with the reserve already known.
let mut w = SegmentWriter::create_with(path, &opts, &write, reserve)?;

With the whole input in hand, SegmentWriter::write_sorted(path, opts, write, generation, items) is those two steps in one call.

SegmentWriter::create_with(path, opts, write, reserve)	every per-file setting and the reserve, applied at construction
SegmentWrite	one field per setter on the writer — inline_max, compress, sync_every — and nothing else
reserve::Planner	the reserve accumulated one key at a time, from lengths
reserve::for_lengths	that planner over a slice
reserve::from_totals	an upper bound when only totals are known
reserve::Reserve	the four pieces, so the hash-directory copy can be priced separately

Why a constructor rather than setters

Every one of those settings must be applied before the first key, and each is silent when forgotten: a plain segment where a compressed one was wanted, or a zero reserve that costs the sparse reader a round trip and raises nothing. Four setters a caller has to remember is the wrong shape for that, and it is the shape that let the first write_sorted ship without compression at all — the same failure SegmentOptions refuses a compression field to avoid, reproduced one layer up.

create_with destructures SegmentWrite rather than reading its fields, so a setter added to the writer without a field beside it stops the build. That invariant is why the choice of what the reserve holds is not in that struct: create_with is handed the reserve as a number, so a policy field there would be one the call does not read.

Why the planner

for_lengths takes a slice of every key's lengths — sixteen bytes a key — and allocated placeholder arenas as large as all the keys and all the inline runs put together. A caller avoiding a gigabyte of records was handed a sizing function that allocates about as much. That was a defect in #4, not a gap in the request.

The estimate never needed the input, only four aggregates: the key count, the record bytes, how the runs cut into blocks, and the lengths of the keys the fence samples. Only the last is awkward, because the fence's stride is not known until the count is — but every stride the format can choose is a power of two at or above the smallest, so keeping one u32 per sixteenth key covers any of them. At ten million keys that is 2.5 MB against 160 MB for a slice of lengths. Planner::retained_bytes() reports it, so the claim is checkable rather than trusted.

The layout arithmetic still has one definition

It was inline in plan_inline after its per-key loop and only ever depended on those aggregates, so it moved to flatindex::section_layout, which plan_inline calls after counting and the planner calls after accumulating. Record bytes come from record_len_tail, the block table's size from block_table_len (which encode_blocks allocates by), the fence's stride from fence_stride. What is left in reserve is the cut into blocks, four lines of the writer's own rule. A second copy of any of it would be a second definition of the format.

Verified

sh scripts/check.sh — 98 tests, clippy -D warnings, formatted, wasm links.

• A twenty-thousand-key segment written without holding a record — key and values generated from an index in both passes — opens from its own probe, reads back, and is byte-identical to the same segment from the batch writer. A version that collected into a Vec first would pass every assertion and prove nothing, which is why the generator shape is the test.
• The planner and the slice agree at key counts either side of the sampling stride (0, 1, 15, 16, 17, 4096, 40 000).
• Compression is checked by asking the file, not the flag: a compressed segment of compressible values must be smaller and must still read back. It fails against the bug it was written for, with the two sizes identical.
• create_with is held to the four setters called by hand, at both reserves; write_sorted is held to create_with.
• The reserve is minimal, not merely sufficient: a test binary-searches the smallest reserve a reader can still open and seek from and holds the estimate to it. The gap is twelve bytes, and it has one cause — the checksum row is cut on the object's pages, so its length depends on where the section lands, which depends on the answer; it is taken at its worst alignment.

One thing worth knowing

Byte-for-byte comparisons of segments normalise four words away. A segment's superblock records SystemTime::now() in seconds and an FNV over the fields covers it, in each of the two slots, so two writes of identical input differ whenever they straddle a tick — which is a test that passes on a fast machine and fails on a slow one, and did, on macOS. A consequence beyond the tests: two builds of the same input are not the same bytes, so a segment cannot be content-addressed or cache-validated across builds. That is a property of the format, untouched here.

Not done

This needs a source that can be traversed twice. For a single pass over something unrewindable, from_totals gives a bound, or write to a temp name and rename — happy to build for whichever they actually have.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F2wxgRU31dUrqaY8KwfsA2

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

reserve::Planner::push uses unchecked arithmetic for rec_bytes, which can overflow (notably on 32-bit/wasm32) and yield an incorrect reserve instead of failing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Refactors segment head-reserve sizing so it can be computed from streamed length data (without holding records), and makes segment construction apply all per-file settings plus the reserve at creation time.

Changes:

• Add reserve::Planner for one-pass aggregate sizing, and rework reserve::for_lengths / from_totals to use it.
• Introduce SegmentWriter::create_with(...) and route write_sorted through it to ensure all per-file settings are applied up-front.
• Add/adjust tests to validate streaming write shape, reserve correctness, and constructor-vs-setters equivalence; update README description.

File summaries

File	Description
tests/segwriter.rs	Adds coverage for create_with behavior and a two-pass streaming writer workflow that never retains full records.
src/reserve.rs	Implements reserve::Planner and rewires reserve sizing helpers to avoid allocating input-shaped buffers.
src/flatindex.rs	Factors layout arithmetic into section_layout and exposes helpers used by the new reserve planner.
src/db.rs	Adds SegmentWriter::create_with and simplifies write_sorted to size reserve and construct writer in one consistent path.
README.md	Updates the segment reserve narrative to mention the planner + streaming writer workflow.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

SegmentWriter::create_with currently moves *write out of a shared reference (non-Copy), which should not compile and needs a small fix before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/reserve.rs:320

• The doc comment claims “a slice of lengths is sixteen bytes a key”, but [(usize, usize)] is 16 bytes/key only on 64-bit; on 32-bit targets it’s 8 bytes/key. Consider expressing this in terms of size_of::<(usize, usize)>() (or qualify “on 64-bit”) to keep the docs correct across targets.

This issue also appears on line 453 of the same file.

src/reserve.rs:458

• This test hard-codes 16 bytes/key for a Vec<(usize, usize)> slice (1_000_000 * 16), which is only true on 64-bit. Using size_of::<(usize, usize)>() keeps the assertion accurate if the test suite is ever run on 32-bit targets (e.g. wasm32).

        assert_eq!(p.keys(), 1_000_000);
        assert_eq!(p.retained_bytes(), 1_000_000usize.div_ceil(16) * 4);
        // Against sixteen bytes a key for a slice of the same lengths, and
        // the 116 bytes a key the records themselves are.
        assert!(p.retained_bytes() * 60 < 1_000_000 * 16);
        assert!(p.finish().is_some());

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There is a confirmed overflow bug in flatindex::section_layout (keys * 2) and a README example that won’t compile as written due to missing references.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/flatindex.rs:739

• section_layout computes the hash capacity growth loop condition with keys * 2, which can overflow usize (notably on 32-bit targets), potentially producing an incorrect capacity or an infinite/short loop. Use checked multiplication for the keys*2 target so overflow returns None like the other sizing paths.
  README.md:96
• In the README inline call to SegmentWriter::create_with, the arguments are shown by value (opts, write), but the actual signature takes &SegmentOptions and &SegmentWrite. As written, this example won’t compile if a user copies it into code.

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite