Skip to content

fix: allow historical sort orders with dropped fields#762

Merged
wgtmac merged 2 commits into
apache:mainfrom
zhjwpku:fix/historical-sort-order-dropped-fields
Jun 23, 2026
Merged

fix: allow historical sort orders with dropped fields#762
wgtmac merged 2 commits into
apache:mainfrom
zhjwpku:fix/historical-sort-order-dropped-fields

Conversation

@zhjwpku

@zhjwpku zhjwpku commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

This fix is aligned with:

apache/iceberg#16521


auto metadata = TableMetadataFromJson(metadata_json);
ASSERT_THAT(metadata, IsOk());
ASSERT_EQ(metadata.value()->sort_orders.size(), 2);

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.

Can we assert the historical sort order still has two fields, including source_id() == 2? Otherwise this would still pass if the dropped-field sort term were skipped. A negative case where that order is the default sort order would also help verify default sort order validation is still strict.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated the tests to cover both preserving the dropped-field historical sort order and rejecting it as the default sort order.

@zhjwpku zhjwpku added the ready to review This PR is ready to review with all previous comments addressed. label Jun 22, 2026
@wgtmac wgtmac removed the ready to review This PR is ready to review with all previous comments addressed. label Jun 23, 2026
@wgtmac

wgtmac commented Jun 23, 2026

Copy link
Copy Markdown
Member

Thanks @zhjwpku for the fix and @huan233usc for the review!

@wgtmac wgtmac merged commit 5ac5246 into apache:main Jun 23, 2026
20 checks passed
@zhjwpku zhjwpku deleted the fix/historical-sort-order-dropped-fields branch June 23, 2026 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants