Repository navigation
fix(table): look up removed-file partition specs by ID - #1816
Conversation
accumulateSummaryDelta resolved partition specs two different ways. Added files went through sp.spec(int(df.SpecID())), a by-ID lookup over GetSpecByID, while removed data files, delete files and deletion vectors indexed the spec slice directly with specs[df.SpecID()]. Spec ID is an identifier, not a slice position. RemovePartitionSpecs filters the slice without renumbering the survivors, and specs loaded from metadata keep the partition-specs array order, so the two come apart on ordinary tables. The index expression is evaluated as a call argument, so it fails before removeFile ever runs: a Delete, Overwrite or RewriteFiles commit that removes files panics with index out of range, or silently attributes the change to the wrong partition when the ID stays in bounds but points at the wrong entry. Use sp.spec() in the three removal loops so both halves of the function agree, and drop the now-unused local. Unknown IDs then yield an empty unpartitioned spec instead of panicking, matching the add path. The regression test evolves specs 0/1/2, removes spec 1 so the slice becomes [id0, id2], then sends the same spec-2 data file down both branches and asserts they report the same changed partition. Fixes apache#1815 Signed-off-by: codeAnqiang-ma <[email protected]> Co-authored-by: Cursor <[email protected]>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Verified locally: the three removal loops in accumulateSummaryDelta (table/snapshot_producers.go:1021-1041 on main) index sp.txn.meta.specs positionally while the add loops resolve by ID via sp.spec(), so any commit removing files after RemovePartitionSpecs (or on metadata whose partition-specs order doesn't match IDs) panics or misattributes the partition. Switching the removal paths to sp.spec(int(df.SpecID())) makes both directions symmetric, including the fall-back to an empty unpartitioned spec for unknown IDs, which is the established behavior of the add path. The regression test is well constructed — asserting the add path first so the add/remove asymmetry is visible, and covering the exact spec-removal scenario that decouples positions from IDs. I ran the test on the PR head and it passes. LGTM.
Fixes #1815
accumulateSummaryDeltaresolves partition specs two different ways. Added files usesp.spec(int(df.SpecID())), a by-ID lookup overGetSpecByID; removed data files, delete files and deletion vectors index the slice directly,specs[df.SpecID()].Spec ID is an identifier, not a slice position.
RemovePartitionSpecsfilters the slice without renumbering the survivors, and specs loaded from metadata keep thepartition-specsarray order (no sort), so positions and IDs come apart on ordinary tables. Becausespecs[df.SpecID()]is evaluated as a call argument, it fails beforeremoveFileever runs: any Delete/Overwrite/RewriteFiles commit that removes files panics withindex out of range, or — when the ID stays in bounds but points at the wrong entry — silently attributes the change to the wrong partition.RemovePartitionSpecs([]int{1})over specs 0/1/2 leavesspecs == [id0, id2]. Sending the same spec-2 data file down both branches:partitions.id_trunc=0panic: runtime error: index out of range [2] with length 2Same file, same spec, same function; only the branch differs.
The fix calls
sp.spec(...)in the three removal loops and drops the now-unused local. Unknown IDs then yield an empty unpartitioned spec instead of panicking, matching the add path.Test evidence
Regression test
TestAccumulateSummaryDeltaResolvesRemovedFileSpecByIDintable/snapshot_producers_test.go. It asserts the add path first, so the failure output itself shows the asymmetry rather than just "remove panics".Before the fix:
After the fix:
go test ./...go test -race ./codec/... ./table/...golangci-lint run(v2.11.4, whole repo)gofmt -lNot run: the Docker-gated integration suite (
go test -tags integration ./...) and the s390x cross-compile job. The change is confined to in-memory summary accounting, with no catalog, IO or platform-specific code involved.Assisted by Cursor; I reproduced the panic locally and reviewed every line of the change.