Feat: replace sort order #1500

JasperHG90 · 2025-01-08T21:24:41Z

This PR adds functionality to replace a table's sort order. Closes #1245

Some basic tests are implemented but need to be expanded.

Currently, a new sort order ID is assigned. This adds a sort order to the list of available sort orders rather than replacing it.

@Fokko do you mind taking a look at this and telling me what you think? Thanks in advance!

kevinjqliu

Thanks for the PR!
Generally lgtm, added a few nit comments

kevinjqliu · 2025-01-11T19:19:57Z

pyiceberg/table/__init__.py

@@ -404,6 +405,20 @@ def update_schema(self, allow_incompatible_changes: bool = False, case_sensitive
            name_mapping=self.table_metadata.name_mapping(),
        )

+    def replace_sort_order(self, case_sensitive: bool = True) -> ReplaceSortOrder:


nit: wdyt about update_sort_order instead?

from the spec,

A data or delete file is associated with a sort order by the sort order's id within [a manifest](https://iceberg.apache.org/spec/#manifests). Therefore, the table must declare all the sort orders for lookup. A table could also be configured with a default sort order id, indicating how the new data should be sorted by default. Writers should use this default sort order to sort the data on write, but are not required to if the default order is prohibitively expensive, as it would be for streaming writes.

All sort orders are kept in the manifest in the sort-orders field.

Thanks. Updated class & method names

kevinjqliu · 2025-01-11T19:23:02Z

tests/integration/test_sort_order_update.py

+        SortField(source_id=1, transform=IdentityTransform(), direction=SortDirection.ASC, null_order=NullOrder.NULLS_FIRST),
+        SortField(source_id=2, transform=IdentityTransform(), direction=SortDirection.DESC, null_order=NullOrder.NULLS_LAST),
+        order_id=1,
+    )


can you add a test for modifying sort order of a table with existing sort order?

With modifying do you mean updating it so that there's a new sort order or do you mean to modify an existing one without creating a new sort order. I've added a test doing the former.

modify an existing one. but i see you added both. thanks!

kevinjqliu · 2025-01-11T19:24:25Z

pyiceberg/table/update/sorting.py

+    from pyiceberg.table import Transaction
+
+
+class SortOrderBuilder:


nit: do we need the builder here? Or just follow UpdateSchema's pattern

We don't need it but personally I like it because it's easier to read. I'll leave it up to you to decide if you want this.

Refactored this.

JasperHG90 · 2025-01-16T20:54:09Z

Thanks for the comments 🙌 @kevinjqliu . Will look at your comments this weekend.

Fokko

This looks great @JasperHG90 Sorry for the late review, it slipped through on my mailbox.

Could you also add a few docs: https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/api.md#partition-evolution

pyiceberg/table/update/sorting.py

pyiceberg/table/__init__.py

tests/integration/test_sort_order_update.py

fix: set default Co-authored-by: Fokko Driesprong <fokko@apache.org>

chore: update signature Co-authored-by: Fokko Driesprong <fokko@apache.org>

…0/iceberg-python into feat/update-sort-order

JasperHG90 · 2025-02-16T14:27:06Z

No problem @Fokko . Thanks for your comments and your review.

JasperHG90 · 2025-02-16T14:49:58Z

This looks great @JasperHG90 Sorry for the late review, it slipped through on my mailbox.

Could you also add a few docs: https://github.com/apache/iceberg-python/blob/main/mkdocs/docs/api.md#partition-evolution

Added some docs.

kevinjqliu

Hey @JasperHG90 apologies for the delay, ive been tied up with some other stuff.

I've compared the current implementation with java's implementation and it seems like there are a few differences. I've documented them in the comments below.

kevinjqliu · 2025-02-16T20:51:45Z

tests/integration/test_sort_order_update.py

+        SortField(source_id=1, transform=IdentityTransform(), direction=SortDirection.ASC, null_order=NullOrder.NULLS_FIRST),
+        SortField(source_id=2, transform=IdentityTransform(), direction=SortDirection.DESC, null_order=NullOrder.NULLS_LAST),
+        order_id=1,
+    )


modify an existing one. but i see you added both. thanks!

kevinjqliu · 2025-02-16T21:03:04Z

mkdocs/docs/api.md

+
+The API to use when updating a sort order is the `update_sort_order` API on the table.
+
+Sort orders can only be updated by adding a new sort order. They cannot be deleted or modified.


im 90% sure this is right, but want to call it out and see if others agree. cc @Fokko

Yes, this is the case today. There are some efforts going on right now around removing old schemas and partition specs, but if you do this, you want to make sure that there is no reference anymore to the partition spec in the metadata tree. Often you don't see that many sort orders, since it is less likely than adding a new field, so it isn't a problem that you cannot remove them.

Only added above for clarity.

kevinjqliu · 2025-02-16T21:04:38Z

pyiceberg/table/update/sorting.py

+
+class UpdateSortOrder(UpdateTableMetadata["UpdateSortOrder"]):
+    _transaction: Transaction
+    _last_assigned_order_id: int


this is not used

Suggested change

_last_assigned_order_id: int

kevinjqliu · 2025-02-16T21:16:30Z

pyiceberg/table/update/sorting.py

+        """Return the sort order."""
+        return SortOrder(*self._fields, order_id=self._last_sort_order_id + 1)
+
+    def _commit(self) -> UpdatesAndRequirements:


this implementation defers from the java one. heres the code path for the java implementation 1, 2

In particular, the java implementation tries to retry sort order whenever possible.
i dont think self._last_sort_order_id: int = transaction.table_metadata.default_sort_order_id and then using order_id=self._last_sort_order_id + 1 is correct since default_sort_order_id might be always be the highest sort order.
_last_sort_order_id should default to null and only changed when a new sort order is added.

JasperHG90 · 2025-02-17T09:32:02Z

Hey @JasperHG90 apologies for the delay, ive been tied up with some other stuff.

I've compared the current implementation with java's implementation and it seems like there are a few differences. I've documented them in the comments below.

No problem @kevinjqliu . Thanks for getting back to me on this. I'll have a closer look at the Java implementation and docs and address your comments this week.

Have a good week!

JasperHG90 · 2025-02-17T09:52:19Z

tests/integration/test_sort_order_update.py

+        (pytest.lazy_fixture("session_catalog_hive"), "2"),
+    ],
+)
+def test_replace_sort_order(catalog: Catalog, format_version: str, table_schema_simple: Schema) -> None:


Note to self: rename the test to update

JasperHG90 added 22 commits January 5, 2025 20:20

chore: scaffolding

ccfcbcd

chore: scaffolding

95da8f3

chore: add skeleton for asc/desc methods

253967d

chore: scaffolding

7b5a98e

chore: change method names

304a806

chore: update methods

48ac5c0

chore: update methods

a47067c

chore: update imports

c1ab2ec

chore: stupid renames

8f27d14

chore: lint

d8720f2

chore: docstrings

90db60a

test: add integration test for replace sort order

1cd2729

test: add test for lookup

0a1e781

refactor: add last sort order id

a550ccb

refactor: add last sort order id and increment

8b09255

chore: add imports

67b9e52

feat: add apply and commit methods

dcaa63f

test: remove spec stuff

ced6a4b

chore: remove unused import

43e09a3

chore: add ReplaceSortOrder to the Transaction class

b460c34

chore: lint

190071f

Merge branch 'apache:main' into feat/update-sort-order

e9475de

kevinjqliu mentioned this pull request Jan 10, 2025

table.update_schema() continues to commit when exist with an exception #1505

Closed

3 tasks

kevinjqliu reviewed Jan 11, 2025

View reviewed changes

JasperHG90 added 5 commits January 24, 2025 10:12

