fix(upsert): bound file match filter to avoid segfault#3420
Open
abnobdoss wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
This fixes the initial upsert scan-planning segfault / recursion failure caused by large composite-key match filters. It should also improve planning performance because the current exact PyIceberg expression tree is expensive to construct, bind, and evaluate for large source batches.
For composite join keys, the initial upsert scan currently builds one
And(EqualTo(...), EqualTo(...))predicate per unique source key and reduces them into a left-leaningOrtree. Large source batches therefore create deep recursive expression trees before the target scan starts; expression visitors then walkOr.left/Or.rightrecursively, so stack usage scales with the number of unique key combinations.This change adds a bounded
create_file_match_filterfor the initial target file-pruning scan. It uses per-column min/max bounds, plusIsNullwhere needed, so the initial scan predicate size is tied to the number of join columns rather than the number of source rows. The exactcreate_match_filteris still used later for overwrite and insert filtering, so row-level correctness is unchanged.The tradeoff is that the bounded file-pruning predicate is conservative: for composite keys it can scan cross-product false positives, which downstream exact matching culls before update/insert decisions. This can technically increase the number of files scanned, but the current failure mode makes upsert impossible for affected large composite-key inputs. The intent of this PR is to remove that crash first; if there is appetite to improve pruning further, a follow-up can use a more selective bounding-box or more sophisticated algorithm while preserving the bounded expression shape.
Match-filter timings below include expression construction, schema binding, PyArrow expression conversion, and applying the resulting Arrow filter to an in-memory table. These do not measure end-to-end upsert time, which depends on table layout, file pruning, IO, and downstream exact matching.
Benchmark script: https://gist.github.com/abnobdoss/824698ea74c29c02ead8da6410b89b6e
Current exact match-filter path:
Bounded file-match filter path after this PR:
Follow-up work should address the remaining exact match-filter recursion paths used later during overwrite and insert filtering. Those predicates need exact semantics, so they should be fixed separately.
Are these changes tested?
Yes.
Added regression coverage for the large composite-key initial scan path, plus focused coverage for bounded file-match filter construction and the conservative false-positive culling path.
Ran:
uv run python -m pytest tests/table/test_upsert.py -k "large_composite_key_initial_scan or file_match_filter or create_match_filter"uv run python -m pytest tests/table/test_upsert.pyThe added
test_upsert_large_composite_key_initial_scan_does_not_recurseregression test runs the large composite-key upsert in a subprocess so runtimes that would segfault report a pytest failure instead of terminating the pytest worker; the equivalent base-branch path exits139under Python 3.10, while this branch exits successfully.Are there any user-facing changes?
No API changes. Upserts with large composite-key source batches should avoid the initial scan-planning recursion failure and may scan a conservative superset of candidate files before exact row matching.