feat(mem_wal): record what a flushed SSTable holds - #8981
Conversation
github-actions
Bot
added
A-format
|
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 |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The outer shard-manifest is a reasonable place for pre-read generation metadata, but the byte value needs a defensible admission contract and the persisted format must be reviewed independently of writer behavior. A viable sequence is a focused protobuf plus format-spec proposal first, followed by the flush implementation and its storage-roundtrip and decoded-memory tests after that contract lands.
Please mark this PR with the breaking-change label.
| // Zero is recorded as unmeasured rather than as a size: an empty | ||
| // memtable is rejected above, so a flushed generation always holds | ||
| // rows, and a zero here can only mean the accounting failed. | ||
| let in_memory_bytes = Some(memtable.batch_store().row_bytes() as u64).filter(|b| *b > 0); |
There was a problem hiding this comment.
BatchStore::row_bytes() is a logical slice estimate, not a conservative materialization bound, so this value does not satisfy the new protobuf promise that it “covers a read that materializes all” rows. A compactor admitting a run from this field can substantially overshoot a hard memory budget. Either define the field as a non-binding payload estimate whose consumer must add structural/headroom costs, or record and verify a conservative decoded-memory bound.
Reproducer run against the workspace Arrow 58.4.0
use std::sync::Arc;
use arrow_array::{Array, ArrayRef, Int32Array, RecordBatch};
use arrow_schema::{DataType, Field, Schema};
fn main() {
let columns = 10_000;
let fields = (0..columns)
.map(|i| Field::new(format!("c{i}"), DataType::Int32, false))
.collect::<Vec<_>>();
let arrays = (0..columns)
.map(|_| Arc::new(Int32Array::from(vec![1])) as ArrayRef)
.collect::<Vec<_>>();
let batch = RecordBatch::try_new(Arc::new(Schema::new(fields)), arrays).unwrap();
let recorded = batch.columns().iter()
.map(|col| col.to_data().get_slice_memory_size().unwrap())
.sum::<usize>() + std::mem::size_of::<RecordBatch>();
println!("recorded={recorded} arrow_physical={}", batch.get_array_memory_size());
}Observed: recorded=40040 arrow_physical=1000000 (~25× undercount; Arrow’s physical figure still excludes schema and RecordBatch ownership).
|
|
||
| // Read back through the manifest, not from the flush result: the | ||
| // protobuf round-trip is the part a later reader depends on. | ||
| let manifest = manifest_store.latest().await.unwrap().unwrap(); |
There was a problem hiding this comment.
latest() on this same ShardManifestStore returns its cached Rust value: after encoding and writing, write() caches the original manifest. This assertion therefore never deserializes the persisted protobuf, and it would still pass if either new field were removed from From<pb::SsTable>. Read the committed version through read_version() (or a fresh store) before asserting both values, and cover an older payload where both fields are absent. Persisted round-trip behavior is the feature this test needs to protect.
| // could not measure the memtable. It is the size of the rows written, so it | ||
| // covers a read that materializes all of them; a read applying the | ||
| // generation's deletion vector materializes less. | ||
| optional uint64 in_memory_bytes = 3; |
There was a problem hiding this comment.
SsTable is embedded in the persisted shard manifest, so these fields are a format-specification change. The repository contract requires a proposal containing the protobuf, matching docs/src/format/ text, and only the minimum compilation edits; writer behavior belongs in a follow-up. This PR changes both flush paths while docs/src/format/table/mem_wal.md still says an entry records only generation and path, leaving the durable estimate/presence semantics outside the standalone contract voters review. Please split the flush implementation and test into a dependent PR and document those semantics in the format proposal.
lance-gatekeeper
Bot
added
the
K-changes
A flushed SSTable records neither its size nor its row count, and neither can be had cheaply afterwards. The files on storage are encoded, so their size understates what materializing the rows costs by whatever the columns compressed by, and the row count needs the generation opened to read. A caller weighing a generation before it reads one has nothing to weigh. Record both on the manifest entry at flush, off the memtable being written: - `in_memory_bytes` from the store's own row accounting, which is already what the flush threshold is measured against. It is the window being written, so it describes a reader that materializes every row. - `physical_rows` in the same sense as `DataFragment.physical_rows`, counting the older duplicates the generation's deletion vector masks -- an over-estimate of what a deduped scan yields rather than an under-estimate. Both are optional. An SSTable flushed before these fields existed carries neither, so a reader has to handle the unmeasured case rather than reading a missing value as zero; a zero measurement is recorded as absent for the same reason, since an empty memtable is already refused and a flushed generation therefore always holds rows. `SsTable::unmeasured` constructs an entry for the paths that build one without a measurement.
aebad10 to
210b404
Compare
lance-gatekeeper
Bot
removed
the
K-changes
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The current revision leaves the three acceptance gaps unchanged. The persisted estimate still needs explicit semantics that are safe for its intended pre-read budgeting use, and the format proposal needs to be independently reviewable and proven through storage rather than the writer's cache.
A viable sequence is a focused protobuf plus matching format-spec proposal with precise estimate and presence semantics and only compilation edits, followed by the flush implementation and storage-roundtrip and decoded-memory tests after that contract lands.
Please mark this PR with the breaking-change label.
| Self { | ||
| // `row_bytes`, not the store's retained heap: the window being | ||
| // written is what a reader of this generation gets back. | ||
| in_memory_bytes: Some(memtable.batch_store().row_bytes() as u64).filter(|b| *b > 0), |
There was a problem hiding this comment.
FlushedSize::of still persists BatchStore::row_bytes() as in_memory_bytes, while the protobuf still calls this the decoded size for pre-read estimation. row_bytes() intentionally measures logical buffer windows and omits the structural cost of materialized Arrow arrays, so it is not a conservative admission bound: a consumer using it as the cost of opening a generation can overshoot its memory budget. Please either define and rename the durable field as a non-binding payload/window estimate whose consumers must add headroom, or record and test a conservative decoded-memory bound. This is the current projection of the earlier finding.
Reproducer run against the current head's Arrow 58.4.0
use std::sync::Arc;
use arrow_array::{Array, ArrayRef, Int32Array, RecordBatch};
use arrow_schema::{DataType, Field, Schema};
fn main() {
let columns = 10_000;
let fields = (0..columns)
.map(|i| Field::new(format!("c{i}"), DataType::Int32, false))
.collect::<Vec<_>>();
let arrays = (0..columns)
.map(|_| Arc::new(Int32Array::from(vec![1])) as ArrayRef)
.collect::<Vec<_>>();
let batch = RecordBatch::try_new(Arc::new(Schema::new(fields)), arrays).unwrap();
let recorded = batch.columns().iter()
.map(|col| col.to_data().get_slice_memory_size().unwrap())
.sum::<usize>() + std::mem::size_of::<RecordBatch>();
println!("recorded={recorded} arrow_physical={}", batch.get_array_memory_size());
}Observed: recorded=40040 arrow_physical=1000000 (~25× undercount; Arrow's physical figure still excludes schema and RecordBatch ownership).
lance-gatekeeper
Bot
added
the
K-changes
1 participant
A flushed SSTable records neither its size nor its row count, and neither can be had cheaply afterwards. The files on storage are encoded, so their size understates what materializing the rows costs by whatever the columns compressed by, and the row count needs the generation opened to read. A caller weighing a generation before it reads one has nothing to weigh.
Record both on the manifest entry at flush, off the memtable being written:
in_memory_bytesfrom the store's own row accounting, which is already what the flush threshold is measured against. It is the window being written, so it describes a reader that materializes every row.physical_rowsin the same sense asDataFragment.physical_rows, counting the older duplicates the generation's deletion vector masks — an over-estimate of what a deduped scan yields rather than an under-estimate.Flat scalars on the entry, matching how the format already carries per-object stats (
DataFragment.physical_rows,DataFile.file_size_bytes,DeletionFile.num_deleted_rows) rather than grouping them into a nested message.Both are
optionalrather than following the older zero-means-unknown convention ofDataFile.file_size_bytes, so the unmeasured case is a type a reader has to handle. An SSTable flushed before these fields existed carries neither; a zero measurement is recorded as absent for the same reason, since an empty memtable is already refused and a flushed generation therefore always holds 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 pair so the two flush paths share one measurement and neither the manifest write nor the result can transpose them.SsTable::unmeasuredconstructs an entry for the paths that build one without a measurement.Note for downstream:
SsTablegains two fields, so code constructing it with a struct literal needs updating.Testing. A flush test asserts both numbers reach the manifest entry through the protobuf round-trip and that the recorded size exceeds the raw payload rather than matching an encoded file length; two conversion tests cover the round trip and the pre-upgrade entry decoding as unmeasured rather than as zero. 662
mem_waltests and 360lance-tabletests pass; fmt clean, and clippy clean on both touched crates.