Merge branch 'apache:main' into feat/update-sort-order

eafafaf

chore: renames (replace to update)

ec5f711

chore: renames (replace to update)

d69a071

test: add test updating sort order

b5a5bd8

refactor: remove the sort order builder

8080fa5

JasperHG90 added 2 commits January 24, 2025 10:36

chore: remove sort order builder

e77a2c1

chore: lint

fc32b28

JasperHG90 requested a review from kevinjqliu January 24, 2025 10:08

chore: update comment

fa1aa50

JasperHG90 changed the title ~~[WIP] Feat: replace sort order~~ Feat: replace sort order Jan 28, 2025

Fokko reviewed Feb 10, 2025

View reviewed changes

pyiceberg/table/update/sorting.py Outdated Show resolved Hide resolved

pyiceberg/table/__init__.py Outdated Show resolved Hide resolved

tests/integration/test_sort_order_update.py Outdated Show resolved Hide resolved

JasperHG90 and others added 8 commits February 16, 2025 15:00

Merge branch 'apache:main' into feat/update-sort-order

1aa6270

test: parametrize over iceberg format versions and remove unnused code

2e9cd3f

chore: fmt

137dbd9

Update pyiceberg/table/update/sorting.py

d8b9001

fix: set default Co-authored-by: Fokko Driesprong <fokko@apache.org>

Update pyiceberg/table/__init__.py

58d302d

chore: update signature Co-authored-by: Fokko Driesprong <fokko@apache.org>

Merge branch 'feat/update-sort-order' of https://github.com/JasperHG9…

0b543b8

…0/iceberg-python into feat/update-sort-order

chore: add arg

fd0e287

chore: fmt

5e57697

JasperHG90 added 2 commits February 16, 2025 15:47

docs: update docs

5131903

chore: set default

cc1ae1c

JasperHG90 requested a review from Fokko February 16, 2025 14:50

kevinjqliu requested changes Feb 16, 2025

View reviewed changes

JasperHG90 commented Feb 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: replace sort order #1500

Feat: replace sort order #1500

JasperHG90 commented Jan 8, 2025

kevinjqliu left a comment •

edited

Loading

kevinjqliu Jan 11, 2025

JasperHG90 Jan 24, 2025

kevinjqliu Jan 11, 2025

JasperHG90 Jan 24, 2025 •

edited

Loading

kevinjqliu Feb 16, 2025

kevinjqliu Jan 11, 2025

JasperHG90 Jan 24, 2025

JasperHG90 Jan 24, 2025

JasperHG90 commented Jan 16, 2025

Fokko left a comment

JasperHG90 commented Feb 16, 2025

JasperHG90 commented Feb 16, 2025

kevinjqliu left a comment

kevinjqliu Feb 16, 2025

kevinjqliu Feb 16, 2025

Fokko Feb 17, 2025

JasperHG90 Feb 17, 2025

kevinjqliu Feb 16, 2025

kevinjqliu Feb 16, 2025

JasperHG90 commented Feb 17, 2025

JasperHG90 Feb 17, 2025

		from pyiceberg.table import Transaction


		class SortOrderBuilder:


		The API to use when updating a sort order is the `update_sort_order` API on the table.

		Sort orders can only be updated by adding a new sort order. They cannot be deleted or modified.

Feat: replace sort order #1500

Are you sure you want to change the base?

Feat: replace sort order #1500

Conversation

JasperHG90 commented Jan 8, 2025

kevinjqliu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasperHG90 Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasperHG90 commented Jan 16, 2025

Fokko left a comment

Choose a reason for hiding this comment

JasperHG90 commented Feb 16, 2025

JasperHG90 commented Feb 16, 2025

kevinjqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasperHG90 commented Feb 17, 2025

Choose a reason for hiding this comment

kevinjqliu left a comment •

edited

Loading

JasperHG90 Jan 24, 2025 •

edited

Loading