Uh oh!
There was an error while loading. Please reload this page.
Reach the fast Parquet codec into the cache, and prune before opening the source - #1063
Conversation
… the source (#1057) Follow-up on the deferred findings from PR #1048. (a) Cache writes used the JS snappy fallback. `stream_append.js` built its `ParquetWriter` with a `codec` but no `compressors`, so hysnappy reached only the sink export encoder while the compaction rewrite - the largest single producer of parquet bytes in the process - kept hyparquet-writer's own JS implementation. The shared WASM instance moves out of the format-parquet plugin into `src/core/util/parquet_snappy.js` so both write paths use ONE instance: two would be two WASM memory floors, each sized to its own worst page, for no gain over one sized to the worse of the two. Evidence to the standard PR #1048's own codec change met. hysnappy returns `byteArray.slice(...)`, a copy out of WASM memory rather than a view, so the writer may hold a page across later calls; `ParquetWriter` merges `{ SNAPPY: <js fallback>, ...compressors }`, so only SNAPPY is displaced and UNCOMPRESSED tables are untouched. Files written through the cache path round-trip under hyparquet 1.29.2 AND under 1.28.2 (a user who downgrades), through hyparquet's built-in snappy and through hyparquet-compressors, over all-null, one-byte, incompressible, highly-compressible and >1 MiB pages. Framing is raw snappy blocks: an independent raw-block decompressor reads them. (b)(c) The indexed grep tier gets its "index pruned everything" fast path back. `queryIndex` runs against the sidecar first, and the source is opened - and its footer read for the physical projection - only once a candidate block survives. A fully-pruned file previously paid a stat plus a 512 KiB footer slice to prove it had nothing, and pruning to nothing is the common case for the selective query this tier exists to make fast. It also narrows the ENOENT window back: a compaction or purge that unlinks a data file mid-walk can no longer fail a query that never needed to read it. The cost is one duplicated in-memory posting decode on files that DO have candidates, because `parquetFind` re-runs `queryIndex` and takes no way to be handed the result; the index footer is parsed once either way, since the metadata rides back into `parquetFind`. (d) hyparquet is deduped again. icebird 0.8.25 began declaring its own exact hyparquet, so npm nested a second copy and LLP 0222 #hyparquet-floor's "a single deduped copy shared with icebird" quietly stopped being true. Rather than record the drift, an `icebird.hyparquet` override restores it, which is close to free: hyparquet 1.29.1 -> 1.29.2 changed package.json alone (added `default` export conditions), with no `src/` delta at all. Two tests hold it, deliberately separate from the floor tests, because dedupe is hygiene and the floor is correctness. No LLP is edited. 0222's claim is true again rather than corrected, and the grep change restores the IO story 0303/0304 already describe. Tests, each failing on master and passing here: - cache-write-codec.test.js, both tests - search-grep-service.test.js, "a file the index prunes to nothing is answered without opening it" - hyparquet-floor-pin.test.js, the two new dedupe tests Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The module header claimed the instance is "shared by every parquet WRITE path in this repo". It is not, and the PR that added it says so in its own body: everything routed through icebird's `writeParquet` takes a `codec` but no `compressors` at all, which covers cache ingest FLUSH (`partition.js` -> `appendRowsToTable`) and `stream_append.js`'s own `legacyAppend` fallback for a table it cannot stream into; and the grep sidecar build is hypgrep's `createIndex`, which builds its own `ParquetWriter` inside the library. This is the canonical explanation for why the compressor is a singleton and where the speedup lands, so a reader who trusts it would conclude the whole cache write path got faster when only the streamed compaction rewrite did. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
philcunliffe
commented
Aug 28, 2026
Verdict: findings (1 low, fixed) — the correctness core holds up under independent re-derivationReviewed at I did not take the PR's evidence matrix on trust. Re-derived independently: The WASM aliasing question — confirmed, twice, two waysThe installed hysnappy is 1.1.1 ( constbyteArray=newUint8Array(memory.buffer)// re-derived AFTER memory.grow...returnbyteArray.slice(outputStart,outputStart+compressedSize)Two things matter and both hold. Empirically, against the installed module: held a returned page, then ran three further compressions including a 4 MiB input that forces I also checked the cross-call hash table, since that is the other way a shared instance could corrupt. The WASM stores match candidates as offsets from a fixed input base and then re-verifies each with a 32-bit load against the current input plus a Interleaving: the sharpest case is two writers alive at once through the one instance. Opened two Also probed for a size ceiling: 1 / 8 / 64 / 200 MiB pages and a 120 MiB repetitive page all compress and round-trip. Round-trip matrix I actually ranWritten through
Shapes: empty table (0 files), all-null, one-byte values, incompressible noise, highly compressible, pages pushed past 1 MiB, a single 3 MB cell, empty strings, multi-byte unicode. Framing is raw blocks, verified directly rather than inferred: the output carries no
|
philcunliffe
commented
Aug 28, 2026
Verdict: clean — no findings. Round 1's Low is fixed, and the failure paths round 1 did not press hold up under injected faults.Reviewed Round 1 was thorough, so I treated its conclusions as claims and spent the effort where it did not look: fault injection into the write path, the fallback writer, module-load behaviour, and grep concurrency. Aliasing — spot-checked, holdsNot the full matrix, one targeted probe against the installed hysnappy 1.1.1: held a returned page, then compressed a 6 MiB input (forces Also re-checked the small end, where a length-prefix bug would corrupt silently: 0, 1, 2, 3, 4, 63, 64, 65-byte pages all round-trip, and all but the 1000-byte one are byte-identical to the writer's JS snappy. Failure and partial-write paths — patched hysnappy to throw on the Nth pageThe question that matters for a cache write path is whether a mid-stream codec failure can land a corrupt or half-written data file. It cannot, for two independent reasons, both verified rather than argued. Nothing is written in place. Injected throw mid-
Injected throw after files had already rolled ( Instantiation failure.
|
philcunliffe
commented
Aug 28, 2026
Ship risk: |
Uh oh!
There was an error while loading. Please reload this page.
Follow-up on the five deferred findings from the review of PR #1048. Three warranted a change; two did not.
(a) The codec speedup now reaches cache writes
src/core/cache/iceberg/stream_append.jsbuilt itsParquetWriterwith acodecbut nocompressors, so hysnappy reached only the sink export encoder and the cache kept hyparquet-writer's own JS snappy.A correction to the issue's framing first, because it changes what the fix covers:
openStreamingAppendhas exactly one caller,maintenance.js's compaction sink. Ingest FLUSH goespartition.js->appendRowsToTable-> icebird'swriteParquet, which accepts nocompressorsat all and cannot be wired without an upstream icebird change. So this reaches the compaction rewrite path, not ingest. That is still the largest single producer of parquet bytes in the process (a rewrite re-encodes every live row of a table), so it is worth having, but "cache ingest" over-states it.The shared WASM instance moves out of the format-parquet plugin into
src/core/util/parquet_snappy.js, so both write paths use one instance. Two would be two WASM memory floors, each sized to its own worst page, for no gain over one sized to the worse of the two. Instantiation is lazy so importing the module does not build a WASM instance in a process that never writes parquet.Evidence, to the standard #1048's own codec change met. The blast-radius worry was silent corruption at write time, surfacing only on read, so each leg is proven rather than argued:
byteArray.slice(outputStart, ...)- a copy out of WASM memory, not a view into it - so the writer may hold a page across later calls. This mattered here and not for the sink encoder: the cache's writer is a customAbortableWriterthat buffers across row groups.ParquetWritermerges{ SNAPPY: <js fallback>, ...compressors }, so only SNAPPY is displaced. AnUNCOMPRESSEDtable (write.parquet.compression-codec) is untouched, as is thecodec: undefineddefault.hyparquet-compressors, over all-null pages, one-byte values, incompressible noise, highly compressible text, and values fat enough to push pages past the writer's 1 MiB default. The 1.28.2 leg used a scratch install outside the worktree.Measured, with the honest numbers. The codec itself is much faster: on a 1 MiB textual page, 1.35 ms vs 5.69 ms (4.2x); on a 1 MiB random page, 0.14 ms vs 1.48 ms, and 59 KB out vs 209 KB. End to end it is smaller than that suggests, because compression is a modest share of a compaction's work. A 40,000-row text-heavy compaction, 6 runs each, medians: 2,971 ms vs 3,058 ms (~3% faster) and 9,151,404 vs 9,227,960 bytes (~0.8% smaller). Directionally consistent with the microbenchmark (~140 ms of compression saved on ~32 MB of pages). Real, not dramatic.
(b) and (c) The indexed grep tier gets its fast path back
A footer-free restructuring does exist, on hypgrep's public API.
queryIndexis exported and reads only the sidecar;parquetFinddoes not touch the source until after its own internalqueryIndexhas pruned. What forced the ordering was purely the call site:columnshas to be a resolved array at call time, and computing it needs the source footer.So
queryIndexnow runs first, and the source is opened - and its footer read for the physical projection - only once a candidate block survives. A fully-pruned file previously paid astatplus a 512 KiB footer slice (parquetMetadataAsync's default initial fetch) to prove it had nothing, and pruning to nothing is the common case for exactly the selective query this tier exists to make fast. On a cache of 128 MiB compacted files, a query that prunes everything read ~512 KiB per file where it previously read zero.That closes (c) at the same time: the source is not opened at all for pruned files, so a compaction or purge that unlinks a data file mid-walk can no longer fail a query that never needed to read it. The ENOENT window narrows back to where it was before #1048.
The cost, stated at the helper:
parquetFindre-runsqueryIndexand takes no way to be handed the result, so a file that does have candidates decodes its posting bitsets twice. That is CPU over a bufferio.readeralready made resident, with no second read, and the index footer is parsed once either way because the metadata rides back intoparquetFind. Only a definite "no blocks" shortcuts; every other outcome, failures included, falls through to the existing path, so a poisoned or unreadable sidecar degrades exactly where it did before with the same warning.No LLP is minted: this restores the IO story LLP 0303
#memory-boundand 0304#indexed-tier-residencyalready describe, rather than settling anything new. Both areAcceptedand neither is edited.(d) hyparquet is deduped again, so LLP 0222's claim is true rather than corrected
Re-derived against current master with a real
npm install(this repo checks in no lockfile):The drift predates #1048: icebird 0.8.25 (#982, 2026-08-21) began declaring its own exact hyparquet, npm nested a second copy, and nothing reddened because both copies sit above the 1.28.2 floor and only a below-floor copy changes an answer. LLP 0222
#hyparquet-floor's "resolving to a single deduped copy shared with icebird" was true when written and stopped being true silently.The honest fix turned out to be the dependency change rather than the doc note, because it is nearly free:
diff -rqbetween the hyparquet 1.29.1 and 1.29.2 tarballs shows package.json as the only differing file (it addeddefaultexport conditions; devDependency bumps). The override moves icebird between two byte-identicalsrc/trees. So anicebird: { "hyparquet": "1.29.2" }override restores the property the LLP records, and noAccepteddoc is edited - its claim is true again.Two tests hold it, deliberately separate from the existing floor tests, because dedupe is hygiene and the floor is correctness: a dedupe failure must not read as a wrong-rows failure. The comment states the direction of the remedy - if a dependency ever declares a hyparquet above the root pin, move the root pin up, never hold the dependency down onto an older reader.
hypvector's nested 1.26.x copies are out of scope, as
hyparquet-floor-pin.test.jsalready documents: it is an optionalDependency, write-side/vector-side, and does not run icebird's converter.(e) No change
physicalProjection'scolumns: []dependency is documented at the helper and pinned at the library seam bytest/core/cache-iceberg-schema-evolution.test.js, "a projection narrowed to nothing still reads one row object per physical row". That test asserts the behaviour directly against hyparquet rather than through its effect, and names itself on failure, so a bump that reinterprets an empty projection reddens by name instead of surfacing as a purge that reports success and spares rows. The pin is sufficient; nothing further is owed unless upstream documents or changes the behaviour.Tests
Each fails on
origin/masterand passes here, proven by reverting the source change and re-running:cache-write-codec.test.js: "the cache compaction writer compresses pages with hysnappy, not the writer fallback"page of 1600 plaintext bytes was not hysnappy's encoding (stored 1605, hysnappy 1603, fallback 1605)cache-write-codec.test.js: "every page shape the cache writes reads back through the query path decompressor"search-grep-service.test.js: "a file the index prunes to nothing is answered without opening it"ENOENT: no such file or directory, stat '.../data/....parquet'hyparquet-floor-pin.test.js: "icebird is held at the root hyparquet pin, not left to nest its own"hyparquet-floor-pin.test.js: "no root dependency nests a hyparquet of its own"icebird/node_modules/hyparquet is hyparquet@1.29.1, beside the root 1.29.2The first test's
divergentcounter is what gives it teeth: the two snappy implementations agree byte for byte on short and repetitive pages, so without proof that at least one page distinguishes them the assertion would pass on either wiring.Green:
npm test(5,397 pass / 0 fail),npm run typecheck,npm run build:types,npm pack --dry-run, and the smokesquery_grep_roundtrip,cache_lifecycle_maintenance,cache_roundtrip,local_parquet_export,purge_removes_cached_rows,incremental_sink_compaction,gateway_claude_capture.Fixes#1057