-
Notifications
You must be signed in to change notification settings - Fork 750
perf: plan take operations through FilteredReadExec's range-read path #7672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
LuQQiu
wants to merge
22
commits into
lance-format:main
Choose a base branch
from
LuQQiu:lu/takeOptimization
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
0ae5213
perf: plan takes through FilteredReadExec's range-read path
LuQQiu 4114b80
bench: use rand 0.9 random_range in take_exec_compare
LuQQiu d94ef90
style: fix clippy findings in take mode
LuQQiu 058eb8b
perf: parallelize fragment opens in FilteredReadExec take mode
LuQQiu 349ee1a
refactor: model the FilteredReadExec input as a row source
LuQQiu 173d816
chore: keep the take benchmark harness out of the tree
LuQQiu bfbb97b
feat(filtered-read): coalesce the row-stream input into read rounds
LuQQiu f3f88cc
feat(filtered-read): accept a _rowaddr key with stable row ids
LuQQiu 0411f5c
perf(filtered-read): tune round I/O ordering and skip no-op alignment
LuQQiu 3911477
refactor(filtered-read): rename the row source to RowSelector, trim docs
LuQQiu e7d6ec6
refactor(filtered-read): name the round stages and dedupe stream-sour…
LuQQiu e893cbc
feat(filtered-read): support with_deleted_rows on the row-stream sele…
LuQQiu 5dfe78c
feat(filtered-read): make the identity flags authoritative on row str…
LuQQiu 5b4919b
refactor(filtered-read): share the scan setup glue with the row-strea…
LuQQiu e982ebd
feat(filtered-read): complete the with_deleted_rows contract on row s…
LuQQiu 529b2ae
revert(filtered-read): reject with_deleted_rows on row-stream reads
LuQQiu 51f8d17
refactor(filtered-read): address review-bot findings and clone cleanup
LuQQiu 4ccf02f
docs(filtered-read): trim review-phase comments to the essentials
LuQQiu c898edd
test: update python plan assertions for the row-stream display
LuQQiu 2e881db
refactor(filtered-read): run each row-stream round through the scan e…
LuQQiu b699b54
refactor(filtered-read): batch vocabulary and comment trims from review
LuQQiu f724d23
fix(scanner): make CoalesceTake collapse work for row-stream takes
LuQQiu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -147,6 +147,19 @@ fn try_rewrite(agg: &AggregateExec) -> DFResult<Option<Arc<dyn ExecutionPlan>>> | |||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| let options = filtered_read.options(); | ||||||||||||||||||||||||||
| // A read with a row-stream source emits one row per input row; its count | ||||||||||||||||||||||||||
| // is driven by the input plan, not by fragment metadata. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // COUNT over such a read == the count of input rows whose id still | ||||||||||||||||||||||||||
| // exists. A future CountLiveRowsExec could answer that without any | ||||||||||||||||||||||||||
| // column I/O: stream the input and, per batch, build the live-row set | ||||||||||||||||||||||||||
| // from fragment metadata + deletion vectors and count member rows | ||||||||||||||||||||||||||
| // (per row, so duplicates count). Deferred because no plan shape today | ||||||||||||||||||||||||||
| // places a row-stream read under a COUNT — count plans request no | ||||||||||||||||||||||||||
| // columns, so the scanner never builds one. | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||
| if filtered_read.row_stream_input().is_some() { | ||||||||||||||||||||||||||
| return Ok(None); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // A refine filter is a residual the index couldn't fully evaluate — it | ||||||||||||||||||||||||||
| // needs column data to apply, which we can't. | ||||||||||||||||||||||||||
| if options.refine_filter.is_some() { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this verbose comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clean up the verbose comments after all the code changes looks good (otherwise agent tend to add back a lot of comments when i delete them up front lollll)