Skip to content
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

Order by tuple #38873

Merged
merged 9 commits into from
Jul 7, 2022
Merged

Order by tuple #38873

merged 9 commits into from
Jul 7, 2022

Conversation

devcrafter
Copy link
Member

@devcrafter devcrafter commented Jul 5, 2022

Changelog category (leave one):

  • Performance Improvement

Fixes #38772

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

A less efficient execution plan can be generated for query with ORDER BY (a, b) than for ORDER BY a, b

@robot-ch-test-poll robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 5, 2022
@devcrafter devcrafter added bug Confirmed user-visible misbehaviour in official release pr-performance Pull request with some performance improvements and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Jul 5, 2022
@devcrafter devcrafter force-pushed the order_by_with_braces branch from 39e48bb to c7f9945 Compare July 5, 2022 21:41
@robot-clickhouse robot-clickhouse added pr-bugfix Pull request with bugfix, not backported by default and removed pr-performance Pull request with some performance improvements labels Jul 5, 2022
@devcrafter devcrafter force-pushed the order_by_with_braces branch from c7f9945 to 8f51f1f Compare July 5, 2022 21:49
@devcrafter devcrafter removed the bug Confirmed user-visible misbehaviour in official release label Jul 5, 2022
@alexey-milovidov alexey-milovidov added pr-performance Pull request with some performance improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Jul 5, 2022
@yakov-olkhovskiy yakov-olkhovskiy self-assigned this Jul 6, 2022
@devcrafter devcrafter force-pushed the order_by_with_braces branch from 8f51f1f to a0f6f6f Compare July 6, 2022 10:26
@devcrafter
Copy link
Member Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jul 6, 2022

update

✅ Branch has been successfully updated

Copy link
Member

@yakov-olkhovskiy yakov-olkhovskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
consider maybe unnecessary changes

src/Interpreters/InterpreterSelectQuery.cpp Outdated Show resolved Hide resolved
src/Interpreters/InterpreterSelectQuery.cpp Show resolved Hide resolved
@yakov-olkhovskiy
Copy link
Member

BTW this optimization makes WITH FILL works for such case 👍

@devcrafter
Copy link
Member Author

devcrafter commented Jul 6, 2022

BTW this optimization makes WITH FILL works for such case 👍

I didn't think about it. Thanks for mentioning it. I'd like to add a test before merge it

+ check that EXPLAIN SYNTAX return the same result for ordinary ORDER BY and ORDER BY tuple
+ performance
@devcrafter
Copy link
Member Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jul 6, 2022

update

❌ Base branch update has failed

merge conflict between base and head
err-code: EEF76

devcrafter and others added 4 commits July 7, 2022 00:36
- complexity comes from dreaming about EXPLAIN queries as an ordinary
  ones returning scalar result. So they can be used in SQL tests
  instead of shell ones
@devcrafter devcrafter changed the title Order by with braces Order by tuple Jul 7, 2022
# check that both queries have the same AST after rewrite, EXPLAIN SYNTAX returns it in form of query
##################
QUERY_ORDER_BY="SELECT number AS a, number % 2 AS b FROM numbers(10) ORDER BY a DESC NULLS FIRST WITH FILL FROM 2 TO 1 STEP -1, b DESC NULLS FIRST WITH FILL FROM 2 TO 1 STEP -1"
QUERY_ORDER_BY_TUPLE="SELECT number AS a, number % 2 AS b FROM numbers(10) ORDER BY (a, b) DESC NULLS FIRST WITH FILL FROM 2 TO 1 STEP -1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devcrafter I'm not sure if we can do such an optimization. Is the query without optimization and with this WITH FILL valid?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-performance Pull request with some performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ORDER BY with columns in braces
6 participants