Skip to content

Add support for evolving a partition column #1334

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 6 commits into
base: main
Choose a base branch
from

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented May 15, 2025

Which issue does this PR close?

To check what happens if a partition column is evolved.

What changes are included in this PR?

Are these changes tested?

@Fokko Fokko force-pushed the fd-evolve-partition branch 2 times, most recently from 1b10b21 to e71b0bb Compare May 15, 2025 13:32
@Fokko Fokko force-pushed the fd-evolve-partition branch from e71b0bb to 9a9174e Compare May 15, 2025 13:37
@Fokko Fokko marked this pull request as ready for review May 15, 2025 14:19
@Fokko Fokko changed the title Evolve a partition column Add support for evolving a partition column May 16, 2025
kevinjqliu
kevinjqliu previously approved these changes May 17, 2025
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM

should we also add the same for timestamp, timestamp_ns, and decimal?
just to complete all lines in this table https://iceberg.apache.org/spec/#schema-evolution

Comment on lines 117 to 118
# Create a table, and do some evolution on a partition column
spark.sql("CREATE OR REPLACE TABLE rest.default.test_promote_column (foo int) USING iceberg PARTITIONED BY (foo)")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit I think we should keep the original test and just create another table for evolution on a partition column

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -403,12 +403,26 @@ impl Datum {
}
}
PrimitiveType::Int => PrimitiveLiteral::Int(i32::from_le_bytes(bytes.try_into()?)),
PrimitiveType::Long => PrimitiveLiteral::Long(i64::from_le_bytes(bytes.try_into()?)),
PrimitiveType::Long => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Relevant section from the spec: https://iceberg.apache.org/spec/#schema-evolution

Copy link
Contributor Author

@Fokko Fokko May 19, 2025

Choose a reason for hiding this comment

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

I first did only the V2 transformations for now. I've added a test for the float and decimal case as well 👍 Decimal works with the current code since we don't have a fixed number of bytes in contrast to int/long.

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM!

Looks like we need to run cargo fmt

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.

2 participants