Skip to content

feat: add an AddFieldsAction to transaction #1176

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cmcarthur
Copy link

Which issue does this PR close?

What changes are included in this PR?

  • Adds a new AddFieldsAction which represents an addition of new fields to an existing table
  • Adds a new method to Transaction add_fields which generates an AddFieldsAction
  • When applied, the AddFieldsAction will register a new schema with the addition of the new fields, and set the current schema version to (schema + 1). (I am not highly confident that this is the right way to manage the schema id, though I've verified that it works in my testing. Is there a better way to mark the next schema version as the current one? Is this necessary?)

Are these changes tested?

Added tests:

Unit: in crates/iceberg/src/transaction/add_field.rs
Integration: in crates/integration_tests/tests/shared_tests/add_field_test.rs

let updates = vec![
TableUpdate::AddSchema { schema },
TableUpdate::SetCurrentSchema {
schema_id: schema_id + 1,
Copy link
Author

Choose a reason for hiding this comment

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

Is this even necessary? Will the new schema version be set to current by default? If not, is there a better way to "increment" the schema version?

@jonathanc-n
Copy link
Contributor

jonathanc-n commented Apr 7, 2025

I believe this functionality should be covered in a schema update action. It does seem nice to have an easier way to add a field. What are your thoughts? cc @liurenjie1024 @Fokko

@cmcarthur
Copy link
Author

Thanks for taking a look @jonathanc-n. Of course, I saw your PR for schema update after I opened this 😀. I will await feedback from the others!

@ZENOTME
Copy link
Contributor

ZENOTME commented Apr 13, 2025

I believe this functionality should be covered in a schema update action. It does seem nice to have an easier way to add a field. What are your thoughts? cc @liurenjie1024 @Fokko

+1 for this. Seems that how iceberg-java do: https://github.com/apache/iceberg/blob/97d1107d60b42d29306c663f23f869fc1d439e6b/api/src/main/java/org/apache/iceberg/UpdateSchema.java#L69

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.

Implement an AddFields operation
3 participants