Implicit key type conversion for join using step in pipeline by vdimir · Pull Request #19885 · ClickHouse/ClickHouse

vdimir · 2021-01-31T19:28:35Z

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

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Supports implicit key type conversion for JOIN. Closes #18567.

Ref PR #18672

akuzm · 2021-02-05T15:56:03Z

src/Interpreters/join_common.cpp

+{
+    assert(left_keys.size() == right_keys.size());
+
+    /// only JOIN USING supported


The fact that only USING is supported is clear enough from the code, the comment should probably tell "why".

I've added comment to TreeRewriter, where we start to deal with join column conversion and to InterpreterSelectQuery where step added to query plan.
https://github.com/ClickHouse/ClickHouse/pull/19885/files#diff-03665017349505c5529e5d0eeeeae80081e064267d6035b94cd428baf11856ceR417
https://github.com/ClickHouse/ClickHouse/pull/19885/files#diff-5452971ae18972aa669265008a29b3732a99d9415f5b3af10d573c0807667a22R978

src/Interpreters/join_common.cpp

akuzm · 2021-02-05T16:00:41Z

src/Interpreters/TableJoin.cpp

        if (leftBecomeNullable(col.type))
            col.type = makeNullable(col.type);

+    std::unordered_map<String, DataTypePtr> type_map;


So the type information in the input block here is "wrong" (before conversion), and we have to overwrite it w/ the converted values? But why? Can the input block be made to already have the proper types?

I guess, when actions for converting are in a separate step, this code won't be needed ...

I moved common type calculation into TreeRewriter and now TableJoin contains information about converted types on earlier stage, so it is possible to change types in addJoinedColumn and have correct types in columns_added_by_join

akuzm

It's nice that this patch is much smaller than the previous version.

I think it would be helpful to add a high-level comment somewhere, that describes what we do -- e.g. we can convert USING columns to their common supertype by inserting additional casts into ExpressionTransform before join. This only works for USING because etc etc...

src/Interpreters/ExpressionAnalyzer.cpp

KochetovNicolai

.

vdimir · 2021-02-09T14:20:03Z

I've found bugs with aggregation due to fact that in analyzeAggregation corrected types are not calculated yet. Common type calculated moved to earlier stage to TreeRewriter and type mapping stored in TableJoin.

It fixes bug and also I suppose makes code a bit clearer

vdimir · 2021-02-09T14:24:55Z

src/Interpreters/SubqueryForSet.cpp

+    else
+    {
+        auto new_dag = ActionsDAG::merge(
+            std::move(*joined_block_actions->getActionsDAG().clone()),


Should we split joined_block_actions into separate variables not to use merge here? Now it consist from createJoinedBlockActions and additional converting step. @KochetovNicolai

As I understand, this actions are applied to right part of join. And we don't use separate step for this actions for now. So, I think it is ok to merge.

Split before_join and converting_join_columns dags Add more detailed comments for column type conversion for join using

vdimir · 2021-02-19T11:34:39Z

I've added implementation of type conversion for JOIN ON syntax. It don't change types in result table. For that conversion do not replace original columns, but and new ones and join performed by this new columns.

KochetovNicolai

I did not read tests, but changes in code LGTM.

akuzm · 2021-03-01T16:27:16Z

Integration test failure doesn't look relevant.
MSan fuzzer looks like this one #20046

Should we merge?

vdimir · 2021-03-01T18:01:02Z

Should we merge?

I found one issue, query doesn't work if syntax JOIN ON used with some constant in WHERE, e.g. SELECT * FROM t1 FULL JOIN t2 ON t1.a = t2.a WHERE 1. It shows error DB::Exception: Not found column t2.a in block. There are only columns: CAST(t2.a, Int32). If type conversion for t1.a and t2.a not needed it works ok as earlier.

Likely after some optimization some required columns are dropped. I'm trying to figure it out.
Maybe we can merge this and fix this issue later or it's better to investigate it more detailed?

akuzm · 2021-03-02T12:22:27Z

It shows error DB::Exception: Not found column t2.a in block. There are only columns: CAST(t2.a, Int32).

Looks kinda bad, probably better to investigate. Do I remember correctly that you keep the name of the column, but replace it with the casted one? So it should have been named just t2.a and not cast... if everything worked correctly.

…bleJoin" This reverts commit e6406c3.

…uery

Fix error with wrong right column type in aggregate with nulls Add logging inferred type for join keys Add test for join_use_nulls

olgarev · 2021-05-31T02:33:25Z

Internal documentation ticket DOCSUP-9868

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Jan 31, 2021

vdimir force-pushed the vdimir/join-cast-types-v2 branch from 35ac215 to 8d7a135 Compare February 3, 2021 11:40

robot-clickhouse added the submodule changed At least one submodule changed in this PR. label Feb 3, 2021

vdimir force-pushed the vdimir/join-cast-types-v2 branch from 8d7a135 to 4ec9636 Compare February 3, 2021 11:47

vdimir removed the submodule changed At least one submodule changed in this PR. label Feb 3, 2021

vdimir requested review from KochetovNicolai and akuzm February 5, 2021 15:38

akuzm reviewed Feb 5, 2021

View reviewed changes

src/Interpreters/join_common.cpp Outdated Show resolved Hide resolved

akuzm reviewed Feb 5, 2021

View reviewed changes

akuzm approved these changes Feb 5, 2021

View reviewed changes

KochetovNicolai reviewed Feb 8, 2021

View reviewed changes

src/Interpreters/ExpressionAnalyzer.cpp Outdated Show resolved Hide resolved

KochetovNicolai reviewed Feb 8, 2021

View reviewed changes

src/Interpreters/ExpressionAnalyzer.cpp Outdated Show resolved Hide resolved

KochetovNicolai reviewed Feb 8, 2021

View reviewed changes

src/Interpreters/ExpressionAnalyzer.cpp Outdated Show resolved Hide resolved

KochetovNicolai requested changes Feb 8, 2021

View reviewed changes

vdimir force-pushed the vdimir/join-cast-types-v2 branch from 4b68c68 to a6ea5ff Compare February 9, 2021 13:55

vdimir commented Feb 9, 2021

View reviewed changes

vdimir added 11 commits February 19, 2021 14:24

Add converting step for 'join using'

4c36cd1

Fix converting right key type in join using

f5b9801

Fix TableJoin, upd 01675_join_implicit_cast

d15c1a2

Calculate common type for join using in TreeRewriter

435f63f

Split before_join and converting_join_columns dags Add more detailed comments for column type conversion for join using

Give up on name mismatch in inferJoinKeyCommonType

4203dd5

Fix style in appendJoin

9b79ab2

Add tests to 01720_join_implicit_cast - switch, use_nulls

cd7d958

Perform implicit type conversion for JOIN ON keys

a378bd0

Remove addRequiredLeftColumn, some tests for join on different types

3a7eddc

Fix converting join on keys, move actions into TableJoin

456414b

Add comments to TableJoin::inferJoinKeyCommonType

1e37d7c

vdimir added 3 commits February 19, 2021 14:28

More tests for join on key type convert

ab0719c

Remove 'error' from comment from 01710_join_use_nulls

dc9e660

Split test join_implicit_cast (due to timeouts in flaky checks)

1a26310

vdimir force-pushed the vdimir/join-cast-types-v2 branch from 0aa2434 to 1a26310 Compare February 19, 2021 11:30

Add tests to join_implicit_cast checks column name clash

7b8b77f

vdimir requested a review from KochetovNicolai February 19, 2021 11:59

vdimir mentioned this pull request Feb 19, 2021

Cross join to inner rewrite #20392

Merged

KochetovNicolai self-assigned this Feb 19, 2021

KochetovNicolai approved these changes Feb 25, 2021

View reviewed changes

vdimir added 2 commits February 25, 2021 16:24

Merge remote-tracking branch 'upstream/master' into join-cast-types-v2

4c6cde4

Merge branch 'master' into join-cast-types-v2

b7c7c97

vdimir added 7 commits March 5, 2021 16:38

Refactor join: make IJoin implementations independent from TableJoin

e6406c3

Revert "Refactor join: make IJoin implementations independent from Ta…

b0f6f30

…bleJoin" This reverts commit e6406c3.

Clean converting-related fields in TableJoin::resetCollected

5c18bbe

Clean subqueries_for_sets before second analyze in InterpreterSelectQ…

4a8708f

…uery

Add test for cast join on keys with second analyze

3da3794

Add logging for need_analyze_again in InterpreterSelectQuery

354757d

Fixes for join key inferring

cc770ad

Fix error with wrong right column type in aggregate with nulls Add logging inferred type for join keys Add test for join_use_nulls

vdimir merged commit 6ec19b2 into master Mar 9, 2021

vdimir deleted the vdimir/join-cast-types-v2 branch March 9, 2021 06:30

vdimir mentioned this pull request Mar 11, 2021

CREATE TABLE throws 'Type mismatch' exception, but still creates the table. #17649

Closed

azat mentioned this pull request Mar 11, 2021

Add type conversion for StorageJoin (previously led to SIGSEGV) #21646

Merged

vdimir mentioned this pull request Jun 23, 2021

Combining ARRAY JOIN with JOIN #25612

Closed

olgarev mentioned this pull request Jul 15, 2021

DOCSUP-9868: Implicit key type conversion for JOIN #26335

Merged

Conversation

vdimir commented Jan 31, 2021 • edited by KochetovNicolai Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akuzm Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akuzm Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdimir Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KochetovNicolai left a comment

Choose a reason for hiding this comment

Uh oh!

vdimir commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdimir Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir commented Feb 19, 2021

Uh oh!

KochetovNicolai left a comment

Choose a reason for hiding this comment

Uh oh!

akuzm commented Mar 1, 2021

Uh oh!

vdimir commented Mar 1, 2021

Uh oh!

akuzm commented Mar 2, 2021

Uh oh!

olgarev commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vdimir commented Jan 31, 2021 •

edited by KochetovNicolai

Loading

KochetovNicolai Feb 8, 2021 •

edited

Loading

vdimir commented Feb 9, 2021 •

edited

Loading

olgarev commented May 31, 2021 •

edited

Loading