iceberg-python icon indicating copy to clipboard operation
iceberg-python copied to clipboard

Feat: replace sort order

Open JasperHG90 opened this issue 11 months ago • 5 comments

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!

JasperHG90 avatar Jan 08 '25 21:01 JasperHG90

Thanks for the comments šŸ™Œ @kevinjqliu . Will look at your comments this weekend.

JasperHG90 avatar Jan 16 '25 20:01 JasperHG90

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

JasperHG90 avatar Feb 16 '25 14:02 JasperHG90

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.

JasperHG90 avatar Feb 16 '25 14:02 JasperHG90

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 avatar Feb 17 '25 09:02 JasperHG90

@kevinjqliu @Fokko do either one of you have time to review this PR in the coming weeks? I have some time on my hands and I’d love to wrap this PR up! Thanks in advance

JasperHG90 avatar Apr 15 '25 08:04 JasperHG90

Hey @JasperHG90 sorry for the late reply, the notification got buried in my mailbox. I'll definitely review this, it would be great to get this in šŸ‘

Fokko avatar May 16 '25 21:05 Fokko

Hey @JasperHG90 sorry for the late reply, the notification got buried in my mailbox. I'll definitely review this, it would be great to get this in šŸ‘

No apology necessary. Happy to receive your review!

JasperHG90 avatar May 19 '25 08:05 JasperHG90

Hello, any update on this PR?

mwa28 avatar Jul 08 '25 15:07 mwa28

@JasperHG90 Sorry for the late reply, and chance that you could revisit this PR?

Fokko avatar Aug 04 '25 12:08 Fokko

@JasperHG90 are you still working on this? I'm happy to pick it up if you're not

rambleraptor avatar Sep 16 '25 21:09 rambleraptor

We could also cover testing against all catalogs via the test_catalog.py in the integration tests!

jayceslesar avatar Sep 18 '25 00:09 jayceslesar

Hi all. Apologies for my late response. TBH this has slipped from my mind in the past couple of months due to work. @jayceslesar and @rambleraptor thanks for your comments. I have some time on my hands the coming couple of weeks, so I'd be happy to colab on this to finish it up if you like. If you'd like to steamroll ahead and implement it ASAP because you need it, that's also fine by me.

JasperHG90 avatar Sep 22 '25 13:09 JasperHG90

Im certainly in no rush so happy to let you see this through! Would be epic to have this in the next minor release (0.11.0) !

jayceslesar avatar Sep 22 '25 14:09 jayceslesar