feat(mem_wal): populate what a flushed SSTable holds - #9002
Closed
xuanyu-z wants to merge 2 commits into
Closed
Conversation
A shard manifest entry records only where an SSTable is, not what it contains. Anything a reader wants to know before opening one -- how much payload it holds, how many rows, how much of that is primary key -- costs a read of the generation to find out, which is the read such a reader is deciding whether to make. Add three optional fields to `SsTable`, all accounted on the writer's MemTable at flush: - `in_memory_bytes`: payload size of the rows. - `physical_rows`: rows held, counting the older duplicates of a primary key that the generation's deletion vector masks. - `primary_key_bytes`: total payload size of the primary-key columns over those rows -- a total, not a per-row size, which varies for a variable-length key. All three are estimates of payload rather than bounds on the cost of reading the SSTable: they exclude the per-array structure a reader materializes, and `primary_key_bytes` is not the size of any encoded form of the key. A consumer budgeting memory adds its own headroom. Documented as such, so the contract does not promise more than the numbers support. All three are optional, so an entry written before they existed records none of them and a reader must not treat absence as zero; `primary_key_bytes` is also absent on a table with no primary key. `SsTable::unmeasured` builds such an entry. Flush writes unmeasured entries for now, which is the minimum edit that keeps the library compiling. Populating the fields is a follow-up, per the change process in `protos/AGENTS.md`.
The manifest entry has fields for an SSTable's payload size, row count and primary-key size; flush wrote none of them. Populate all three from the MemTable being written, which is where the numbers exist and where two of them are already maintained. `in_memory_bytes` and `physical_rows` are counters the batch store keeps -- the first is what the flush threshold itself is measured against. `primary_key_bytes` is accumulated where the bloom filter is updated, which already holds the key columns: one size estimate per key column per batch, against a loop that already walks every row. Read off the MemTable while it is frozen, which is load-bearing: `BatchStore::append` bumps these counters before it publishes `committed_len`, and a scan is bounded by `committed_len`, so only a sealed store agrees with what a scan of it will see. `FlushedSize` carries the three together so both flush paths share one measurement and neither the manifest write nor the result can transpose them. A zero reads as absent, which is also how a table with no primary key reports: an empty MemTable is refused before a flush, so a flushed generation always holds rows. Tests read the entry back through `read_version` rather than `latest`. The store caches the manifest it just wrote, so `latest` returns that same Rust value and a test asserting through it passes even for a field that never reached the protobuf -- verified by dropping `primary_key_bytes` from the encode path, which these tests now catch. `FlushFixture` carries the setup all thirteen flush tests repeat.
github-actions
Bot
added
A-format
Contributor
|
Important Format specification voteThis PR modifies the Lance format specification, so it requires 3 binding +1 votes from PMC members (excluding the proposer) and a minimum 72-hour voting period, weekends excluded, before it can merge. Vote by approving this PR (+1) or requesting changes (−1, a veto). See the voting process. Status: ❌ Blocked — 0 of 3 required approvals
Updated automatically by the format-spec vote gate, which re-checks every 15 minutes — just voted? Re-check now (press Run workflow; leave the input blank to re-check every open format PR). A PMC member may apply the |
Contributor
Author
|
Folding this back into #8981 — keeping the change as one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #8981, which adds the fields; that PR's commit appears here until it merges.
The manifest entry has fields for an SSTable's payload size, row count and primary-key size, and flush writes none of them. Populate all three from the MemTable being written, which is where the numbers exist and where two of them are already maintained.
in_memory_bytesandphysical_rowsare counters the batch store already keeps — the first is what the flush threshold itself is measured against.primary_key_bytesis accumulated where the bloom filter is updated, which already holds the key columns in hand: one size estimate per key column per batch, against a loop that already walks every row. So it costs O(columns) on a path that is already O(rows).Read off the MemTable while it is frozen, which is load-bearing:
BatchStore::appendbumps these counters before it publishescommitted_len, and a scan is bounded bycommitted_len, so only a sealed store agrees with what a scan of it will see.FlushedSizecarries the three together so both flush paths share one measurement and neither the manifest write nor theFlushResultcan transpose them.A zero reads as absent, which is also how a table with no primary key reports: an empty MemTable is refused before a flush, so a flushed generation always holds rows.
Testing. The two new tests read the entry back through
read_version, notlatest. That matters: the store caches the manifest it just wrote, solatestreturns the same Rust value and a test asserting through it passes even for a field that never reached the protobuf. Verified by droppingprimary_key_bytesfrom the encode path — the old shape passed, this one fails.FlushFixturecarries the store/shard/flusher setup that all thirteen flush tests in the module repeat; the two new tests use it, and converting the other eleven is a mechanical follow-up.663
mem_waltests pass, 13 of them in the flush module; fmt clean.