-
Notifications
You must be signed in to change notification settings - Fork 289
Make add_files
to support snapshot_properties
argument
#695
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
Make add_files
to support snapshot_properties
argument
#695
Conversation
@enkidulan - thank you for working on this. I think https://github.com/apache/iceberg-python/blob/main/tests/integration/test_add_files.py is the right place for these tests |
@@ -122,8 +123,13 @@ def _create_table( | |||
return tbl | |||
|
|||
|
|||
@pytest.fixture(name="format_version", params=[pytest.param(1, id="format_version=1"), pytest.param(2, id="format_version=2")]) | |||
def format_version_fixure(request: pytest.FixtureRequest) -> Iterator[int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making params more human-friendly:
test_add_files_to_unpartitioned_table[format_version=1] PASSED
test_add_files_to_unpartitioned_table[format_version=2] PASSED
..
# instead of
test_add_files_to_unpartitioned_table[1] PASSED
test_add_files_to_unpartitioned_table[2] PASSED
Thanks @syun64. I've added the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for adding this in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! love the format_version
change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great! Thanks @enkidulan.
I pushed a small update to make Python 3.8 tests pass. I had to replace |
@enkidulan Thanks for fixing the test and the great work! Thanks @syun64 and @kevinjqliu for reviewing. Merging! |
Issue #694
I'm not sure where to add tests. Is
tests/catalog/test_glue.py
the right place?