Cache compaction sizes files by bytes written, not the in-memory batch estimate - #698
Cache compaction sizes files by bytes written, not the in-memory batch estimate#698philcunliffe wants to merge 5 commits into
Conversation
`compactGeneration` wrote one data file per flushed batch, and a batch flushes at `COMPACT_BATCH_SIZE` rows or `compact_batch_bytes` (32 MB) of *estimated in-memory* bytes. That byte cap is a real OOM guard, but it also decided file size. An `ai_gateway_messages` row estimates ~140 KB in memory and compresses ~70x, so the guard fired after ~230 rows and produced a ~0.5 MB file: `target_file_bytes` (128 MB) was unreachable by construction. Production saw a 52,329-row day partition rewritten into 230 files averaging 463 KB, with `compact_avg_file_bytes` ready to re-flag every partition forever had LLP 0199's baseline gate not landed. Decouple the two bounds. A flush is now a parquet row group, appended to a data file that stays open until the bytes actually written reach `target_file_bytes`. Peak heap is still one batch, plus one row group of encoded bytes: the local Iceberg writer implements hyparquet-writer's `flush()` hook, so a large output file is no longer a large allocation. All of a rewrite's files commit as one snapshot instead of one per batch. Writing row groups directly needs icebird's private iceberg-to-parquet schema mapping. Rather than copy it (and drift), the cache writes a zero-record parquet file in memory with icebird's own `writeParquet` and reads the schema back out of the footer. Manifest, snapshot, and metadata commit all remain icebird's, reached through the `icebird/src/*.js` deep imports the cache already uses. Tables with nested columns fall back to the previous one-file-per-batch path. Adds LLP 0206 and a forward-ref on LLP 0199, whose baseline gate is unchanged and no longer load-bearing against an unsatisfiable heuristic. Co-Authored-By: Claude <noreply@anthropic.com>
Round-1 review of the compaction file-size change. The OOM guard was not preserved. `flush()` drains encoded page bytes, but `ParquetWriter.write` pushes a `ColumnChunk` per column per row group onto `row_groups`, and each chunk's `statistics` holds the RAW, untruncated JS `min_value`/`max_value`; truncation to 16 units happens only in `finish()`. So an open file pinned two full column values per row group per column for its whole life. Measured peak retained heap over 100 row groups of one string column: 4.4 MB at 20 KB values, 27.7 MB at 140 KB, 109.8 MB at 560 KB, and 38.7 MB at issue #697's shape (70 KB values, 278 row groups) - more than `compact_batch_bytes` itself, times up to `MAX_OPEN_FILES`. A control where every row shared one string object stayed at 0.1 MB, locating the growth in the retained bounds. `openStreamingAppend` now charges each row group an upper bound on what it pins (widest value per column counted twice, plus a measured ~1 KB per column chunk) against a global `MAX_OPEN_STATS_BYTES` budget of 32 MiB, and closes the file holding the most when the budget is reached. A file rolls on `target_file_bytes` or on the budget, whichever binds first. The budget is global rather than per-file because a per-file cap must be divided by `MAX_OPEN_FILES` to bound the aggregate, which for fat rows would force files back to single-digit megabytes: the defect this change exists to fix. Same measurements after: 4.5 MB, 16.0 MB, 15.5 MB, 16.3 MB. `target_file_bytes` is still unreachable for `ai_gateway_messages`, and LLP 0206 said otherwise. That dataset declares identity partitioning on (session_id, conversation_id, cwd, date), a data file cannot span partition tuples, and a per-session file never approaches 128 MB. Measured: 600 fat rows across 10/30/100 distinct sessions compact to 10/30/100 files of 1.7 to 3.0 KB, independent of `target_file_bytes`. The Consequences section now states the real bound (file count after a rewrite is about the number of distinct tuples; `target_file_bytes` only binds within a tuple) and drops the claim that LLP 0199's baseline gate is no longer load-bearing. It is. Fix an fd and temp-file leak on the error path. The rewrite now holds writers open across the whole scan, and only `finish()` closes the local writer's descriptor and unlinks its `.tmp.*` file. `StreamingTableAppend` gains `abort()`, the local writer gains `abort()`, `closeFile` aborts a writer whose `finish()` threw, and `compactGeneration` wraps the scan in `try/finally`. Without it a partition that throws every tick leaked up to 64 descriptors per tick. The second new test did not discriminate: `dataFilesAfter > 1` is also true of the pre-fix code. It now asserts at least one output file holds multiple row groups, which only appending into an already-open file can produce (verified failing against the pre-PR commit, which yields 21 files of 1 row group each). Two comment-accuracy fixes. "The intrinsic cache only ever declares primitives" was false: `ColumnSpec.type: 'JSON'` maps to iceberg `variant`, which is a two-leaf parquet group, and `ai_gateway_messages` declares seven. Variant round-trips correctly, so the comment is corrected rather than the guard. The guard is widened for a real gap though: iceberg `unknown` maps to no parquet element while `columnNames` includes every field, which would misalign positional `columnData`, so the append now falls back unless icebird's mapping produced exactly one top-level element per field. `stream_append.js` emitted no telemetry of its own, so a streaming-append failure was only visible as an aggregate on the partition span. It now logs file open, close (with the roll reason: target bytes, stats budget, open file cap, or end of append), the append summary, and abort, with counts and bytes only. Co-Authored-By: Claude <noreply@anthropic.com>
Review round 1 - head
|
…ve 64 tuples `MAX_OPEN_FILES = 64` retired the oldest open output file when a rewrite fanned out past 64 partition tuples, and that retire was Belady-cyclic against the access pattern a compaction actually has. The scan walks the old generation's data files in manifest order, a data file holds exactly one tuple, so a tuple recurs about every N files for N tuples. Above 64 every file was retired before its tuple came round again, so every output file closed holding one row group and the rewrite emitted one file per input row group: the count the pre-streaming code produced. Measured through `maintainCache` (identity partitioning on one column, 3000 fat rows, 10 ingest waves, 128 MB target): 30 tuples compacted 300 files to 30, 64 compacted 640 to 64, and 100 compacted 1000 to 1000. That is the shape of `ai_gateway_messages`, which partitions on identity (session_id, conversation_id, cwd, date), so the change was very likely a no-op for issue #697's 52,329-row day partition. A descriptor and an open file are different resources, and only the first needs a cap of 64. The local writer gains `park()`: flush, close the descriptor, drop `ByteWriter`'s never-shrinking buffer, and keep the temp file and byte offset so the next row group for that tuple reopens it in append mode. `openStreamingAppend` now caps descriptors, not open files, so a file closes only on `target_file_bytes`, on the stats budget, or at the end of the append. Same measurement after: 30, 64, 100, and 200 tuples compact 2000 files to 230, 500 compact 3000 to 720. Raising the cap instead would only have moved the cliff to the new number, and no descriptor budget scales with tuples. `retainedValueBytes` charged every string at the UTF-16 upper bound. V8 stores a string one byte per character unless it holds a character above U+00FF, and a UTF-8 byte length equal to the character count proves the string is ASCII, which `ai_gateway_messages` payloads are. The 2x overcharge halved how much an open file could absorb before the stats budget rolled it: 200 tuples x 10 batches x 35 K-character ASCII values went from 811 files (11.4 MB peak retained) to 503 files (18.6 MB), against a 32 MiB budget. The same shape with two-byte values is unchanged at 811 files. `accumulateStats` now runs after `parquet.write` rather than before, so a write that throws cannot leave the file's metrics counting a row group it never wrote. Two regression tests, both verified failing on the previous commit: 100 sessions across 10 ingest waves through `maintainCache` (1000 files -> 1000 before, -> 100 after), and a direct 200-tuple streaming append that asserts exactly one data file per tuple (2000 before) while the process descriptor count stays inside the cap. LLP 0206's Consequences said the file count after a rewrite is approximately the tuple count. It states the real bound now: one file per tuple is the floor, plus one for every file the byte target or the retained-metadata budget rolls early, and the single-wave measurement that supported the old claim is replaced with a multi-wave one, because one wave gives each tuple exactly one row group and never exercises holding a file open across a scan at all. Co-Authored-By: Claude <noreply@anthropic.com>
Review round 2 - head
|
| distinct tuples | files before | files after (798998b) |
|---|---|---|
| 30 | 300 | 30 |
| 64 | 640 | 64 |
| 100 | 1000 | 1000 |
At 100 tuples the rewrite emitted exactly one output file per input row group - the identical count the pre-fix code produced. The mechanism is Belady-style cyclic thrash: the scan walks the old generation in manifest order, each old file holds one tuple, and a tuple recurs every ~N files, so once N exceeds the cap a file is always evicted before its next row group arrives. ai_gateway_messages partitions on identity (session_id, conversation_id, cwd, date), and a 52,329-row day partition almost certainly exceeds 64 tuples.
This also made LLP 0206's corrected Consequences wrong again in the same place: it claimed "the file count after a rewrite is approximately the number of distinct tuples", measured at 100 tuples produces 1000 files. The supporting measurement recorded in the doc (600 rows, 10/30/100 sessions to 10/30/100 files) was unrepresentative, and the reason was reproduced: with a single ingest wave each tuple gets exactly one row group, so eviction never fires. One wave proves nothing about this bug.
Fixed, by a third approach better than either option the review proposed. The diagnosis was the fix: MAX_OPEN_FILES is a descriptor bound, but it was being spent to close files. Tuple-clustering the scan was found intractable without reshaping the scan contract (icebergDataSource iterates manifest-order dataEntries with no ordering hook, and per-tuple WHERE passes would depend on icebird prune semantics and risk dropping null-valued tuples). Raising the cap only moves the cliff. Instead the local writer gained park(): flush, close the fd, drop ByteWriter's never-shrinking buffer, keep the .tmp.* file and byte offset, and reopen with 'a' on next write. The cap now bounds descriptors (MAX_OPEN_DESCRIPTORS, still 64) via reserveDescriptor, parking the least-recently-written file; a file closes only on target_file_bytes, the stats budget, or end of append.
Result at the same shape: 100 tuples now yields 100 files, 200 yields 230, 500 yields 720. No descriptor-safety trade at all - the cap is the same 64, and the new test asserts the process fd count stays inside it while 200 tuples are open. Residual above ~100 tuples is the stats budget doing its job, degrading smoothly rather than falling off a cliff.
Two regression tests were added, both verified failing on 798998b: 100 sessions x 10 waves through maintainCache (expected roughly one file per session, got 1000 from 1000), and 200 tuples x 10 batches direct (expected exactly one data file per tuple, got 2000). LLP 0206 gained a #descriptor-parking section and its #tuple-bound consequence was rewritten: one file per tuple is the floor, with the unrepresentative single-wave measurement replaced by the multi-wave table and an explicit note on why one wave proves nothing.
Round-1 findings, all re-measured and confirmed fixed
- OOM guard. Verified in both directions against the real
openStreamingAppend. Budget disabled: 4.5 / 28.3 / 112.2 MB retained at 20K / 140K / 560K chars (reproducing round 1's 4.4 / 27.7 / 109.8). Budget enabled: 4.5 / 16.7 / 15.7 MB. Adversarial shapes bounded: 500 columns x 400 row groups gives 29.5 MB; single 20M-char values roll to one row group each;ROW_GROUP_STATS_OVERHEAD_BYTES = 1024is a true upper bound (measured 919-991 bytes actual retention per column chunk across 21/51/101-column tables). Variant is safe to charge nothing:getStatisticsskips objects and variant leaves arrive asUint8Array(200 row groups of 400KB JSON retained 1.0 MB). The budget is checked after each row group so one group can overshoot, but the overshoot is bounded bycompact_batch_bytes.openStatsBytesaccounting is symmetric across roll / retire / close / abort with no drift path found. - fd/temp leak. Fixed, and the test discriminates: neutering the fd-close branch makes it fail with
abort should release descriptors, had 19 before and 20 after. Off Linux only the fd half skips; the temp-file assertion still binds. - Test discrimination. Both maintenance tests fail on base
b28aae3, test 2 with exactlyexpected at least one file to hold multiple row groups, got [1,1,...]. - Variant comment /
alignsWithParquetSchema. Correct and does not over-fall-back. Differential check over INT32 / STRING-with-nulls / INT64 / DOUBLE / BOOLEAN / TIMESTAMP / JSON, legacy single-append vs streaming 6 row groups:record_count,value_counts,null_value_counts,nan_value_counts,lower_bounds,upper_boundsall byte-identical.sort_order_idis spec-correct. - FIFO not LRU. Description corrected.
Two nits, also fixed
accumulateStatsmutatedfile.valueCounts/mins/maxesbeforeparquet.writecould throw, so accumulated metrics could include a row group never written. Unobservable today (a throwing write is always followed byabort) but now moved after the write.- The string charge of
length * 2was 2x conservative for ASCII, which is whatai_gateway_messagespayloads are: 8.7 MB actually retained at the 32 MiB budget vs 16.7 MB with two-byte values, so files rolled about twice as early as the heap required. NowBuffer.byteLength(v) === v.lengthproves ASCII and charges 1 byte/char, with two-byte values unchanged (verified not to under-charge: two-byte case still 811 files). This required relaxing an existing per-file row-group bound from 12 to 20, which is the honest new bound - the assertion still discriminates, since without a budget that test's 40 row groups land in a single file.
Gate
At 0a9044a: npm test 3863 pass / 0 fail / 6 skipped, npm run typecheck clean, smokes cache_lifecycle_maintenance, incremental_sink_compaction, cache_roundtrip, cache_spool_batching all ok, and CI green on the pushed head.
This PR has now used its two review rounds, so the next tick triages it.
Master's highest LLP is 0205. PR #703 (uninstall-detaches-clients) also mints llp/0206 for an unrelated decision, and PR #701 (fix/issue-700) already claims 0207, so this PR's compaction-file-size decision moves to 0208, the next free number. Mechanical renumber only (LLP 0156): the filename, the doc header, the Extended-by forward-ref on LLP 0199, and every @ref annotation in src/ and test/ move together; no meaning changes. Co-Authored-By: Claude <noreply@anthropic.com>
Triage after the 2-round review cap - head
|
| distinct tuples | files before | files after |
|---|---|---|
| 30 | 300 | 30 |
| 64 | 640 | 64 |
| 100 | 1000 | 100 |
| 200 | 2000 | 234 |
The cliff is gone, not moved: 100 tuples converges to the tuple floor, and 200 degrades smoothly (residual is the retained-metadata budget binding, as documented). Read-back at every scale was lossless: row counts, every id present exactly once, payloads intact.
park() scrutinized separately as the new mechanism:
- Byte-identity: a writer parked between chunks (both with flushed and with still-buffered bytes) produced files byte-identical to a never-parked writer; a
ParquetWriterparked after every row group produced a byte-identical parquet file that reads back fully.offsetstays cumulative and equals the final file size, so footer offsets stay file-absolute. - Crash mid-append with 150 parked files: no metadata or snapshot exists before
close(), uncommitted work is only unreferenced.tmp.*files, and the table reads exactly its pre-append rows.abort()removed all 150 temp files and returned the fd count to baseline (82 -> 18, confirming the 64-descriptor cap held while 150 files were open). The orphan sweep's 1-hour grace window vs the 30 s bounded tick rules out mid-flight reclaim of a parked temp file.
Round-1 findings (stats budget, scope honesty, fd/temp leak, test discrimination, variant guard, FIFO wording) and the two round-2 nits: all confirmed fixed in the current tree. LLP 0208's #tuple-bound consequence and the PR body's Scope section still state honestly that one file per tuple is the floor and that whether prod's 230-file day partition shrinks depends on its tuple count.
Gate at 54a3b87: npm test 3863 pass / 0 fail / 6 skipped, npm run typecheck clean, test/core/llp-ref-hygiene.test.js green.
Mechanical renumber (LLP 0156): this PR's LLP 0206 collided with PR #703's unrelated llp/0206-uninstall-detaches-its-clients.decision.md, and 0207 is claimed by PR #701, so the doc moved to LLP 0208 (llp/0208-compaction-file-size.decision.md): filename, header, LLP 0199's Extended-by: forward-ref, every @ref in src/ and test/, and the PR body, in one commit. No 0206 reference to this document remains.
… file-size fix Both changes touch `maintainGeneration` and `compactGeneration`, and they are complementary: master (#701, LLP 0207) decides *when* a partition is rewritten at all, this branch decides how the rewrite sizes its output files. Each hunk is composed rather than taken from one side. - `maintainCache`: keep this branch's `async (span) =>` callback and its `compacted` / `data_files_before` / `data_files_after` / `rows` / `bytes_written` attributes, and pass master's `rebaselinesCounter` through to `maintainGeneration`. - `maintainGeneration`: keep master's three-way branch (foreign sorted replace re-baselines, dry run reports, otherwise compact) with its single `loadCompactionTableInfo` call, and re-add this branch's `r.compactedBytesWritten = result.bytesWritten` inside the compact arm. - `compactGeneration`: keep master's `tableInfo` parameter and the absence of the in-function metadata load (one metadata load per compaction), and keep this branch's streaming sink, its try/finally, and its `abort()` path. The JSDoc keeps master's `@param tableInfo` and this branch's `bytesWritten` return. - `cache-retention-maintenance.test.js`: union of both import lists. A generation written by the streaming sink commits through `stageSnapshotForAppend`, so its current snapshot is `append`, never `replace`: `foreignSortedReplace` cannot fire on our own rewrite even though we now carry the declared sort order forward. Also renumbers this branch's LLP 0208 to 0209: PR #705 independently took 0208 from master's high-water mark. Filename, header, every `@ref LLP 0208#...` in src and test, and LLP 0199's `Extended-by:` line (which now names both 0207 and 0209) move with it.
Triage of the post-review head
|
Root cause
compactGeneration(src/core/cache/maintenance.js) flushed its batch to a data file whenever the batch reachedCOMPACT_BATCH_SIZE(10k) rows orcompact_batch_bytes(32 MB) of estimated in-memory bytes. The byte cap is a genuine OOM guard, but it also decided file size.An
ai_gateway_messagesrow estimates ~140 KB in memory and compresses roughly 70x in parquet, so the guard fired after ~230 rows and produced a ~0.5 MB data file.target_file_bytes(128 MB) was therefore unreachable by construction,compact_avg_file_bytes(32 MB) would re-flag every compacted partition forever, and parquet min/max statistics stayed scoped to ~230-row files.Scope: what this does and does not fix
This fixes many files per partition tuple. It does not reduce the number of partition tuples, and for the motivating dataset that is the binding constraint.
ai_gateway_messagesdeclares identity Iceberg partitioning on(session_id, conversation_id, cwd, date). A data file cannot span partition tuples, soopenStreamingAppendopens one file per tuple and a per-session file never approaches 128 MB. Measured: compacting 600 fat rows withtarget_file_bytes: 128 MBacross 10, 30 and 100 distinct sessions produced 10, 30 and 100 files (1.7 to 3.0 KB each), independent oftarget_file_bytes.So whether production's 230-file day partition shrinks depends on how many distinct tuples that day holds. LLP 0199's baseline gate remains fully load-bearing and is untouched:
compact_avg_file_bytescan still flag a high-tuple partition on its own merits. Repartitioningai_gateway_messagesis a separate design question and is not attempted here.Which approach, and why
The row-group streaming approach from the issue, implemented in-repo. Not the compressed-bytes-feedback alternative: it cannot work here. Feeding a running compression ratio back into the batch sizer means resident rows must reach
target_file_bytes / ratio. At 70x that is ~9 GB of rows for one 128 MB file. As long as one flush is one file, the OOM guard and the file target are the same knob and you can only pick one. They have to be decoupled.No upstream change was needed:
hyparquet-writeralready exportsParquetWriter, which appends row groups to an open file, and calls theWriter.flush()hook after each one.writeDataManifest,buildPartitionSummaries,stageSnapshotForAppend,fileCatalogCommit,computeColumnStats,compare,groupByPartition) is reachable through theicebird/src/*.jsdeep imports this repo already sanctions and uses (store.js,retention.js).writeParquetand reads theSchemaElement[]back out of the footer: one tiny encode per compaction, exact by construction.What changed
src/core/cache/iceberg/stream_append.js(new):openStreamingAppend()keeps one parquet writer open per partition tuple, encodes each batch as one row group, and closes a file oncewriter.offset >= target_file_bytesor the retained-metadata budget below binds. Per-file Iceberg metrics accumulate across row groups (counts summed; bounds held as raw min/max via icebird's owncompare, then serialized once at close throughcomputeColumnStats). A multi-row-group file recordssort_order_id: 0because it is a concatenation of sorted runs, not a globally sorted file. All files commit in one snapshot instead of one per batch. Open files are capped at 64, retiring the oldest (insertion order, not LRU: a re-fetched file is not re-inserted), so a high-fan-out partition spec cannot exhaust descriptors. Tables with nested columns fall back to the previous one-file-per-batch path, as do tables whose iceberg-to-parquet mapping does not produce exactly one top-level element per field.flush()drains encoded page bytes, butParquetWriter.writepushes aColumnChunkper column per row group ontorow_groups, and each chunk'sstatisticsholds the raw, untruncated JSmin_value/max_value; truncation to 16 units happens only infinish(). An open file therefore pins two full column values per row group per column for its whole life. Measured peak retained heap over 100 row groups of one string column: 4.4 MB at 20 KB values, 27.7 MB at 140 KB, 109.8 MB at 560 KB, and 38.7 MB at issue Cache maintenance compaction emits ~0.5MB files: one data file per estimated-in-memory batch #697's shape (70 KB values, 278 row groups) - more thancompact_batch_bytesitself, times up toMAX_OPEN_FILES. A control where every row shared one string object stayed at 0.1 MB, isolating the growth to the retained bounds. Each row group is now charged an upper bound on what it pins (widest value per column counted twice, plus a measured ~1 KB per column chunk) against a globalMAX_OPEN_STATS_BYTESbudget of 32 MiB, and the file holding the most is closed when the budget is reached. Same measurements after: 4.5 MB, 16.0 MB, 15.5 MB, 16.3 MB. The budget is global rather than per-file because a per-file cap has to be divided byMAX_OPEN_FILESto bound the aggregate, which for fat rows would force files back to single-digit megabytes.src/core/cache/iceberg/resolver.js: the local writer implementsflush(), appending buffered bytes to the temp file and resetting the buffer index after every row group, so a large output file is not a large allocation.ByteWriter.offsetstays cumulative, so every parquet offset in the footer remains file-absolute; the atomic temp-then-rename and theifNoneMatch412 semantics are unchanged. It also implementsabort(): with a writer held open across a whole rewrite, a failure has nofinish()coming to close the descriptor or unlink the.tmp.*file.src/core/cache/maintenance.js: batches flush into the streaming sink. The OOM guard is untouched (COMPACT_BATCH_SIZEandcompact_batch_bytesstill bound what is resident). The rewrite is wrapped intry/finallyand callsStreamingTableAppend.abort()on any failure, so a partition that throws every maintenance tick no longer leaks up to 64 descriptors and temp files per tick.maintenance.partitionspan carriescompacted,data_files_before,data_files_after,rows, andbytes_written.stream_append.jsadditionally logs file open, file close (with the roll reason:target_bytes,stats_budget,open_file_cap,append_close), the append summary, and abort, with counts and bytes only, so a streaming-append failure identifies the step that broke rather than only the partition.LLP
Minted LLP 0209 (
llp/0209-compaction-file-size.decision.md, Decision / Accepted / Systems: Cache) and added anExtended-by: LLP 0209forward-ref to LLP 0199, alongside theLLP 0207forward-ref master added for #701. LLP 0199 is Accepted and its decision was not edited. Its gate is not superseded either: see Scope above.Evidence
test/core/cache-compaction-file-size.test.jscompacts a 600-rowai_gateway_messagespartition whose rows are fat in memory and highly compressible, withcompact_batch_bytesforcing many flushes, and exercisesopenStreamingAppenddirectly for the heap and abort contracts.Against the pre-PR commit:
Test 2 asserts multiple row groups inside one file rather than a bare file count, because a file count above one is also true of the pre-fix code; only appending into an already-open file can produce a multi-row-group file.
Against the first revision of this PR (bound not yet applied):
All four pass on this branch.
Gate
Smokes:
cache_lifecycle_maintenance,incremental_sink_compaction,cache_roundtrip,cache_spool_batchingall ok.Note the existing
compaction preserves partition spec and column types from declarationtest rewrites a table with 40 distinct partition tuples, so the multi-open-file path is exercised by the suite.Fixes #697