Upsert wide table memory - #3862
Conversation
1d2a776 to
fe4bb00
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR optimizes upsert row-difference detection for wide tables by moving comparisons into PyArrow Compute (with a structured fallback for complex/nested types), reducing Python scalar materialization and improving performance.
Changes:
- Add column-at-a-time change masking in
get_rows_to_update, including nested struct comparison and a sliced Python fallback for non-comparable types. - Add explicit source/target column-name validation with a new error message.
- Add a focused test suite covering null semantics, nested structs, list/map behavior, casts, and comparison strategy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyiceberg/table/upsert_util.py |
Reworks matched-row comparison to use PyArrow Compute masks per column, adds nested-struct handling and a sliced Python fallback, and introduces explicit column-name validation. |
tests/table/test_upsert.py |
Adds tests verifying new comparison semantics and ensuring comparisons happen per column (and that structs avoid Python comparisons). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fe4bb00 to
b57ac73
Compare
hedger9487
left a comment
There was a problem hiding this comment.
Great optimization on pushing wide-table comparisons to PyArrow Compute (~130x speedup is awesome)!
A couple of quick thoughts while looking through the implementation:
-
Positional field access in structs: In
_get_changed_struct_mask, using integerindexinpc.struct_field(..., index)assumes identical struct field order between source and target. If field orders differ (e.g.{"a": int, "b": str}vs{"b": str, "a": int}), it compares mismatched types and crashes withArrowNotImplementedError. Would matching subfields byfield.namebe safer here? -
A small thought on structure: I was wondering if it might also be cleaner to check
if pa.types.is_struct(...)up front rather than relying onArrowNotImplementedErrorfor branching? That way we could avoid exception-driven control flow and maybe even unify both helpers into a single recursive function. What do you think?
1364666 to
790d533
Compare
get_rows_to_update compared the matched rows one cell at a time, one PyArrow call per row and per column, so an upsert got slower with every column of the table. It compares one column at a time now: 20k matched rows over 200 columns takes about 0.15s instead of about 20s. PyArrow cannot compare struct columns, so a struct is compared field by field, recursing into the structs its fields hold. Lists and maps are still compared in Python, a slice at a time so the objects of a whole column are never held at once. When PyArrow refuses to compare two columns because their types differ, the source is cast to the type of the target. The full table cast this removes used to do that, and without it a naive timestamp and a zoned one holding the same instant are reported as different on every run. Closes apache#3860
790d533 to
10a0461
Compare
|
Hey @hedger9487, Thanks for the review. For 1, the function was called only when the types matched; it wouldn't have caused a bug, but if it were used elsewhere, it could, so I followed your advice. On 2, my opinion is to keep the code as is, or, if needed, to refactor the except branch into a separate function, as it really is a fallback for the limitations of |
|
Hey @GaspardMerten, Makes sense! Glad the My thought was just that intentionally hitting exceptions during normal runs felt a bit counter-intuitive, but using Definitely no need to refactor on my account — keeping it as-is makes total sense. Thanks for explaining and for the awesome optimization! |
|
@rambleraptor, would you have time to review this? Thanks a lot, currently had to patch a pipeline in an unfriendly way.. |
Rationale for this change
upsertcompares the matched rows one cell at a time. For every matched row, it takes a one-row slice of the source and of the target, then calls.as_py()on each non-key column until it finds a difference. That is one PyArrow call per rowand per column, so the cost grows with the size of the table and not with the amount of data that actually changed.
On a table with 200 columns, comparing 20k matched rows takes around 20 seconds before anything is written on a standard computer (mine), while it takes 0.15s if we push this comparison logic to PyArrow Compute (PC).
Two other things come out of that:
timestamp[us]were not cast totimestamp[us, UTC]as PyArrow refuses, it reverted back to Python, which made every row reported as changed. This is a small thing we gain.! Lists and maps have no fields to compare and still go to Python, a slice at a time so the objects of a whole column are never held at once.
The result is the same: two nulls still count as equal, a null and a value still count as a change, and rows whose non-key columns did not change are still skipped.
Are these changes tested?
Yes, 10 tests in
tests/table/test_upsert.py.Eight cover the comparison itself: nulls, a struct that is null, a nested struct, a list column, a column whose type differs from the target, a cast PyArrow refuses, a source missing one of the target columns, and no match at all.
Two cover the change in the way rows are compared, which no assertion on the result can see, because both ways return the same rows. One counts the comparisons and checks there is one per column, whatever the number of rows. The other checks a struct never reaches the Python comparison, with the types the upsert path really produces (a scan reads a
stringas alarge_string).The existing
tests/table/test_upsert.pyandtests/tablesuites pass unchanged.Are there any user-facing changes?
No API or behaviour change.
get_rows_to_updatereturns the same rows.One error message changes. A source that does not have every column of the target was rejected by
Table.castwithTarget schema's field names are not matching the table's field names. It is now rejected by an explicit check, with a message naming the source. Without that check, the missing columns would never be compared and wouldbe written as null.