Skip to content

Command-line parity with rapidgzip, and line counting - #11

Closed
BenjaminDEMAILLE wants to merge 38 commits into
COMBINE-lab:mainfrom
BenjaminDEMAILLE:cli-parity
Closed

Command-line parity with rapidgzip, and line counting#11
BenjaminDEMAILLE wants to merge 38 commits into
COMBINE-lab:mainfrom
BenjaminDEMAILLE:cli-parity

Conversation

@BenjaminDEMAILLE

Copy link
Copy Markdown
Contributor

Sub-project 4a of 4 replacing #5. Stacked on #10, which stacks on #9, which stacks on #8. GitHub cannot base a cross-fork pull request on a fork branch, so this targets main and its diff contains all three parents; review the top four commits only. Merge #8, #9, and #10 first.

--analyze is deliberately not here. It needs a DEFLATE block walker in the core library plus a byte-exact reimplementation of rapidgzip's report, which together are the same size again. That is sub-project 4b, already designed in docs/superpowers/specs/2026-08-01-analyze-design.md.

The option surface

Every option rapidgzip 0.16.0 accepts is now accepted here under the same short and long names, so a command line written for that tool works. The work already in the library becomes reachable without writing Rust: indexes in five formats, seeking, zlib and raw DEFLATE.

main.rs kept only arguments and dispatch. Input classification and output destination went to source.rs, index formats to index.rs, range parsing and extraction to ranges.rs, result printing to report.rs. The two pieces with real logic, range parsing and output path derivation, are pure functions with unit tests beside them.

Three options are accepted no-ops because they name behaviour this crate does not have: --io-read-method, which has one strategy here, and --sparse-windows with its negation, since index windows are always dense. A dense index is valid and interoperable, merely larger, so accepting them lies about nothing.

--no-verify is refused instead. Verification is structural here: a member is accepted only after its CRC32 and size check out. Accepting the flag would promise a speedup that does not exist.

Two more refusals replace silent surprises: --import-index without --ranges, where the index would go unused, and --index-format gztool-with-lines without --count-lines, where the format has nowhere to get its counters.

Line counting, and the bug that made it necessary

Checkpoint::line_offset and GzipIndex::total_line_count already existed and were read by the gztool importer. Every decode path wrote zero. A gztool-with-lines index exported from here was silently wrong.

DecoderBuilder::count_lines fills them. Counting happens in the Output implementations, on the thread that emits, because that is the only place the bytes are final: the marker path's chunks hold 16-bit symbols until the coordinator resolves them, and a marker can resolve to a newline.

Checkpoint line offsets come from merging two ordered streams. The builder keeps the offsets it has been offered but not yet passed; each run of output resolves the ones it covers in one scan. It tracks whether every checkpoint was resolved and claims a total only when they all were, so a future path that offers late degrades to an index without counters rather than one full of zeros.

IndexedReader::seek_to_line seeks by zero-based line, reading at most one checkpoint spacing regardless of file size, and refuses an index without counters rather than scanning from the start.

Two more interop bugs, found by a test

Adding a test where real gztool extracts by line from an index we wrote found two bugs the existing byte-offset test could not see.

gztool numbers lines from one; our offsets count newlines before a point. Export and import now convert.

More seriously, a member checkpoint recorded the member start, so gztool resumed by inflating raw DEFLATE at the gzip header and reported Compressed data error. indexed_gzip and gztool both record where DEFLATE begins. Member checkpoints now do too. BGZF block checkpoints still record the block start, which is what .gzi means by an offset.

The old test missed this because it only ever seeks to an interior checkpoint, which carries a window and a bit offset and was always correct. Only the empty-window member points were wrong.

Verification

  • 18 CLI integration tests driving the built binary through CARGO_BIN_EXE_rapidgzip-rust, no new dependency.
  • 9 unit tests for range parsing and output path derivation.
  • 11 line-counting tests asserting the property the format actually promises: each checkpoint's line offset equals the newlines preceding its decompressed offset, on both the marker grid and BGZF.
  • All 7 index interop tests pass against real bgzip, indexed_gzip, and gztool 1.8.2, in both directions, including the new extract-by-line case.
  • fmt, clippy, rustdoc, and the full suite green.

