-
Notifications
You must be signed in to change notification settings - Fork 308
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
perf: fine–tuned plan steps #4258
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
WalkthroughThe recent changes encompass updating the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DistExtensionPlanner
participant SessionState
participant LogicalPlan
participant PhysicalPlan
Client->>DistExtensionPlanner: Call optimize_input_logical_plan
DistExtensionPlanner->>SessionState: Use session_state
DistExtensionPlanner->>LogicalPlan: Optimize input logical plan
LogicalPlan-->>DistExtensionPlanner: Return optimized plan
Client->>DistExtensionPlanner: Call create_physical_plan
DistExtensionPlanner->>OptimizedPlan: Use optimized plan
OptimizedPlan-->>PhysicalPlan: Create physical plan
PhysicalPlan-->>DistExtensionPlanner: Return physical plan
DistExtensionPlanner-->>Client: Return result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
Cargo.toml (1)
107-115
: Issues found with the revision hashes inCargo.toml
The specified revision hash
d7bda5c9b762426e81f144296deadc87e5f4a0b8
for thedatafusion
dependencies does not match any commit in thewaynexia/arrow-datafusion
repository. Please verify and update the revision hashes to ensure they are correct.
- Files/Lines impacted:
Cargo.toml
, lines 107-115Analysis chain
LGTM! Updated repository URLs for
datafusion
dependencies.The changes to update the repository URLs of several
datafusion
dependencies from the Apache repository to thewaynexia/arrow-datafusion
repository are approved.However, ensure that the updated revision hashes are correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the revision hashes of the updated `datafusion` dependencies. # Test: Fetch the latest commit hashes from the `waynexia/arrow-datafusion` repository. # Expect: The fetched hashes should match the updated revision hashes in `Cargo.toml`. repos=( "datafusion" "datafusion-common" "datafusion-expr" "datafusion-functions" "datafusion-optimizer" "datafusion-physical-expr" "datafusion-physical-plan" "datafusion-sql" "datafusion-substrait" ) for repo in "${repos[@]}"; do git ls-remote https://github.com/waynexia/arrow-datafusion.git HEAD doneLength of output: 1496
Script:
#!/bin/bash # Description: Verify the specific revision hashes of the updated `datafusion` dependencies in Cargo.toml. dependencies=( "datafusion" "datafusion-common" "datafusion-expr" "datafusion-functions" "datafusion-optimizer" "datafusion-physical-expr" "datafusion-physical-plan" "datafusion-sql" "datafusion-substrait" ) # Expected revision hash from Cargo.toml expected_hash="d7bda5c9b762426e81f144296deadc87e5f4a0b8" # Function to get the commit hash for a specific revision get_commit_hash() { local repo_url=$1 local revision=$2 git ls-remote $repo_url $revision | awk '{print $1}' } # Check each dependency for dep in "${dependencies[@]}"; do repo_url="https://github.com/waynexia/arrow-datafusion.git" commit_hash=$(get_commit_hash $repo_url $expected_hash) echo "$dep: $commit_hash" doneLength of output: 4966
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (3)
- Cargo.toml (1 hunks)
- src/query/src/datafusion.rs (2 hunks)
- src/query/src/dist_plan/planner.rs (4 hunks)
Additional comments not posted (5)
src/query/src/dist_plan/planner.rs (2)
140-147
: LGTM! Direct optimization of the input logical plan.The changes to optimize the input logical plan directly without using an explicit
Analyzer
are approved.
Line range hint
377-411
: LGTM! Improved handling ofExplain
plans andMergeScan
optimization.The changes to take the optimized plan by reference and the improved handling of
Explain
plans andMergeScan
optimization are approved.src/query/src/datafusion.rs (3)
51-51
: LGTM! Added import forMergeScanLogicalPlan
.The addition of the import for
MergeScanLogicalPlan
is approved.
377-411
: LGTM! Improved handling ofExplain
plans andMergeScan
optimization.The changes to handle
Explain
plans and optimize logical plans forMergeScan
are approved.
395-407
: LGTM! Skipped optimization forMergeScan
plans.The changes to skip optimization for
MergeScan
plans are approved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4258 +/- ##
==========================================
- Coverage 85.18% 84.91% -0.27%
==========================================
Files 1060 1060
Lines 189069 189092 +23
==========================================
- Hits 161057 160571 -486
- Misses 28012 28521 +509 |
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 guess I know what it does.
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.
LGTM
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
!!! DO NOT LEAVE THIS BLOCK EMPTY !!!
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Checklist
Summary by CodeRabbit
Updates
Enhancements