-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Optimize Operator Input Handling #16689
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
deb28f1
to
a082d9b
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
9948c66
to
7c0ad91
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
6350b0e
to
573b3e7
Compare
newRHS, changed := bottomUp(root.RHS, rootID, resolveID, rewriter, shouldVisit, false) | ||
if DebugOperatorTree && changed.Changed() { | ||
fmt.Println(ToTree(newRHS)) | ||
} | ||
anythingChanged = anythingChanged.Merge(changed) | ||
root.RHS = newRHS |
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.
nit picking but how about extracting this, so it can be reused for the LHS and RHS
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 tried, but haven't found a nice way to do it without creating extra intermediate structs, and that's exactly what we are trying to avoid here.
Do you have an example of what you were thinking?
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.
Been through the PR once, and it looks good to me.
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Summary
This PR introduces several optimizations to improve query planning and execution performance:
Operator
interface to useunaryOperator
andbinaryOperator
types in most cases, replacing the genericInputs()
andSetInputs()
methods.evalengine
environment to reduce allocations.These changes aim to reduce memory allocations, improve performance, and optimize the query planning process.
Key Changes
unaryOperator
andbinaryOperator
types and refactored most operators to use these.evalengine
environment across multiple operations.Performance Impact
The changes resulted in significant performance improvements across various planner benchmarks:
Raw performance figures:
Related issue:
#16789
Checklist