-
Notifications
You must be signed in to change notification settings - Fork 166
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
GH-4878 optimise sub-select #4879
Conversation
909870f
to
a8b468f
Compare
a8b468f
to
c1d042b
Compare
Develop branch
This branch
|
@JervenBolleman Any chance you could take a look at this PR? I made a test, but I didn't make a benchmark query yet. The benchmark run I did shows that there at least doesn't seem to be any performance degradation. |
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 think this is a good improvement and worth committing even without a benchmark query. A one of diff test is fine by me.
+ LINE_SEP, | ||
q.getTupleExpr().toString()); | ||
assertThat(q.getTupleExpr().toString()).isEqualToNormalizingNewlines("QueryRoot\n" + | ||
" Projection\n" + |
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.
Should we not have + LINE_SEP instead of \n to be consistent?
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.
isEqualToNormalizingNewlines
fixes the new line seperators for us
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.
But might change it back actually, since it's the only place that tests the line separation aspect of the query plan.
@@ -170,16 +170,16 @@ public void setTotalTimeNanosActual(long totalTimeNanosActual) { | |||
/** | |||
* @return Human readable number. Eg. 12.1M for 1212213.4 and UNKNOWN for -1. | |||
*/ | |||
static String toHumanReadbleNumber(double number) { | |||
static String toHumanReadableNumber(double number) { |
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.
Thanks for catching this typo. Should we extract this into an utility at somepoint?
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.
Could be moved to a more common module, but I don't think I want to do that now.
+ | ||
" │ o: Var (name=d)\n" + | ||
" └── StatementPattern (resultSizeActual=2) [right]\n" + | ||
" │ ╚══ LeftJoin (new scope) (BadlyDesignedLeftJoinIterator) (costEstimate=6.61, resultSizeEstimate=12, resultSizeActual=4) [right]\n" |
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.
This looks like a nice improvement in the query plan.
5a31ca3
to
c0aedf9
Compare
@JervenBolleman I actually found out that the QueryJoinOptimizer is able to optimise sub-selects, but not if there are multiple sub-selects or if there are any BIND clauses anywhere. I've made two benchmarks that will make sure that we don't break that optimisation later. |
GitHub issue resolved: #4878
Briefly describe the changes proposed in this PR:
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resources
to format from the command line)