feat(table): add Transaction.ReplaceSortOrder (Java Table.replaceSortOrder parity) - #1833
Merged
Merged
Conversation
…Order parity) Java's Table.replaceSortOrder / BaseReplaceSortOrder replaces the default write order and UpdateRequirements.forUpdateTable fences the commit with AssertDefaultSortOrderID. This adds the same operation: AddSortOrder + SetDefaultSortOrder(-1), no-oping when the staged default already has identical fields. FileToDataFile exports the parquet-footer-to-DataFile conversion used by AddFiles. Upstream's unexported fileToDataFile dropped the sortOrderID parameter and claimed no sort_order_id; the export restores it so callers converting foreign parquet footers can convey the file's sort layout (spec data-file field sort_order_id). AddFiles still passes 0 so it does not claim sort on files it did not write. Co-authored-by: Cursor <cursoragent@cursor.com>
zeroshade
approved these changes
Aug 14, 2026
zeroshade
left a comment
Member
There was a problem hiding this comment.
Reviewed the diff, cross-checked against the metadata builder and requirement-dedup logic, and ran the full ./table suite on the PR head — everything passes and the description's claims match the code.
A few things I verified because they're subtle:
- The sequential-replace flow (table/transaction.go:526-531) only works because
applydedups the secondAssertDefaultSortOrderID(base)by semantic key before validation — a duplicated requirement is conflict-checked against its kept twin rather than re-validated against staged metadata (which has already moved past the base default). That's correct and matches Java's UpdateRequirements semantics (requirements assert base state), but it's load-bearing; if the dedup-before-validate ordering inapplyever changes, thesequential replaces assign fresh idstest will catch it, which is good. - The no-op check compares against the staged default via
meta.defaultSortOrderID, not the base table, so replace-then-replace-back within one transaction behaves correctly, and the incoming order's ID is ignored as advertised (sameSortFieldsis field-only, mirroringreuseOrCreateNewSortOrderID's comparison). FileToDataFile's panic-recover wrapper matches the existing pattern infilesToDataFiles, andAddFilesstill passes 0, preserving #1184's behavior.
Two minor notes, neither blocking:
- table/transaction.go:518-524 — the no-op check reads
t.metawithout holdingt.mx(onlyapplylocks). This is consistent with the other transaction entry points, and the worst case is a redundant-but-idempotent apply, so I'm fine with it; just noting the transaction type isn't actually safe for concurrent mutators despite the mutex. FileToDataFiletreatssortOrderID == 0as "no claim", so a caller can't explicitly stampsort_order_id = 0(the spec's reserved unsorted order). Since unset ≈ unknown and a 0 claim carries no real information, I think this is the right trade-off, and the doc comment states it clearly.
Small test-coverage nit: there's no case replacing a sorted default back to UnsortedSortOrder (the no-op test starts from unsorted). Not required for merge.
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
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.
What
Adds
Transaction.ReplaceSortOrder(order): the Go analogue of Java'sTable.replaceSortOrder()/BaseReplaceSortOrder, which replaces the table's default write order. It appliesAddSortOrder+SetDefaultSortOrder(-1), and no-ops when the staged default already has identical fields (field-only compare — the incoming order's id must not affect the result). Java's version is a fluentasc()/desc()builder; taking a completeSortOrderis the idiomatic Go adaptation of the same operation.It also exports
FileToDataFile, the parquet-footer-to-DataFileconversion backingAddFiles, with asortOrderIDparameter. #1184 removed that parameter from the internal function soAddFileswould not claimsort_order_idon files it did not write — correct for that caller, but it left no way for a caller who does know a foreign file's sort layout to convey it (data-file fieldsort_order_idin the spec).AddFiles/filesToDataFilesstill pass 0, preserving #1184's behavior.The two ship together because a rewrite/compaction flow uses both:
ReplaceSortOrderinstalls the table's write order, andFileToDataFilestamps that order id on the rewritten files it registers.Behavioral note: re-selecting an order that already exists in base metadata should emit only
set-default-sort-orderwith the concrete id — that is #1831's fix; without it, strict REST catalogs reject the reuse commit'sadd-sort-order. The diffs are independent; this PR does not depend on it textually.Tests
FileToDataFile: stats round-trip;sortOrderID0 leaves the field unset, non-zero is stamped; missing file returns an error.go test ./table/...andgolangci-lint runare clean.Made with Cursor