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

Fix order by in case of sorting by primary key prefix and non primary key suffix. #7759

Merged
merged 3 commits into from Nov 15, 2019

Conversation

CurtizJ
Copy link
Member

@CurtizJ CurtizJ commented Nov 13, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):

Fix ORDER BY in case of sorting by primary key prefix and non primary key suffix.


UInt64 limit_for_merging = (need_finish_sorting ? 0 : limit);
executeMergeSorted(pipeline, sorting_info->prefix_order_descr, limit_for_merging);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to figure out how this bug works:
With bug: input block streams sorted by A, (1) sort each block by AB, (2) merge streams as if sorted by A, (3) finish sorting the stream by AB as if the input stream is sorted by A (ok) and each block by AB <-- this last assumption is broken by merge.
With fix: input block streams sorted by A, merge streams as if sorted by A, sort each block by AB, finish sorting the stream by AB as if the input stream is sorted by A, and each block by AB -- all assumptions hold.
Is this correct?

As a side note, the names order_descr and sorting_info->prefix_order_desc make it impossible to tell which one is the input order and which is the required output order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right.

We can rename prefix_order_desc to read_order_descr or input_order_descr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like input_order instead of sorting_info and output_order instead of order_descr?

CurtizJ and others added 2 commits November 15, 2019 17:03
Co-Authored-By: Alexander Kuzmenkov <36882414+akuzm@users.noreply.github.com>
@akuzm akuzm merged commit 723e02f into ClickHouse:master Nov 15, 2019
@CurtizJ CurtizJ added pr-bugfix Pull request with bugfix, not backported by default v19.15 labels Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants