[GaspardMerten]

Rationale for this change

Follow-up to #4334, which made get_rows_to_update compare the matched rows column by column. Columns PyArrow cannot compare still fall back to to_pylist(), and struct columns always do: PyArrow has no comparison kernel for structs, and a scan returns the strings inside a struct as large_string while the dataframe holds string, so the two struct types never match.

This PR:

• compares struct columns field by field in PyArrow, recursing into nested structs. A null struct is only equal to another null struct.
• when PyArrow refuses to compare two columns because their types differ, casts the source column to the target type and compares again, before falling back to Python.

Lists and maps still go through Python.

Before / after

table.upsert() of a table with 30 struct<a: long, b: string> columns, where the given share of rows has one changed struct column. SqlCatalog, local files, best of 3 runs (2 for 1M). "Comparison" is the time spent in get_rows_to_update. "main" includes #4334.

Rows	Comparison (main → this PR)	Upsert, 1% changed	Upsert, 10% changed
10k	132 ms → 14 ms (9×)	458 ms → 321 ms (1.4×)	505 ms → 331 ms (1.5×)
100k	1.43 s → 86 ms (17×)	3.21 s → 1.93 s (1.7×)	3.44 s → 1.96 s (1.8×)
1M	14.82 s → 1.79 s (8×)	31.33 s → 18.39 s (1.7×)	33.03 s → 19.76 s (1.7×)

Tables without struct columns are unchanged, as expected after #4334 (200 long and string columns, 100k rows: ≈1.45s on both).

With this change, the comparison is no longer the main cost of an upsert. On the 1M-row struct table, most of the remaining time goes into building the key filters (create_match_filter and binding a 1M-value In), which is what the open #3420 and #4027 address. For context, upserts of the same 1M-row table on top of this PR, with nothing changed, and with every struct column changed in 10% of rows plus 100k new rows (one integer key, contiguous values, local files):

	No changes	10% changed + 100k new rows
main	31.4s	41.2s
this PR	16.8s	24.2s
this PR + #4027	14.2s	20.0s
this PR + #3420	7.5s	10.8s
this PR + #3420 + #4027	4.3s	8.2s

Combining #3420 and #4027 needed a one-line conflict resolution.

The cast also changes the result for columns whose types differ but hold the same values. Calling get_rows_to_update with a naive timestamp column against a UTC one holding the same instants flags every row as changed on main, since the Python fallback compares a naive and an aware datetime. With this PR none are flagged:

target = pa.table({"id": [1, 2], "ts": pa.array([1, 2], pa.timestamp("us", tz="UTC"))})
source = pa.table({"id": [1, 2], "ts": pa.array([1, 2], pa.timestamp("us"))})
len(get_rows_to_update(source, target, ["id"]))  # main: 2, this PR: 0

table.upsert() itself rejects that schema before getting there, so this only affects direct callers.

Are these changes tested?

Yes, new tests in tests/table/test_upsert.py:

• a null struct against a struct, and a struct with a null field
• nested structs, including a null inner struct
• struct fields in a different order
• a struct of string against a struct of large_string, as a scan returns it
• a naive against a zoned timestamp

The unit suite passes, as does prek run on the changed files.

Are there any user-facing changes?

Only speed, for tables with struct columns.

[Copilot]

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.

[hedger9487]

Great optimization on pushing wide-table comparisons to PyArrow Compute (~130x speedup is awesome)!

A couple of quick thoughts while looking through the implementation:

1. Positional field access in structs: In _get_changed_struct_mask, using integer index in pc.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 with ArrowNotImplementedError. Would matching subfields by field.name be safer here?

2. 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 on ArrowNotImplementedError for 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?

[GaspardMerten]

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 not_equal, which might evolve over the years. We shouldn't treat this as normal, but hope it gets fixed someday. Hence, in a way, the current approach feels in mho more adapted to the situation; what do you think?

[hedger9487]

Hey @GaspardMerten,

Makes sense! Glad the field.name adjustment was helpful.

My thought was just that intentionally hitting exceptions during normal runs felt a bit counter-intuitive, but using except to explicitly signal an upstream shim is an interesting angle I hadn't considered!

Definitely no need to refactor on my account — keeping it as-is makes total sense. Thanks for explaining and for the awesome optimization!

[GaspardMerten]

@rambleraptor, would you have time to review this? Thanks a lot, currently had to patch a pipeline in an unfriendly way..

[github-actions]

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.

[GaspardMerten]

@ragnard @gardenia

Hi,

Would you be available to review this PR?

Best,

Gaspard

[github-actions]

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.

[rambleraptor]

I've got some thoughts, but this broadly looks good. Thanks for the contribution!

[github-actions]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

[GaspardMerten]

Hey @Fokko, @rambleraptor!

First of all, thank you for reopening the PR and for your feedback.

Since #4334 landed in the meantime, I rebased on main and narrowed the PR to the two things that were left:

• struct columns were still compared with the Python fallback. They are now compared field by field in PyArrow.
• columns whose types differ are now cast before comparing. That covers a struct of string against the struct of large_string a scan returns (which is what kept structs in the Python fallback), and a naive against a zoned timestamp, which flagged every row as changed.

This also resolves both review comments: the check on the source and target columns that raised a ValueError is gone, so an empty target returns an empty table as on main, and the tests no longer monkeypatch private functions; they only check the returned rows.