Skip to content

Conversation

gabeiglio
Copy link
Contributor

Just adding more tests to the catalog tests.

with table.transaction() as transaction:
with transaction.update_schema() as update_schema:
update_schema.add_column("new_col", IntegerType())
expected_schema = update_schema._apply()
Copy link
Contributor

Choose a reason for hiding this comment

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

this apply is just to get the new schema back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather hardcode the schema:

  • This makes the test more readabile because you can see what you're asserting against.
  • There are some idea's of avoid using "private" methods in the community ;)

@gabeiglio gabeiglio requested a review from Fokko September 12, 2025 20:55
Copy link
Contributor

@jayceslesar jayceslesar left a comment

Choose a reason for hiding this comment

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

honestly wild how much we are covering with these tests

Copy link
Contributor

@rambleraptor rambleraptor left a comment

Choose a reason for hiding this comment

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

This is awesome. Thank you!

@vinjai
Copy link
Contributor

vinjai commented Sep 22, 2025

Impressive work @gabeiglio. This would really help in the long run.

@gabeiglio
Copy link
Contributor Author

@kevinjqliu @Fokko this might have fell of the radar, wondering if you folks could review it?

Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

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

Thank you all for the reviews! @jayceslesar @rambleraptor @vinjai

And thanks @gabeiglio for the contribution. This is a great addition to the test suite.

Copy link
Contributor

@Fokko Fokko 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 pinging me @gabeiglio This looks great 🙌

@Fokko Fokko merged commit 6020f24 into apache:main Oct 9, 2025
10 checks passed
@Fokko
Copy link
Contributor

Fokko commented Oct 9, 2025

Thanks for working on this @gabeiglio, and thanks @vinjai, @rambleraptor, @jayceslesar and @sungwy for the review 🚀

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.

6 participants