Skip to content

Cross join to inner rewrite#20392

Merged
vdimir merged 9 commits intoClickHouse:masterfrom
vdimir:cross-to-inner-join-rewrite
Mar 10, 2021
Merged

Cross join to inner rewrite#20392
vdimir merged 9 commits intoClickHouse:masterfrom
vdimir:cross-to-inner-join-rewrite

Conversation

@vdimir
Copy link
Copy Markdown
Member

@vdimir vdimir commented Feb 12, 2021 •
edited
Loading

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

Changelog category (leave one):

  • Improvement

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

  • Support more cases to rewrite CROSS JOIN to INNER JOIN.

Close #19856

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Feb 12, 2021
@vdimir vdimir marked this pull request as draft February 12, 2021 14:51
@vdimir vdimir changed the title [WIP] Cross join to inner rewrite Cross join to inner rewrite Feb 15, 2021
@vdimir vdimir marked this pull request as ready for review February 15, 2021 12:02
@vdimir vdimir force-pushed the cross-to-inner-join-rewrite branch from f22319c to ed9d49a Compare February 15, 2021 19:31
@vdimir vdimir force-pushed the cross-to-inner-join-rewrite branch from 8fb8df3 to 1680c8e Compare February 16, 2021 17:18
@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Feb 19, 2021

For now rules fort == operator in JOIN ON more strict than in WHERE section. JOIN ON requires exact type match, but WHERE allows implicit type conversion. So, some queries may do not work after rewrite. But after changes from PR #19885 would be merged we can move expression safely.

@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented Mar 5, 2021

By the way, why do we even have cross/comma join as separate concepts? They are fully equivalent to a clauseless INNER join. There is some difference in operator precedence with comma, but it shouldn't survive past the parsing stage.

Probably the reason is that we don't have a suitable structure for representing the join tree, and use raw AST for this?

akuzm
akuzm reviewed Mar 5, 2021
View reviewed changes
src/Interpreters/CrossToInnerJoinVisitor.cpp
throw Exception("Logical error: function requires arguments", ErrorCodes::LOGICAL_ERROR);
if (node.arguments->children.size() != 2)
return false;
std::optional<size_t> getIdentMembership(const ASTIdentifier & ident, const std::vector<TableWithColumnNamesAndTypes> & tables)
Copy link
Copy Markdown
Contributor

@akuzm akuzm Mar 5, 2021

Choose a reason for hiding this comment

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

Nice, this almost starts to look like the proper join clause infrastructure. Eventually we should have an explicit data structure describing the join tree, and then for each WHERE clause, check which relations participate in it, and put it to the appropriate level in this tree.

Copy link
Copy Markdown
Member Author

@vdimir vdimir Mar 5, 2021

Choose a reason for hiding this comment

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

I also tried to use this code in #21370 to fix problem with Merge engine. While executing JOIN with Merge there are step when we remove right table from query and perform analysis on it (I don't completly figure out why it is needed, maybe it requires other approach). But expression that uses right table left in query and it leads to error. This code can be used to remove them too.

@akuzm akuzm self-assigned this Mar 5, 2021
@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Mar 9, 2021 •
edited
Loading

Probably the reason is that we don't have a suitable structure for representing the join tree, and use raw AST for this?

Yes, I think so. This visitor always rewrites COMMA to CROSS replacing kind

ClickHouse/src/Interpreters/CrossToInnerJoinVisitor.cpp

Lines 322 to 328 in 90d019a

/// COMMA to CROSS
if (num_comma)
{
for (auto & table : joined_tables)
table.rewriteCommaToCross();
}

@UnamedRus UnamedRus mentioned this pull request Mar 9, 2021
Closed
@vdimir vdimir merged commit 33dce12 into ClickHouse:master Mar 10, 2021
@vdimir vdimir deleted the cross-to-inner-join-rewrite branch March 10, 2021 06:53
@vdimir vdimir mentioned this pull request Mar 12, 2021
Merged
@alex-zaitsev alex-zaitsev mentioned this pull request Oct 5, 2021
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@akuzm akuzm akuzm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@akuzm akuzm

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AND (cond OR cond) prevent CROSS JOIN to INNER JOIN rewrite

3 participants

@vdimir @akuzm @robot-clickhouse