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

Improve push-down logic for joined statements #4387

Merged
merged 11 commits into from
Mar 2, 2019

Conversation

abyss7
Copy link
Contributor

@abyss7 abyss7 commented Feb 13, 2019

Also implement the debug query "ANALYZE", which prints out the query after syntax analysis. Should be helpful for tests.

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

@abyss7 abyss7 requested a review from 4ertus2 February 13, 2019 20:16
@abyss7 abyss7 force-pushed the CLICKHOUSE-4268 branch 2 times, most recently from f3c84ac to 1344ff6 Compare February 19, 2019 14:29

-- Optimize predicate expression with view
-- TODO: simple view is not replaced with subquery inside syntax analyzer
ANALYZE SELECT * FROM test.test_view WHERE id = 1;
Copy link
Contributor

@zhang2014 zhang2014 Feb 19, 2019

Choose a reason for hiding this comment

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

Maybe the same as #4387 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not the same - at least from the SyntaxAnalyzer point of view.

ANALYZE SELECT id FROM test.test_view WHERE id = 1;
ANALYZE SELECT s.id FROM test.test_view AS s WHERE s.id = 1;

-- TODO: this query shouldn't work, because the name `toUInt64(sum(id))` is undefined for user
Copy link
Contributor

@zhang2014 zhang2014 Feb 22, 2019

Choose a reason for hiding this comment

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

I'm going to fix this. Maybe SELECT * FROM (SELECT toUInt64(b), sum(id) AS b FROM test.test) WHERE `toUInt64(b)` = 3; should work ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the problem is the opposite: it works now, but it shouldn't, because we somehow allow users to exploit internal aliases. This should be fixed by hiding internal aliases from user. Potentially, we can change internal alias naming scheme any time and such queries will become invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

The alias is actually added for support SELECT * FROM (SELECT 1) WHERE `1` = 1 , it works without predicate optimization. original pull request: #3107

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, it's not related to the push-down - it's a standalone issue. I discussed it with @alexey-milovidov already.

@@ -594,8 +596,28 @@ Names qualifyOccupiedNames(NamesAndTypesList & columns, const NameSet & source_c
return originals;
}

void replaceJoinedTable(const ASTTablesInSelectQueryElement* join)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@abyss7 abyss7 changed the title Implement the debug query "ANALYZE" Improve push-down logic for joined statements Feb 28, 2019
@alexey-milovidov alexey-milovidov merged commit ee90388 into ClickHouse:master Mar 2, 2019
@4ertus2 4ertus2 removed their request for review October 23, 2019 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants