fix(table): guard nil base in snapshot timestamp check - #1799
Conversation
Signed-off-by: badalprasadsingh <badal@datazip.io>
Signed-off-by: badalprasadsingh <badal@datazip.io>
zeroshade
left a comment
There was a problem hiding this comment.
LGTM. The guard on b.base in addSnapshotInternal (table/metadata.go:521 on main) is correct — NewMetadataBuilder never sets base, so any AddSnapshot on a create-path builder panicked, and treating a missing base as "no previous update" (0) is the right floor since maxTS still picks up b.lastUpdatedMS.
One thing worth noting for the record: this PR actually fixes two nil derefs, not one. On a fresh v2+ builder, snapshot.SequenceNumber <= *b.lastSequenceNumber would panic first for any snapshot with a parent, before the code ever reached b.base.LastUpdatedMillis(). The new currentLastSequenceNumber() handles that, and its 0 fallback is consistent with nextSequenceNumber() and with what Build() writes for a nil lastSequenceNumber, so the parented-snapshot-seq-0 rejection behaves identically to a builder over freshly created v2 base metadata. Might be worth mentioning the second panic in the commit message, but not blocking.
Test coverage is solid — parentless/parented, seq-number rejection at the initial value, the last-updated timestamp bound, the v3 row-lineage path that was previously unreachable, and the AddSnapshotUpdate preserve path. I ran go build ./... and the full ./table suite on the PR head; everything passes.
|
Thanks @zeroshade! Updated the PR description to call out both nil |
Description
Fixes #1798
Two nil derefs in
addSnapshotInternalon a create-pathMetadataBuilder(baseis nil untilMetadataBuilderFromBaseis used):*b.lastSequenceNumberpanicked for parented snapshots. AddedcurrentLastSequenceNumber(), falling back to 0, matchingnextSequenceNumber()/Build().b.base.LastUpdatedMillis()panicked on nilbase. Guarded it likecurrentNextRowID(), treating a missing base as 0.Testing
Added the necessary tests required for it.