Skip to content

Conversation

rambleraptor
Copy link
Contributor

Rationale for this change

PyIceberg is missing a variety of Catalog Tests compared to the Java implementation. This adds 3 different tests for schema evolution to help match the Java library coverage.

Are these changes tested?

Tests should pass.

Are there any user-facing changes?

Just tests.

Copy link
Contributor

@gabeiglio gabeiglio left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! the tests are looking good! Left some small comments 👍🏼

Comment on lines +403 to +404
table.update_schema().add_column("col1", StringType()).add_column("col2", StringType()).add_column(
"col3", StringType()
).commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit to format for better readability (if the linter allows it)

Suggested change
table.update_schema().add_column("col1", StringType()).add_column("col2", StringType()).add_column(
"col3", StringType()
).commit()
table.update_schema()
.add_column("col1", StringType())
.add_column("col2", StringType())
.add_column( "col3", StringType())
.commit()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter deeply does not like this (even with the backslashes to make it syntactically correct)

@rambleraptor rambleraptor requested a review from Fokko September 22, 2025 22:06
@rambleraptor rambleraptor force-pushed the schema_update_catalog_tests branch from deb1b89 to 839e774 Compare September 22, 2025 22:07
@saul-data
Copy link

Please add a test that adds data to a schema after evolution, you will notice it doesn't pick up the latest schema: #2467

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.

5 participants