feat(transaction): Implement TransactionAction for updata_loc, update_props, and upgrade_format

[CTTY]

Which issue does this PR close?

Related Issues:

• [EPIC] Support for appending data to iceberg table. #1382 [EPIC]
• Add a method in TableCommit to apply changes to TableMetadata to create a new TableMetadata #1386
• Add retry in Transaction::commit method #1387
• Implement saving TableMetadata to new location. #1388
• Swith metadata pointer to new location for different catalogs(sql, hive, glue, etc) #1389

What changes are included in this PR?

• Added implementations of TransactionAction
  • UpdateLocationAction
  • UpdatePropertiesAction
  • UpgradeFormatVersionAction
• Added as_any to TransactionAction trait
• Added do_commit in Transaction to commit applied actions

We will also need to implement TransactionAction for FastAppendAction and ReplaceSortOrderAction, and I'm planning to do that in a separate PR.

Are these changes tested?

Added unit tests

[CTTY]

I added this mainly for testing/inspecting the content of applied TransactionAction s

[liurenjie1024]

This could be a provided method.

[CTTY]

I played with this a little bit but it seems like this has to be done within concrete implementations. With code like below:

    fn as_any(self: Arc<Self>) -> Arc<dyn Any> {
        self
    }

And this will fail to the error:

   |
43 |         self
   |         ^^^^ doesn't have a size known at compile-time

It seems that even if self is an Arc<T>, the contained T still has to be Sized to be cast to Any in a provided method. Adding where Self: Sized bound to the method directly will make it uncallable on Arc<dyn TxAction> so it won't work as well

[liurenjie1024]

How about using as_any? This way we don't need to implement this for each action:

pub(crate) trait TransactionAction: AsAny {
...
}

[CTTY]

Thanks Renjie, I gave it a try and it's working fine except some syntax sugar to use it on Arc:

        let action = (*tx.actions[0]) // this
            .downcast_ref::<UpdateLocationAction>()
            .unwrap();

This is still much cleaner than using Any directly!

[liurenjie1024]

Thanks @CTTY for this pr, generally looks good!

[liurenjie1024]

This could be a provided method.

[liurenjie1024]

How about using as_any? This way we don't need to implement this for each action:

pub(crate) trait TransactionAction: AsAny {
...
}

[liurenjie1024]

Thanks @CTTY for this pr, LGTM!

[dentiny]

Ooops, it's an interface change, one minor suggestion, better to call it out in PR description and later release notes. :)
It actually breaks moonlink compilation :(

[CTTY]

Yes, we are making TransactionActions retryable and had to change the existing APIs. There are more API changes coming for other actions like ReplaceSortOrderAction(#1441) and FastAppendAction(WIP)

We definitely need to call this out in the next release notes. cc: @liurenjie1024

I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev? You can set up a non-blocking github CI to track iceberg-rs/main if you want to catch compatibility issues early

[dentiny]

We definitely need to call this out in the next release notes.

Thanks!

[dentiny]

I'm not very familiar with moonlink and here goes the dumb question: Would it be better for moonlink to depend on a released/stable iceberg-rs version instead of tracking an unreleased git rev?

That's the ideal case, but I think the interval between iceberg-rust releases are too long.

You can set up a non-blocking github CI to track iceberg-rs/main if you want to catch compatibility issues early

It's not too much of an overhead as of now, because I sync iceberg-rust upstream on weekly basic.

[CTTY]

The release interval certainly is an issue 😄

[liurenjie1024]

Thanks @dentiny for this comment. I thought about keeping the original api as deprecated, but the concern was to the overhead of maintaining it. But I do agree that we should shoutout this breaking change when release.