BenjaminDEMAILLE and others added 30 commits August 1, 2026 16:20
Every entry point required a `ReadAt` source, so gzip arriving on a pipe
could not be decoded at all.

Path 1 only ever moves forward through the compressed bytes, so extract
the operations it uses into an internal `InputCursor` trait and implement
it for both the positional `SourceCursor` and a new forward-only
`StreamCursor`. Member framing, footer verification, trailing-garbage
detection, and the output limit are then literally the same code for a
stream as for a file, rather than a second implementation that could
drift.

Adds `Decoder::decode_stream` and `Decoder::stream_reader`, mirroring
`Decoder::decode` and `Decoder::reader`. `Decoder::open` routes a path
that cannot be read positionally to the streaming driver, which turns a
call that failed before into one that succeeds. The CLI accepts `-` for
standard input and follows the same routing.

The streaming runtime is configured with one worker so the telemetry
reports the concurrency actually in use. A streaming reader does not join
its coordinator on drop, because that coordinator can be parked in a read
against a producer that never writes again.

No new dependency and no new unsafe code.

Refs COMBINE-lab#6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every case is fed through a source that implements only `Read`, so none
of them can silently fall back to positional reads the way a `Cursor`
would, and each is compared against the positional decode of the same
bytes.

Covers single-member, concatenated and empty members, BGZF with its EOF
member, a fully stored stream, truncation inside DEFLATE and inside the
footer, a corrupt CRC32 and a corrupt ISIZE naming the offending member,
trailing garbage, the output limit, telemetry, `Box<dyn Read + Send>`
through paraseq, dropping a reader against a stalled producer, and a slow
producer writing in small chunks.

On unix it also decodes a real `cat` pipe and checks that
`Decoder::open` routes a FIFO to the streaming path while leaving a
regular file on a parallel one.

Refs COMBINE-lab#6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records what streaming input supports, what it does not, and why: the
same verification as a regular file because it is the same sequential
code, but no parallelism because every parallel path needs positional
reads.

ARCHITECTURE.md explains that the length snapshot is absent for a stream
rather than mutable, that end of input plus the existing trailing-garbage
check is what makes it authoritative, and why a streaming reader does not
join on drop.

SAFETY.md is unchanged; this feature added no unsafe code.

Refs COMBINE-lab#6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading into an empty buffer tail returns `Ok(0)`, which the cursor would
read as end of input and turn into silent truncation. Callers only refill
a drained window so this is unreachable today, but the failure mode is bad
enough to be worth closing.

Refs COMBINE-lab#6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First of four sub-projects reimplementing PR COMBINE-lab#5 in reviewable pieces.
Covers index construction as a by-product of decoding, the native,
GZIDX, .gzi, and gztool on-disk formats, and a separate IndexedReader
for random access.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirteen tasks from index types through the four on-disk formats, the
IndexedReader, decode-path wiring, interop tests, and documentation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wrapper keeps the crate's inflate-side unsafe in one place and now
also accepts a predecessor window as a plain byte slice, which the
indexed reader needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Index building is opt-in through DecoderBuilder::build_index and arrives
in DecodeReport::index. Member starts are offered by the sequential and
streaming paths, which every source reaches. DecodeReport is no longer
Copy because it can now carry an index.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Advanced SIMD arm always returns, which made the scalar fallback
unreachable on AArch64, and the mask load had no safety comment. Both
are invisible on x86_64, where CI runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The estimated-grid path offers each chunk start with its resolved
predecessor window, which is where interior, non-byte-aligned resume
points come from. BGZF blocks are independent members, so every
non-empty block start is offered with no window, using each block's
ISIZE footer for its decompressed offset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six ignored tests exercise both directions for bgzip, indexed_gzip, and
gztool, with a CI job that installs all three. The first run found a
real bug: a checkpoint without a predecessor window is not necessarily a
member start, since indexed_gzip records its first point at the DEFLATE
start, so the reader now detects a gzip header instead of assuming one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
deflateBound takes a c_ulong, which is 32 bits on Windows, and the
pinned gztool tag does not exist; the project's tags reach v1.8.2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A single-stream loop over InputCursor covers both containers for
positional and non-seekable sources alike. Format selection is explicit
through DecoderBuilder::format, with auto-detection between gzip and
zlib; raw DEFLATE has no header and must be requested. Raw streams
accept an optional expected decompressed size, the only end-to-end check
that format allows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both containers are a single DEFLATE stream, so the estimated-grid path
handles them by starting at the format's DEFLATE offset and verifying
the format's trailer where a gzip footer would go. Accounting keeps an
Adler-32 for zlib instead of a CRC32. Index checkpoints record the
DEFLATE start, as indexed_gzip does, so IndexedReader resumes there
without change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sequential gzip loop, the single-stream zlib and raw DEFLATE loop,
and the BGZF block decoder now inflate through InflateBackend, so an
alternative implementation plugs in without touching a call site. The
marker/window path and IndexedReader keep the concrete zlib-rs inflater,
because they resume at arbitrary bit offsets and need zlib's Z_BLOCK
contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The member loop still drove zlib-rs directly, so it was not actually
pluggable. It now goes through the trait like the other whole-stream
paths.

Making that work exposed a hole in the abstraction: `RawInflater` kept
inherent `new`, `reset`, and `message` alongside its trait methods, so
every call site resolved to the inherent version and the trait was dead
code. Those three now live only in the trait impl. What stays inherent is
what the trait deliberately omits, `prime` and the two dictionary calls,
which belong to the paths that resume mid-stream.

Drop `set_dictionary` from the trait for the same reason: no whole-stream
path installs a window, since each stream it inflates starts at its own
beginning.

Backends report DEFLATE errors without a stream position, having seen
only the buffer they were handed. `at_bit_offset` lets the call site,
which knows the position, fill it in.
`isal_backend.rs` implements the backend trait over ISA-L, so the
whole-stream paths decode through it when the feature is on. The default
build is untouched: `isal-sys` is an optional dependency and nothing else
changed.

ISA-L is linked, not vendored. `use-system-isal` and `shared` keep the
build from compiling the library from source, which would need autotools
and an assembler.

Two details the C interface forces:

`inflate_state` embeds a 64 KiB scratch buffer, so it is boxed rather
than held inline. It is zeroed before `isal_inflate_init`, which sets
every scalar but leaves the scratch buffers alone.

ISA-L reads ahead into a bit buffer, so at the end of a stream it has
taken input the stream does not own. Whole bytes still in that buffer are
given back, otherwise the gzip footer and the next member start at the
wrong offset. The concatenated-member and BGZF tests are what catch this.

The full suite passes with `--features isal`, including the indexed-seek
tests, which is the check that the bit-accurate paths still run on
zlib-rs.
BenjaminDEMAILLE and others added 8 commits August 1, 2026 20:59
A job installing libisal-dev, then running clippy and the suite with the
feature on. The default jobs cannot reach this code: the feature is off
and the library is not vendored.
`benches/inflate_backend.rs` decodes a 16 MiB corpus single-threaded
through the two pluggable paths reachable from a byte slice. The backend
is a compile-time choice, so comparing means two runs against a criterion
baseline; the benchmark identifiers stay backend-agnostic so those runs
line up.

The corpus carries pseudo-random fields. Verbatim-repeating text
compresses into a few very long matches that both backends copy at memory
speed, which hid the Huffman decoding entirely: an earlier draft measured
3.9 GiB/s and would have compared nothing.

On Apple M-series against isa-l 2.32.1, ISA-L is 20% slower on both
paths, well outside the noise. That is the finding, and it is recorded
rather than buried. The CI job now also runs both benchmarks on x86-64,
where ISA-L's assembly decoder is strongest and where the project has no
local hardware to measure. The feature stays off by default either way.

Documentation covers the split in ARCHITECTURE.md, the ISA-L unsafe
argument in SAFETY.md, the measurement in PERFORMANCE_AUDIT.md, and how
to enable and reproduce in README.md and the crate docs. SAFETY.md also
gets a stale path corrected: the zlib wrapper moved to inflate.rs.
Sub-project 4 splits in two. Reproducing rapidgzip's --analyze needs a
block walker in core plus a byte-exact reimplementation of its report;
the option surface needs none of that. Together they would be a pull
request too large to review, which is the failure this whole exercise
exists to avoid.

