Skip to content

fix(mem_wal): reconcile a schema change through one plan - #9143

Open
xuanyu-z wants to merge 48 commits into
lance-format:mainfrom
xuanyu-z:memwal-replay-schema-drift
Open

xuanyu-z wants to merge 48 commits into
lance-format:mainfrom
xuanyu-z:memwal-replay-schema-drift

Conversation

@xuanyu-z

@xuanyu-z xuanyu-z commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What this changes

A table with a MemWAL holds rows in three places at once: the base table, sealed
generations, and the active memtable. A schema change lands on the base table
only, so every read has to reunite rows written under an older schema with a
table that has moved on.

A sealed generation is a Lance dataset of its own, written under whatever the
table's schema was at the time, so the names it stores are its own. Only field
ids relate them to the table's: a rename keeps the id and moves the name.

This adds one resolution, GenerationRead, and routes every read path through
it — scan, point lookup, vector search, full-text search. It answers three
questions in the order a read needs them:

  1. what to project — the wanted columns under the names the file has, with
    a column it never stored left out;
  2. whether a predicate can be pushed into it, and under which name — one it
    cannot answer at all (a column sealed before it existed, or a nested one)
    runs above the reconciliation instead;
  3. how to bring the result back — renames followed, absent columns filled
    with nulls, nested columns rebuilt to the shape the table declares.

Replay uses the same Plan the reads do, so a WAL entry written under an older
schema conforms the same way a scan of it reads.

What is refused on a table with a MemWAL

Each is undecidable from schemas alone, so it is rejected rather than guessed.
Tables without a MemWAL are untouched, and each has a test.

refused why
change a column's type same name, new id is indistinguishable from "dropped and added back under the same name"
make a column non-nullable validated against committed fragments, and a row still in the MemWAL is not among them; flushing first does not close the window, since writes keep arriving into the next generation

Each error says what to do instead — drop the MemWAL, or add a column of the new
type and backfill it.

A rename is not among them, at any depth. Lance does not compact a MemWAL's
generations; a caller does, and reconcile_batches below is how it follows a
rename through the merge.

A dropped index no longer bricks a shard

The maintained index set is fixed at initialize_mem_wal and cannot be edited.
Opening a shard used to fail outright when the set named an index the dataset no
longer had — which a drop, a replace, or dropping the covered column all
produce. That left the table unopenable for good.

Opening now skips the missing index and serves without it: the base index is
gone for everyone, so the fresh tier has nothing to keep in step with. Naming an
index that does not exist is still rejected at initialization, which is the last
moment it can be corrected.

Contracts this establishes

Three of these fire with no schema change at all, which is why they had gone
unnoticed: they are properties of ordinary reads that the reconciliation has to
respect.

Three schemas, not one

schema what it describes
stored the source's actual arrays and persisted field identities
intermediate candidates in flight, through reconciliation and version resolution
public the final user-visible result

Nullability comes from the source, not the table. A generation stores every
non-key column as nullable however the table declares it — that is what lets a
strict table hold a tombstone, whose payload is null in everything but the key.
A point lookup carries tombstones through on purpose, so those rows have to
survive reconciliation; the table's own nullability is restored at the public
boundary, after the tombstones are dropped.

Recursion is complete or it is wrong

A field id lives inside a nested column's Arrow type at every level, so a
relabel that replaces only the outer type leaves Struct<Struct<…>> rejected —
Arrow validates a container against the child types its own type declares. Every
nested container takes part: struct, list, large list, fixed-size list, and map.

A test pins this to Lance itself: every type Field::try_from gives children to
must be one the reconciliation recurses into. If Lance starts giving children to
a type this does not know, those ids would go unrestored and the column would
fall back to matching by name — so the test fails instead.

Predicate placement is a separate question from translation

Whether a predicate can be translated to the source schema and whether it is
safe to evaluate there are different questions. A predicate is pushed into a
generation's own scan when the stored and declared shapes agree — which is the
common case, including ordinary predicates on ordinary structs. Deferring those
was a correctness bug, not just a slow path: a search selects its top-k first,
so a predicate applied afterwards loses a lower-ranked row that should have won.

A nested predicate needs the parent's whole type compared, not its name: a
filter on info.child reports info as its column reference, and renaming
the child does not move the parent's name. Field ids are compared too, so a
child dropped and added back under the same name and type is not mistaken for
the original.

Rules this establishes

  • A generation's own columns (_tombstone, anything the table has dropped) are
    numbered in its own schema, so their ids collide with whatever the table gave
    those numbers. They are stripped before resolution rather than resolved to a
    table column that happens to share an id.
  • Lance writes -1 for a field it has not assigned an id to. Treating that as
    an id would pair every unassigned column with every other, so a negative id
    counts as none at all.
  • A reader is handed the table's plain schema. Field ids live inside a nested
    column's own Arrow type, so only replay — which writes back into the
    memtable's id-carrying storage schema — keeps them.
  • SchemaRelabelExec relabels the arrays as well as the schema, for the same
    reason.
  • A search whose column the generation predates has nothing to offer and
    contributes an empty arm rather than failing the query.

API

with_identity_schema takes the id-carrying schema on all four planners —
LsmScanner, LsmPointLookupPlanner, LsmVectorSearchPlanner and
LsmFtsSearchPlanner. LsmScanner::new fills it in from the dataset. A caller
that supplies none matches by name, as before ids existed.

arrow_schema_with_field_ids is now public, so a caller holding an Arrow schema
can build the id-carrying one.

reconcile_batches brings a sealed generation's batches to the table's schema —
the same resolution the read paths apply, for a caller that reads a generation
for itself. Merging one by matching names cannot follow a rename: a struct's
children carry ids of their own, and a pair that exchange names leaves the two
shapes identical and each child's values under the other's.

Testing

3843 tests pass, 3 skipped. GenerationRead's rules have direct unit tests:
which name a column is asked for, when a predicate can be pushed down and under
which name, that a generation's own columns never answer for one of the table's,
and that a table without ids falls back to names. A renamed field inside a map
resolves by id, and a quoted path containing dots resolves to its real parent.

End-to-end coverage lives in the sophon PR, which drives a real WAL server:
6 schema changes × 2 places rows can live × 11 read routes, plus the same grid
over vector search (5 × 2 × 3) and full-text search (5 × 2 × 2), a four-stage
lifecycle equivalence, and generated histories checked against an independent
model — 198 generated checks in all.

@github-actions github-actions Bot added the bug Something isn't working label Sep 11, 2026
@xuanyu-z
xuanyu-z force-pushed the memwal-replay-schema-drift branch from 19fc83e to 2dd4c47 Compare September 14, 2026 22:40
@xuanyu-z xuanyu-z changed the title fix(mem_wal): replay a WAL entry whose schema has since moved fix(mem_wal): read and replay a table whose schema has moved Sep 15, 2026
@xuanyu-z
xuanyu-z force-pushed the memwal-replay-schema-drift branch from e7801cc to bbbbb20 Compare September 15, 2026 19:25
@xuanyu-z xuanyu-z changed the title fix(mem_wal): read and replay a table whose schema has moved fix(mem_wal): reconcile a schema change through one plan Sep 15, 2026
@xuanyu-z
xuanyu-z force-pushed the memwal-replay-schema-drift branch from 21cf4fd to 9d07b04 Compare September 16, 2026 00:48
A replayed entry was re-labelled to the current storage schema by position:
the entry's columns were taken in order and rebound to whatever the schema now
declares. That only holds while the schema is the one the entry was written
under.

It is not, after a column is added or dropped. An entry that predates an added
column is one column short, and the rebind fails on the width — the shard
cannot be opened at all. An entry that predates a dropped column is one column
long, and the rebind silently stores each remaining column under its
neighbour's name, which is worse: the open succeeds and the rows are wrong.

Match by name instead. A column the schema declares and the entry does not
carry is filled with nulls, which is what a column added later means for rows
written before it; non-primary-key columns are nullable in the storage schema
whatever the base table declares, so the null is always representable. A column
the entry carries and the schema no longer declares is dropped.

Two things stay errors, because no fill is right for them: a missing primary
key, which cannot be invented, and a column whose type changed.

Live writes are unaffected. They are checked against the logical schema before
they reach here, so every column is present and this still only appends
`_tombstone`.
…eeps its rows

Replay matches a WAL entry to the storage schema by name, which holds for a
column added or dropped but not for one renamed. A rename is the one change
that moves a column's name while keeping its identity: the name is mutated in
place and the field id is untouched, which is why data files need no rewrite --
they list field ids, not names.

Matching on name alone puts the fresh tier at odds with that. The entry carries
the old name, the schema declares the new one, and the column is nulled under
its new name while the old one is dropped: every row still in the memtable
loses the value.

Carry the id. `From<&Field> for ArrowField` drops it, so the memtable's storage
schema stamps it into field metadata itself, and Arrow IPC preserves field
metadata, so every WAL entry written under that schema carries it too. Conform
then matches on id where both sides have one and falls back to name where they
do not -- entries written before this have no ids, so both tiers are needed.

Scoped to the memtable path on purpose. Emitting the id from the global Arrow
conversion would change every schema Lance hands out, including for callers
that compare schemas for equality; this changes only the schema the memtable
and its WAL entries are written under.
`alter_columns` casts the rows already in the base table rather than rewriting
them, so a column's type can move under a shard that still holds entries
written at the old type. Conform rejected those outright, which left the shard
unopenable: the entry is not wrong, it is just older than the schema.

Cast it, under the same `safe: false` option `alter_columns` uses, so a
replayed row lands in the state it would have had if written after the change
and a lossy cast stays an error rather than a column of nulls. Both halves of
the table then agree on what the change meant.
Ids are carried for top-level fields, which is the granularity conform works
at. A struct column is taken or it is not, and a change inside it is a change
to the column's type.
`ShardWriter::open` takes whatever schema it is given. One built without field
ids still works -- a replayed entry falls back to matching by name -- but a
column renamed while that shard holds rows then reads as null for every one of
them, and nothing about the call says so.
A maintained set is fixed when the write spec is installed and cannot be edited
afterwards, so an index that disappears -- dropped outright, or carried away
with the column it covered -- is named by a set that can never stop naming it.
Refusing to build the configs then refuses the claim, and a table whose claim
cannot be built serves no reads at all, over a condition that costs the fresh
tier one index.

Skip it and say so. The base index is gone for every reader; the fresh tier
simply has nothing left to keep in step with.

Validating a set before it is installed still rejects an unresolvable name.
That is the operator's mistake and the one moment it can still be corrected.
UnionExec requires its arms to agree on schema and does not reconcile.
A sealed generation holds the shape it was sealed under, so once the base
table's schema moves the LSM scan fails: an added or renamed column errors
and a retyped one aborts the process. Bring every arm to the base table's
shape first - cast where the type moved, fill typed nulls where the column
is absent - and project each SSTable with its own generation's schema.
A computed transform derives the new column from the rows it can read, which
is the committed fragments and nothing else. Rows still held in the WAL are
invisible to it, so they take a null and keep it when they merge down - a
wrong value written silently, that no later pass corrects.

Emptiness is not a safe exemption: a write can land between the check and
the commit, so the only race-free rule is to refuse whenever a MemWAL is
present. AllNulls stays allowed, since a null is what it means everywhere.
… attached"

Refusing removes the feature rather than fixing it: a WAL-backed table could
no longer take a computed or UDF column at all. The fix belongs where the WAL
can actually be drained - sophon seals and compacts the fresh tier into base
before materialising, so the transform sees every committed row.

This reverts commit aeb8496ad4d2ee4b38c85e9e8df1a1c8a1e29df1.
Name the moment the maintained set is fixed as `initialize_mem_wal`, drop an
inline comment that restated the doc above it, and state the id-before-name
rule without arguing the alternatives.
The union matches arms by name. A generation sealed under the old name
stores the column under that name, so the match found nothing and the column
was filled with typed nulls -- every row still present, every value gone.

Relabel each generation's columns to the names the base table uses for the
same field ids before the union sees them. A rename keeps the id, so the
values arrive under the name the other arms use. Columns the base table does
not declare keep their own names, as do all of them when no base arm is
there to agree with.
A struct's child can be renamed on its own: the parent column keeps its
name, so the arms agree there, and only the struct's inner name differs.
Casting one struct to the other is refused for having no field-name overlap,
which failed the whole scan.

Match nested fields by id too, and rebuild the struct array when a child is
renamed -- the child names live in the array's own type, not in the schema
above it, so relabelling the schema alone leaves the batch disagreeing with
it. The children are reused, so the rebuild costs a pointer copy.
A column the base table does not declare exists only in a generation, so its
id is drawn from the generation's own numbering and can collide with the id
base gave an unrelated column -- relabelling by that id moves `_tombstone`
onto a user column's name. Skip a rename onto a name the arm already carries.
A generation's `_tombstone` is numbered in the generation's own schema, so
its id collides with whatever base gave that id -- relabelling by id carried
the tombstone in under a user column's name, where a column added after the
seal read as that tombstone cast to its type instead of as null.

Skipping a rename onto a name the arm already carries does not cover it: the
name the tombstone was taking is one the arm does not have.
Each rule earned its place from a defect: a column takes base's name for its
id, a column whose id base no longer declares keeps its own, a generation-only
column is never renamed onto a base name, a rename onto a name the arm carries
is skipped, and a struct child is matched by its own id.
…ng them

A generation stores each column under the name the table had when it was
sealed, and every name the caller supplies is base's. Matching one to the
other after the fact left four ways to answer wrongly: a named projection
asked the file for a name it does not have and read nulls; a predicate did
the same and failed the scan; a column renamed into a name a dropped column
had returned the dropped column's data; and replay copied a renamed column
into a new one that reused its name.

Resolve the mapping first. By field id, which a rename keeps and a drop
retires; then by name for a retype, which keeps the name and takes a new id,
and only where no id match has claimed that name. A column base has dropped
is not read at all.

The caller's names are then translated into the generation's before
projecting, and a predicate runs above the relabel -- with the columns it
names filled in -- whenever this generation cannot evaluate it as written.
The limit follows the filter rather than preceding it.
A rename frees a name, so a stored column can answer to a schema column's
name without being that column, and a cast takes a new field id, so the same
column can answer to no id at all. Matching by id and then by name resolved
both, but nothing stopped a single stored column from being taken twice --
once by an id match and again by a name match it was no longer entitled to.

A name match now applies only to a column no id match has claimed. Replay and
the scan resolve it the same way, so a retype keeps its values and a column
that merely reuses a freed name reads as null.

Three more, all in the scan: a predicate this generation cannot evaluate runs
above the relabel, so the columns it names have to be read even when the
caller did not ask for them; a column base has dropped is no longer a column
a predicate may be answered from; and the fallback target is the newest
generation, which is the first of them, since sources arrive generation-DESC.
…retype

A cast takes a new field id and keeps the column's name, which is exactly what
dropping a column and adding another under that name looks like. Nothing in the
schemas tells the two apart, so every attempt to reconcile both ended up
guessing: matching by name resurrected dropped values, matching by id alone lost
retyped ones.

A table with a MemWAL now refuses to change a column's type, before any of the
request commits and including a request that also renames. A table without one
is unaffected. With that gone, identity is the field id and nothing else, and
the guessing goes with it.

Replay and the scan resolve a schema change the same way, through one plan that
says for each target column which stored column supplies it, which children need
rebuilding, and what a column the source never had produces. Field ids are
carried recursively, so a struct's children are matched by their own identity
rather than by an Arrow cast that follows names. The target is the table's
schema, supplied by the caller, rather than whichever source the collector
ordered first.

Projection and predicate are planned together: the columns a deferred predicate
names are read even when the caller did not ask for them, and a limit follows a
filter that has not run yet.
… paths

Vector and full-text search open their own SSTable arms and projected the
table's column names onto them. A rename moves the table's name while the file
still holds the old one, so the projection asks for a column that is not there
and the search fails.

They translate the projection the same way the scan does, by field id.
The relabel exec no longer rebuilds struct arrays: arms are reconciled to the
table's names before the union sees them, so the only relabel left is the
logical/storage nullability boundary it was written for.

Also corrects the comments that still described reconciling a retype.
…atch it

A field id resolves which column is which, and a nested column carries its
children inside its own type -- so the ids have to reach them and the arrays
have to be built under them.

Ids are stamped and stripped through a struct's children, a list's element and
that element's children in turn, and restored the same way onto a scan's output,
which is built from the dataset and carries none. A nested column's array is
rebuilt under the target's children even when nothing about them moved:
otherwise an unchanged struct produces a batch that disagrees with the schema it
is built under, which Arrow rejects. Lists and fixed-size lists keep their
offsets, width and validity; only the element's own type moves.

A predicate over a rebuilt column is answered after reconciliation. Its
reference names the parent -- a predicate on a struct's child refers to the
struct -- and the parent's name does not move when a child is renamed, so a name
check alone would push it down to be evaluated against the names this generation
happens to hold.

Live input is bound to its own schema before reconciliation. It is trusted for
its values and not for its identity: a validated batch's columns are already the
right ones in the right order, and any ids it carries are the caller's to claim
rather than the table's to honour.
A sealed generation is written under whatever the table's schema was at
the time, so the names it stores are its own. Only the scan resolved them
to the table's; the point lookup projected the table's names straight at
the file, and the vector and full-text searches translated the projection
but not the predicate, the searched column, or the output.

GenerationRead is that resolution, and all four read paths now go through
it: what to project, whether a predicate can be pushed down and under
which names, and how to bring the result back. Three consequences the
paths had each got differently:

- a generation's own columns (_tombstone, anything the table has since
  dropped) are numbered in its own schema, so their ids collide with
  whatever the table gave those numbers. They are stripped before
  resolution rather than resolved to a table column that shares an id.
- a reader is handed the table's plain schema, so reconciliation emits
  it. Field ids live inside a nested column's own type, and only replay,
  which writes back into the memtable's storage schema, keeps them.
- a search whose column the generation predates has nothing to offer, and
  contributes an empty arm rather than failing the whole query.

SchemaRelabelExec relabels the arrays as well as the schema, for the same
reason: a nested column carries its ids in its type, so relabelling the
schema alone left the two disagreeing.

LsmScanner and LsmPointLookupPlanner take the id-carrying schema through
with_identity_schema, which LsmScanner::new fills in from the dataset.
A caller that supplies none matches by name, as before ids existed.
@lance-gatekeeper lance-gatekeeper Bot added the K-risk Latest Gatekeeper recommendation includes a non-blocking risk. label Sep 16, 2026
@xuanyu-z
xuanyu-z force-pushed the memwal-replay-schema-drift branch from 59b1d0d to 8fd8d11 Compare September 16, 2026 16:00
@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 16, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 16, 2026
@xuanyu-z
xuanyu-z force-pushed the memwal-replay-schema-drift branch from 8fd8d11 to bcb71a1 Compare September 16, 2026 16:11
@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 16, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 16, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

The nested-rename finding is fixed in c0b1498: MemWAL-backed tables now reject nested renames before any schema change, while top-level renames remain supported.

The remaining accepted limitation is that direct callers pairing a memtable with a schema created elsewhere can still fail reads after a rename. The supported writer/reader protocol replaces them together, avoiding an extra reconciliation node on every memtable batch.

Please mark this PR with the breaking-change label.

A sealed generation's columns relate to the table's by field id, and a
struct's children carry ids of their own. A caller merging a generation
into the base table by matching names cannot follow a rename: one leaves
the two shapes disagreeing, and a pair that exchange names leaves the
shapes identical and the values crossed.

`reconcile_batches` hands that caller the resolution the read paths
already apply, so the merge follows ids at every level.

With the resolution available, refusing a nested rename is no longer the
only way to keep one safe, so the refusal is withdrawn. A retype and a
nullability tightening stay refused: those are undecidable from schemas
alone whatever the caller does.
A generation numbers `_tombstone` in its own schema, so its id is
whatever that generation reached -- and the table may have given that
same number to a column of its own. Honouring it resolves the two to
each other and refuses the merge on their types.

The scan path already strips those before resolving; `reconcile_batches`
now does the same, so a caller reading a generation for itself gets the
same answer a read of it would give.
@LuQQiu

LuQQiu commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Please address 1, others are non-blocking, good to have

Review Comments

  1. Upgrade hazard: generations flushed before this PR can read back wrong (main concern).
  Pre-PR, the shard schema was self.schema().into(), which drops ids — so existing sealed generations were written with
  positionally auto-assigned ids (0..n via set_field_id), not the table's. After upgrade, stored_names matches those ids
  against the table's real ids with no fallback (fallback fires only when the table side has no ids, and LsmScanner::new
  always supplies them). On any table that had evolved before initialize_mem_wal (id gaps), this mispairs with no schema
  change at all:

  - Table a(0), c(2) (b dropped pre-init); an old generation stores a, c as ids 0, 1 → c silently reads as null.
  - Worse, with a(0), c(2), d(3), the old generation numbers d as id 2 → d's values are served as column c if the types
    agree.

  Old WAL entries are safe (they carry no ids → name fallback), but old flushed generations are not, and nothing
  distinguishes "ids are the table's" from legacy positional ids. If pre-PR MemWAL deployments with un-compacted
  generations are in scope, this needs a marker (e.g., a schema-metadata flag stamped on generations written under the new
  scheme, with name-matching for generations lacking it). If MemWAL carries no upgrade guarantee yet, that decision
  should at least be stated in the PR. This is the one thing I'd ask the author before merge.

  2. project_to_canonical no longer errors on unknown columns — for anyone (projection.rs). The None => internal error arm
  became typed-null fill, and a type mismatch became a silent CastExpr, for all callers of the canonical projection, not
  just reconciled arms. A future planner bug that previously surfaced as "Column missing from canonical projection source
  schema" now silently produces null columns. Consider keeping a debug assertion or restricting the null-fill to the paths
  that legitimately need it.

  3. Nested _tombstone special case fires at any depth (reconcile.rs::resolve_field). A user struct child literally named
  _tombstone that's absent from the source gets Source::Live (a false BooleanArray) instead of typed nulls — and a
  construction failure if it isn't Boolean. resolve_children already passes &[] for pk columns at depth; the tombstone
  rule should similarly apply only at the top level.

  4. Deferred-predicate vector search ranks the whole generation. k = dataset.count_rows(None).max(1) is a real perf
  cliff: one renamed struct child turns a filtered ANN search into ranking every row of that generation until compaction
  catches up. It's the correct choice (documented well — cutting top-k before the filter loses rows) but worth a
  log::warn! or metric so operators can see why a query got slow.

  5. Active memtable is out of contract for renames. The comment in planner.rs states that a memtable paired with a schema
  it wasn't created from requires a writer reopen. So after a rename, sealed generations and replay resolve correctly but
  the active memtable serves under old names until the writer reopens. That operational requirement lives only in a code
  comment — it belongs in the api.rs module-level Limitations section alongside the maintained-index caveats.

  6. Minor nits.
  - api.rs:384: comment says See MissingIndex::Skip — enum is OnMissingIndex.
  - reconcile_batches(pk_columns: &[String]) — repo guidelines prefer flexible inputs (&[impl AsRef<str>]) for public
    APIs; same for the Vec<String> on the planners, though those match existing local style.
  - GenerationRead::stored_name is an O(n) reverse scan over the map per call, called per projected column — fine at these
    widths, just noting it's quadratic-ish by design.

// the table's columns and are never resolved to one.
.filter(|f| f.name() != TOMBSTONE && !is_system_column(f.name()))
.filter_map(|f| {
let id = field_id_of(f)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Prevent reuse of field IDs that are still retained by MemWAL generations

This can also mispair generations written after this PR. Lance field IDs are not currently a permanent high-water mark: Manifest::max_field_id() only considers the current base schema and referenced base files, and the existing test_drop_add_columns demonstrates that dropping a field can lower the maximum ID and allow the next added field to reuse it.

A retained MemWAL generation is not referenced by the base manifest. For example, a generation may still contain old_value(id=1) after that field is dropped, while a subsequently added new_value is also assigned id=1. stored_names then treats old_value as a rename of new_value; when their types agree, old values are silently returned under the new field instead of null. reconcile_batches makes the same ID match during compaction.

A legacy-generation marker does not address this case because the generation was correctly stamped with the table ID that was valid when it was written. We need to prevent reuse of IDs still reachable from MemWAL generations—for example with a persistent high-water mark, by draining retained generations before reuse, or by rejecting the relevant drop/add sequence while a MemWAL is attached. Please add a regression covering drop, ID reuse, and a retained generation.

Three tests built the same generation two ways and asserted whether a
predicate on the parent reached it. They are one rule with three inputs,
so they read better as a table: unmoved pushes down, a renamed or a
replaced child does not.
lance-format#9292 gave the fresh tier cross-column predicates, generalizing the FTS
planner from one column to a slice; this branch routes every sealed
generation through one resolution so a rename is followed. They meet in
`build_source_plan`'s SSTable arm and in the per-column index contract.

The arm now resolves every queried column to the name the generation
stores it under, rather than just the one. Three cases follow: nothing
moved, so the tree reaches the scanner untouched and a cross-column
predicate keeps its own leaf bindings; one column moved, so the whole
tree binds to the stored name as before; several columns with one moved,
which `with_column` cannot express -- it rebinds the whole tree and would
collapse the predicate onto a single field -- so that is refused rather
than ranked on the wrong column.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants