-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add benchmark for planning sorted unions #14157
Conversation
@@ -289,6 +362,17 @@ fn criterion_benchmark(c: &mut Criterion) { | |||
}); | |||
}); | |||
|
|||
// -- Sorted Queries -- | |||
register_union_order_table(&ctx, 100, 1000); |
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.
The table has 100 columns
/// UNION ALL | ||
/// select null as c1, c2, ... null as cn from t ORDER BY c2 | ||
/// ... | ||
/// select null as c1, null as c2, ... cn from t ORDER BY cn |
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 inner ORDER BY
if the query got the outer one? 🤔 Shouldn't be inner sorting ignored?
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 probably get it, the problem is with the planning of such query not the execution
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 probably get it, the problem is with the planning of such query not the execution
Yes, exactly
do we really need inner
ORDER BY
if the query got the outer one? 🤔 Shouldn't be inner sorting ignored?
Yes, indeed. I think the way it is ignored is that the sort equivalence code determines that the inner sorts aren't needed (or in this case they are all equivalent, so the top order by can a merge rather than sort)
The sort equivalence code (OrderEquivalenceProperties
in particular) is what is consuming all this time
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 thanks @alamb
Which issue does this PR close?
UNION
andORDER BY
queries #13748Rationale for this change
#13748 is a planning speed problem, so as I increase the performance I want a benchmark that shows the improvements
What changes are included in this PR?
Add bechmark to the
sql_planner
suiteAre these changes tested?
Clippy / compilation is tested via CI
Are there any user-facing changes?
A new benchmark
Are there any user-facing changes?
Check out the flamegraph:
Whole flamegraph:
![flamegraph](https://private-user-images.githubusercontent.com/490673/404000954-312d9742-f460-495d-bfc5-65cfb9e5821c.svg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg5MzE5MDUsIm5iZiI6MTczODkzMTYwNSwicGF0aCI6Ii80OTA2NzMvNDA0MDAwOTU0LTMxMmQ5NzQyLWY0NjAtNDk1ZC1iZmM1LTY1Y2ZiOWU1ODIxYy5zdmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIwN1QxMjMzMjVaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1lYWI4ZmE5MmUzZmU3ZDVkOGM5MWQ3YzQxNzc2ZTM4MzZiOGQ0MjBhNzljNGQxOGNjYmY0MTNlYjM2YjcxY2Y1JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.FtB0lzwurkG1VT2B0_ffAkPpezsyr6IpT4GW0HCBFvI)