Designing the CLI surfaced a real bug: Checkpoint::line_offset and
GzipIndex::total_line_count are read by the gztool importer and written
as zero by every decode path, so a gztool-with-lines index exported from
here would be silently wrong. Line counting lands in 4a, where -l and
line-addressed ranges need it anyway.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DecoderBuilder::count_lines` fills `DecodeReport::line_count` and, when
an index is also collected, every checkpoint's line offset plus the
index's total. This closes a latent bug: `Checkpoint::line_offset` and
`GzipIndex::total_line_count` were read by the gztool importer and
written as zero by every decode path, so a gztool-with-lines index
exported from here would have been silently wrong.

Counting happens in the `Output` implementations, on the thread that
emits. That is the only place the bytes are final: the marker path's
chunks hold 16-bit symbols until the coordinator resolves them, and a
marker can resolve to a newline.

Checkpoint line offsets come from merging two ordered streams. The
builder keeps the offsets it has been offered but not yet passed, and
each run of output resolves the ones it covers in a single scan. Offers
always precede the emit of the bytes at their offset, so nothing is
passed before it is known. Rather than trust that, the builder tracks
whether every checkpoint was resolved and claims a total line count only
when they all were, so a future path that offers late degrades to an
index without line counters instead of one full of zeros.

The tests check the claim the format actually makes: each checkpoint's
line offset equals the newlines preceding its decompressed offset, on
both the marker grid and BGZF.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`IndexedReader::seek_to_line` resumes at the checkpoint at or before a
zero-based line and scans forward, so at most one checkpoint spacing of
output is read regardless of file size. It refuses an index without line
counters rather than scanning from the start, which would defeat the
point of having an index.

Writing gztool's line-aware format now refuses an index that never
counted lines. Writing zeros produced a file gztool accepts and then
trusts, which is worse than refusing.

Adding an interop test where real gztool extracts by line found two bugs
in the export that the existing byte-offset test could not see.

gztool numbers lines from one: its first point reads L1 with no preceding
newline. Ours count newlines before the point. Export and import now
convert.

More seriously, a member checkpoint recorded the member start, so gztool
resumed by inflating raw DEFLATE at the gzip header and reported
"Compressed data error". indexed_gzip and gztool both record where
DEFLATE begins, and our own reader already tolerated either, having been
taught to skip a header it finds at a checkpoint. Member checkpoints now
record the DEFLATE start too. BGZF block checkpoints keep recording the
block start, which is what the .gzi format means by an offset.

The existing byte-offset test missed this because it only ever seeks to
an interior checkpoint, which carries a window and a bit offset and was
always correct. Only the empty-window member points were wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every option rapidgzip 0.16.0 accepts is now accepted here, with the same
short and long names, so a command line written for that tool works. The
work already in the library, indexes in five formats, seeking, zlib and
raw DEFLATE, becomes reachable without writing Rust.

main.rs kept only arguments and dispatch. Input classification and output
destination went to source.rs, index formats to index.rs, range parsing
and extraction to ranges.rs, and result printing to report.rs. The two
pieces with real logic, range parsing and output path derivation, are
pure functions with unit tests next to them, so the integration tests do
not have to enumerate their edge cases.

Three options are accepted no-ops, because they name behaviour this crate
does not have: --io-read-method, which has one strategy here, and
--sparse-windows with its negation, since index windows are always dense.
A dense index is valid and interoperable, merely larger, so accepting the
flag lies about nothing.

--no-verify is refused instead. Verification is structural here: a member
is accepted only after its CRC32 and size check out. Accepting the flag
would promise a speedup that does not exist.

Two more refusals replace silent surprises: --import-index without
--ranges, where the index would go unused, and --index-format
gztool-with-lines without --count-lines, where the format has nowhere to
get its counters.

Output goes where rapidgzip sends it, including the derived name with the
compressed suffix stripped, and an existing file is kept unless --force
says otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README.md gains the option examples, the index-format list, and a section
naming the three deliberate differences from rapidgzip: the accepted
no-ops, the refusal of --no-verify, and that this tool always decodes
rather than skipping when output goes to /dev/null.

ARCHITECTURE.md explains where line counting happens and why it cannot
happen in the workers, plus how checkpoint line offsets are resolved.
The crate docs cover count_lines and seek_to_line, and record that a
checkpoint names the DEFLATE start rather than a container header, with
BGZF the exception the .gzi format requires.

CHANGELOG.md gains a Fixed section for the two gztool bugs, which is the
first entry that section has needed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rob-p

rob-p commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Status update after the current-main review and #18

The feature set remains worthwhile, but this stacked branch should not be merged into the current head. #8 and #9 landed through redesigned implementations, the optional-backend stack was not adopted, and several old index/model fixes in this diff are now either obsolete or conflict with the format-neutral current API.

#18 also changes one important CLI decision: --import-index without --ranges is no longer an unused-index error. Once #18 lands, the CLI should use the imported index for strict full-stream parallel decoding.

The clean implementation should be split into two reviewable changes.

1. Line metadata and line seeking in the core

  • Keep counting opt-in and perform it only over final ordered bytes, after marker resolution.
  • Preserve DecodeReport: Copy; an optional scalar line count is compatible with that guarantee and does not put an owning index in the ordinary report.
  • Merge ordered output offsets with authoritative checkpoints from the current DeflateIndex/CheckpointKind model. Do not carry forward the old member-header/payload-offset fixes, because current main already represents both explicitly.
  • Set DeflateIndex::total_line_count and per-checkpoint line_offset only when every value is known. gztool-with-lines export must refuse missing metadata rather than synthesize zeroes.
  • Add line seeking to IndexedReader with a clear zero-based public convention and explicit conversion only at the gztool codec boundary.
  • Cover gzip, concatenated and empty members, BGZF, zlib/raw counting, sequential/marker/streaming paths, no-final-newline input, backward/repeated line seeks, and real gztool interoperability.

This can land independently before the CLI surface.

2. CLI parity on current main

  • Retain the proposed module split: source/output routing, index formats, range parsing, reporting, and attributions should not accumulate in main.rs.
  • Implement flags against current Format, DeflateIndex, IndexedDecodeReport, IndexingDecoderReader, and typed error APIs rather than adapting the old stacked types.
  • Treat compatibility flags in three documented categories: real behavior, harmless syntax aliases, and unsupported semantic requests. Continue rejecting --no-verify. For options such as sparse-window transformation that name behavior we do not implement, an explicit unsupported error is safer than a silent promise unless we can demonstrate that reference scripts depend on the no-op.
  • Keep -d and -k as harmless compatibility aliases because this binary is decode-only and never removes input.
  • Preserve automatic -P 0 as a computed worker budget, then let the runtime controller and application ceiling decide actual allocation.

The index routing should now be:

  • imported index plus byte/line ranges: IndexedReader;
  • imported index plus ordinary full output/test/count: Decode full streams in parallel from an existing index #18's decode_from_index or reader_from_index;
  • export without import: an explicit indexing decode followed by the requested codec;
  • line ranges or gztool-with-lines: require real line metadata and explain its absence;
  • non-seekable input: reject operations requiring positional reuse, while retaining coarse forward index construction only where the current API supports it.

Full-stream imported-index failure must remain strict. A malformed, source-size-mismatched, wrong-format, or exact-boundary-mismatched index should produce IndexDecodeError and a nonzero exit, never fall back and appear successful.

Validation plan

  1. Land the focused core line-count/line-seek PR with unit, property, format, and gztool interop coverage.
  2. Rebuild the CLI modules from current main in a separate PR.
  3. Test every output destination and overwrite rule, stdin restrictions, supported format, index codec, import/export combination, byte and line range form, count mode, error exit, broken pipe, and quiet/verbose behavior through the built executable.
  4. Add a full-output imported-index test that asserts DecoderPath::IndexedParallel, byte identity, and strict rejection of an index for another same-sized archive.
  5. Run fmt, Clippy with warnings denied, full tests, doctests/rustdoc, and the available indexed_gzip/gztool/bgzip interoperability jobs.

I suggest retaining this PR as the design and compatibility reference until the two clean successors are opened, then closing it as superseded rather than attempting to rebase or cherry-pick the stack.

@rob-p

rob-p commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Superseded by #19, a clean implementation based on current main after #18. It retains the line-aware indexing and CLI goals while using DeflateIndex, strict imported-index decoding, all current format paths, explicit rejection of unsupported modes, vectorized ordered counting, and the current scheduler. Thank you for the original design and interoperability work; it directly informed the replacement.

@rob-p rob-p closed this Aug 3, 2026
@BenjaminDEMAILLE
BenjaminDEMAILLE deleted the cli-parity branch August 4, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants