fix(merge_insert): make analyze_plan follow execute's source routing - #8772
Merged
wjones127 merged 9 commits intoSep 2, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest merge from main preserves the feature patch. Materialized analysis still follows the statistics-bearing provider route used by execution, streams remain streaming, and the routing and join-selection regressions pass against the updated base.
wjones127
approved these changes
Sep 2, 2026
wjones127
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Thanks for the fix!
Contributor
Author
|
Thank you @wjones127 |
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.
What this fixes
MergeInsertBuilder.analyze_plan(data)coerced every input to a one-shot stream, so it reported the streaming plan even whenexecute(data)on the same input would run a different one (#8771).Which side of the hash join gets collected, and the join type, both follow the statistics the source reports.
executesends a materialized source through an in-memory table that reports an exact row count and byte size, and DataFusion'sJoinSelectionpicks the collected side from that. A stream reports nothing.analyze_planreported the wrapping it chose rather than the oneexecutewould choose, so anyone profiling a merge read metrics off the wrong side of the join.analyze_plannow dispatches on_is_materializedexactly asexecutedoes.What the diagnostic printed, and what it prints now
The docstring example in
dataset.pypasses apa.table. Before, for that input:After:
The second one is what
executehas been running all along. The doctest asserted the first.Rust surface
MergeInsertJobgainsanalyze_plan_batchesandanalyze_plan_provider, mirroring the existingexecute_batchesandexecute_provider.analyze_plan(stream)keeps its signature and delegates to the provider entry, so external Rust callers still compile and a stream is still reported as a stream.Two doc corrections came out of reviewing this.
explain_plannow says outright that it only ever reports the streaming shape, because it receives a schema rather than data and so cannot know how the source would be wrapped; it also points atanalyze_planwhile noting thatanalyze_planruns the merge and may write data files, whichexplain_plandoes not. Andanalyze_plan_batchesdocuments the two cases where it reports the streaming shape anyway:SourceDedupeBehavior::FirstSeenre-wraps the source in a stream ahead of the join, and an empty batch list carries no schema so the provider falls back to the dataset's.What this does not change
No execution behaviour.
executealready routed materialized sources through the in-memory table; only the diagnostic was out of step with it.A materialized
analyze_plannow collects the reader into memory in Rust before planning, where it used to stream. The inputs_is_materializedaccepts are already fully in memory, so the extra copy is bounded by data the caller holds, and it is the same copyexecutehas always made.The source types that could report statistics but do not are untouched.
lance.LanceDataset,pa.dataset.Dataset, andpa.dataset.Scannerall arrive as a bare reader through_coerce_readereven though each knows its row count and can be scanned again, and the default streaming path drains the whole source into a spill before reporting no statistics at all. Both are remaining bullets on #4583, and this change is what makes their effect visible from Python.One pre-existing gap this touches without fixing:
batches_to_providerfalls back to the dataset's schema when the batch list is empty, so a zero-batch materialized source is validated against the target's columns rather than its own.execute_batchesandexecute_uncommitted_batcheshave always done this, and closing it changesexecute's public behaviour from a silent no-op to an error, which needs its own change and its own tests.One drive-by, disclosed rather than hidden:
explain_plan's not-supported message said only full-schema sources are supported.can_use_create_planaccepts a subset schema and, for a delete-only merge, the join keys alone, and its own doc comment lists all three. Rewriting the sibling message on theanalyze_planpath made the two contradict each other, so both now name the two real reasons instead. Thedoes not support explain_planprefix that four tests match on is unchanged.Test plan
test_merge_insert_analyze_plan_matches_execute_routing: apa.Tablesource must reportDataSourceExecandjoin_type=Left, aRecordBatchReadermust reportStreamingTableExecandjoin_type=Right. The first assertion fails without the dispatch change.test_analyze_plan_reports_the_given_source_shapecovers the three Rust entries, includinganalyze_plan_providerdirectly.test_plan_join_build_side_follows_source_statisticspins which side the join collects at both of DataFusion's decision points: pasthash_join_single_partition_threshold_rowswhere only the source can be collected, and below it where the smaller side wins.cargo test -p lance --lib merge_insert -- --test-threads=1: 220 pass.uv run pytest python/tests/test_dataset.py -k merge_insert: 26 pass.uv run pytest --doctest-modules python/lance/dataset.py -k "explain_plan or analyze_plan": 2 pass.cargo fmt --all,cargo clippy --all --tests --benches -- -D warnings,uv run make lintfrompython/.