Skip to content

docs: add Transaction example #1436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 25, 2025

Conversation

jdockerty
Copy link
Contributor

@jdockerty jdockerty commented Jun 12, 2025

Which issue does this PR close?

N/A, although I can link this to the Write support epic is that is useful?

What changes are included in this PR?

Adding additional documentation to the transaction module to provide a basic example of the FastAppendAction usage.

Other actions are not included at this time.

it looks like this

image

Are these changes tested?

N/A

@jdockerty jdockerty force-pushed the docs/transaction-example branch from 433a900 to 17d3e20 Compare June 12, 2025 11:49
@jdockerty jdockerty marked this pull request as ready for review June 12, 2025 12:00
@CTTY
Copy link
Contributor

CTTY commented Jun 12, 2025

Hi @jdockerty , thanks for this PR! I do think adding more documentation is necessary since iceberg-rs's tx semantics is slightly different from iceberg-java. There is an ongoing effort of standardlizing transaction api and will change the existing usages a little bit, maybe we can wait until that work is finished and add the new doc later:

@jdockerty
Copy link
Contributor Author

@CTTY This sounds reasonable to me, thanks for the heads up 💯

For my understanding, the linked PRs are fundamentally changing the Transaction usage and splitting things up?

E.g. there are more well-defined actions on the new path, such as an UpdateLocationAction.

It doesn't look like this affects the FastAppendAction, which is what this doc PR adds an example for. Is this code path changing soon in a follow-up, after the other PRs have been merged?

@CTTY
Copy link
Contributor

CTTY commented Jun 14, 2025

It doesn't look like this affects the FastAppendAction, which is what this doc PR adds an example for. Is this code path changing soon in a follow-up, after the other PRs have been merged?

Yes, the ongoing refactoring effort is for all transaction actions. I'm planning to work on FastAppendAction next and hopefully it will get in by next weekend

@CTTY
Copy link
Contributor

CTTY commented Jun 23, 2025

Hi @jdockerty , with the closing of #1451, Transaction API semantic should be finalized as of now. Please feel free to resume this effort!

@jdockerty
Copy link
Contributor Author

@CTTY I've aligned this to the new path in f9d66d3, showcasing the new ApplyTransactionAction trait requirement etc.

Do you mind taking another peek at this when you get a chance? 🙇

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @jdockerty for this pr, looks good to me!

@liurenjie1024 liurenjie1024 merged commit 6c25387 into apache:main Jun 25, 2025
17 checks passed
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