Fix predicates optimization for distributed queries with HAVING#10621
Merged
alexey-milovidov merged 6 commits intoClickHouse:masterfrom May 31, 2020
Merged
Conversation
azat
force-pushed
the
enable_optimize_predicate_expression-order
branch
2 times, most recently
from
feaab52 to
df9fecd
Compare
Member
Author
e8104d14aa |
zhang2014
reviewed
May 2, 2020
src/Interpreters/Aggregator.cpp
Outdated
zhang2014
reviewed
May 2, 2020
Contributor
There was a problem hiding this comment.
cc: @alexey-milovidov
I think this is an unnecessary change, and the change predicates order should be reasonable.
We keep it so that we can continue to find same bugs.
Member
Author
There was a problem hiding this comment.
I don't have any strong arguments, but executing different queries on the initiator and remote shards (even if only the order differs) sounds tricky
But catching some possible issues maybe a good argument to ignore trickery
azat
changed the title
alexey-milovidov
added
the
pr-bugfix
Member
Author
|
Check failures looks like due to problems on CI |
Member
Author
|
Friendly ping |
azat
force-pushed
the
enable_optimize_predicate_expression-order
branch
from
e8104d1 to
a4957e7
Compare
azat
changed the title
Member
Author
|
Test report summary:
|
…optimize_predicate_expression) Before this patch enable_optimize_predicate_expression changes the order *every* time, so: foo AND bar -> bar AND foo And this causes troubles for distributed queries, since query on shard will have different order of expressions with the initiating server.
Relax this to allow accepting aggregate keys in different order (typical use case is distributed queries, where the initiator server will do final merge).
azat
force-pushed
the
enable_optimize_predicate_expression-order
branch
from
a4957e7 to
6a9ec13
Compare
Member
Author
|
Looks like some tests stuck?
Everything else is green (including performance) |
Member
Author
|
@alexey-milovidov can you take a look? |
alexey-milovidov
approved these changes
May 31, 2020
Member
|
Ok. |
Member
|
This pr also fixes #11413 |
KochetovNicolai
pushed a commit
that referenced
this pull request
Jun 3, 2020
…ion-order Fix predicates optimization for distributed queries with HAVING (cherry picked from commit a8f5b10)
KochetovNicolai
pushed a commit
that referenced
this pull request
Jun 3, 2020
…ion-order Fix predicates optimization for distributed queries with HAVING (cherry picked from commit a8f5b10)
KochetovNicolai
pushed a commit
that referenced
this pull request
Jun 3, 2020
…ion-order Fix predicates optimization for distributed queries with HAVING (cherry picked from commit a8f5b10)
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix predicates optimization for distributed queries (
enable_optimize_predicate_expression=1) for queries withHAVINGsection (i.e. when filtering on the server initiator is required), by preserving the order of expressions (and this is enough to fix), and also force aggregator use column names over indexes. Fixes: #10613, #11413Details:
The problem is that server initiator need to do final aggregate, but Aggregator works based on column indexes, and since expression optimizer switch it every time, server initiator will have different order of columns with the results from the shards that it needs to aggregate, I'm looking into make Aggregator respect column names (if that makes sense?), but will take some time
Cc: @zhang2014