Conversation
…pdate error expectations
…in key The error message renders the type from the Iceberg table's pyarrow schema, and schema_to_pyarrow converts pa.list_ into pa.large_list (see pyiceberg/io/pyarrow.py). The test regex must match the rendered large_list<element: int32>, not the source list<item: int32>.
A pa.null() source column was being rejected by _check_pyarrow_schema_compatible (format-version=2 forbids null) before the join-column validation could surface the intended "Null-type column ... cannot be used as a join key" error. Reordering the checks lets the upsert-specific rejection fire first, giving users the actionable message. Dataframe-level checks now skip columns that are absent from the source so the pre-existing _check_pyarrow_schema_compatible path still owns the "PyArrow table contains more columns" error in test_key_cols_misaligned.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Still active - happy to address any feedback. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Still active - happy to address any feedback. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Still active - happy to address any feedback. |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Can you update the upsert docs in api.md? That'll help a lot for users to know which join keys are supported.
Move join column validation from Transaction.upsert into upsert_util.validate_join_cols and reject every key that previously reached PyArrow and crashed: columns missing from the source, UUID and view types, null values, duplicate names. Add test matrices for rejected and accepted keys and document them.
# Conflicts: # tests/table/test_upsert.py
Name the Iceberg table "the table" and the Arrow argument "the input" consistently, point nested paths at the struct they live in instead of saying "top-level", and finish each message with what to do about it.
| <!-- prettier-ignore-start --> | ||
|
|
||
| <!-- markdownlint-disable MD046 -- Allowing indented multi-line formatting in admonition--> | ||
|
|
There was a problem hiding this comment.
This change is to fix the list rendering here: https://py.iceberg.apache.org/api/#files:~:text=Name%20Mapping%20and%20Field%20IDs
I figured to fix this in this same PR since I had to apply this to get lists to render in the upsert doc change in this same file.
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This seems very reasonable and I love the extra validation. I've got some style concerns around the tests, but this looks great. Thanks for doing this!
|
|
||
| @pytest.mark.parametrize( | ||
| "table_type, source_key, expected_error, match", | ||
| [ |
There was a problem hiding this comment.
This is a personal preference, but I think the test parameterization makes the tests messy. Some ideas:
- A helper method and then calling that helper method multiple times.
- Comments for the parameters to help make it more obvious what some of these tests are doing
3 participants
Rationale for this change
Table.upsertmatches rows on the join columns, but it never checked that those columns were usable. Bad keys reached PyArrow and failed there with a kernel error, a rawKeyError, or in one case silently wrong data. This PR validates the join columns first, inupsert_util.validate_join_cols, and raises a clear error before anything is written.With this change three kinds of problems are now caught:
join_colsitself is malformed: duplicate names, or not a list.This PR disallows floats as join columns because Arrow and Iceberg disagree on whether
-0.0equals0.0, which left duplicate keys in the table, and NaN cannot be expressed as a filter.Are these changes tested?
Yes.
tests/table/test_upsert.pyhas three parametrized matrices: keys that must be rejected, invalidjoin_cols, and every supported key type verified by reading the table back. Each rejection case also asserts that no snapshot was written.Are there any user-facing changes?
Floating-point join keys now raise instead of working. Ordinary float keys did upsert correctly before, so this is a deliberate trade-off for the
-0.0and NaN cases above. Dictionary-encoded keys also raise, since string dictionaries broke the table on read. Every other rejected case was already failing, just with a worse error. The Upsert docs inapi.mdnow list which join keys are supported.For full disclosure, this PR was developed with the assistance of an AI coding assistant.