Implicit key type conversion for join by vdimir · Pull Request #18672 · ClickHouse/ClickHouse

vdimir · 2020-12-31T15:14:00Z

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):

Join supports implicit key type conversion

Close #18567

vdimir · 2021-01-11T17:30:57Z

Current test is pretty simple, so I'm going to add more tests.
Current implementation doesn't support casting non-nullable types (due to using castColumnAccurateOrNull).
Also RIGHT and FULL join with USING do not work now, it requires type of key column to be different from type of columnd from left table.

tests/queries/0_stateless/01622_join_implicit_cast.sql

akuzm · 2021-01-12T14:14:58Z

src/Interpreters/MergeJoin.cpp

+            left_sample_block, table_join->keyNamesLeft(), right_sample_block, key_names_right);
+
+        /// Casted columns will be stored in separate columns with unique generated names
+        String uuid_string = UUIDHelpers::generateV4().toUnderType().toHexString();


UUIDs in column names can be confusing. Maybe we can use names like cast(UInt8, left_table.key_column)? If you're using the cast function, you should get these names in a natural way.

src/Interpreters/join_common.cpp

akuzm · 2021-01-12T14:29:07Z

By the way, why do we need any casts at all? The function equals works without them, and we can perform an equi-join whenever equals works. Probably because we don't use equals but use a hash-table lookup, so we need binary-identical columns on both sides. OK, I get it :)

It would be helpful to have a big comment somewhere describing the general logic of the process. E.g. the above point, the fact that we try to match the behavior of equals, the fact that we cast right-side to match left-side, etc.

akuzm · 2021-01-12T14:41:22Z

src/Interpreters/ExpressionAnalyzer.cpp

-            query_analyzer.appendJoinLeftKeys(chain, only_types || !first_stage);
+            Block left_block_sample = source_header;
+
+            query_analyzer.appendJoinLeftKeys(chain, only_types || !first_stage, left_block_sample);


Did you consider adding the necessary casts to the right table here, using the ExpressionTransform that happens with it before joins? And then using this casted column as the right-side key. Probably you'll need much less special logic in other places then...

I thought about this but I not sure that it doesn't break types in result table. We need to restore types in joined table. It's clearer to me how to perform conversion inside join implementation without affecting pipeline, but I realize that transform may be better...

src/Interpreters/HashJoin.cpp

… column in join with using

akuzm · 2021-01-20T22:47:18Z

src/Interpreters/join_common.cpp

+    const auto & dst = result_sample_block.getByPosition(result_pos);
+
+    #ifndef NDEBUG
+        if (!JoinCommon::typesEqualUpToNullability(src.type, dst.type))


assert is also OK for debug-only checks.

akuzm · 2021-01-20T22:59:21Z

src/Interpreters/join_common.cpp

 }

+/// Join keys not casted in some cases:
+/// - for tables with `engine = Join` (in this case left_block hasn't rows)


You can use a Join table as a righthand side of join, shouldn't we have normal left blocks in this case?

But I think our variant of type conversion doesn't apply to Join tables anyway -- the hash table is already built, but we convert the righthand values before building the hash table.

akuzm · 2021-01-20T23:07:52Z

src/Interpreters/join_common.cpp

    return materialized;
 }

+/// Join keys not casted in some cases:


Suggested change

/// Join keys not casted in some cases:

/// We allow joins not only when the key type is the same on both sides, but

/// also when the right side can be implicitly CAST'ed to the left type. To

/// perform the join with implicit cast, we cast the right-side column to

/// `Nullable(left type)`, and use this column as a join key. We substitute

/// NULLs for non-convertible values. We then perform the join normally

/// using this modified key column, which gets us the expected result for inner

/// and outer joins as well. We cast the right side because it is expected

/// to have less rows.

/// Join keys are not casted in some cases:

We need to have this high-level description of the algorithm somewhere. Not sure if I wrote it correctly, or where is the best place to put it.

akuzm · 2021-01-20T23:19:11Z

src/Interpreters/MergeJoin.cpp

+    bool cast_allowed = JoinCommon::isCastJoinKeysAllowed(left_sample_block, right_sample_block, *table_join);
+    if (cast_allowed)
+    {
+        /// Left key columns will be converted to types from right if it isn't match


Are we casting left columns for merge join, but right columns for hash join? Maybe we can always cast right, for consistency?

It would be interesting to add a test to see how the runtime switching from hash join to merge join works with casting.

akuzm · 2021-01-20T23:43:24Z

I am somewhat concerned by the amount of changes. I imagined it could be done by adding a new virtual column CAST(right-key, Nullable(left-type)) and substituting it for the right join key, and then performing the join normally. And this would be done somewhere at the level of ExpressionAnalyzer, and wouldn't require any changes to the join executor code at all. E.g. an additional pass somewhere around SelectQueryExpressionAnalyzer::makeTableJoin, that adds a converted column using createJoinedBlockAction, and substitutes the right key name with it.

Does this make sense?

vdimir · 2021-01-21T14:04:03Z

Does this make sense?

Got it, thanks.
Ok, I leave this PR as is and will try to do in such way. We will compare implementations, or if I'll face with some problems of such approach and we will discuss it deeper.

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

Internal documentation ticket DOCSUP-9868

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

vdimir changed the title ~~[wip] Join imlicitly convers key types~~ [wip] Join imlicitly converts key types Dec 31, 2020

vdimir changed the title ~~[wip] Join imlicitly converts key types~~ [wip] Implicit key type conversion for join Dec 31, 2020

alexey-milovidov mentioned this pull request Jan 11, 2021

Roadmap 2021 (discussion) #17623

Closed

vdimir force-pushed the join-cast-types branch from d6b2e0d to 85664fd Compare January 11, 2021 17:25

vdimir marked this pull request as ready for review January 11, 2021 17:31

vdimir changed the title ~~[wip] Implicit key type conversion for join~~ Implicit key type conversion for join Jan 11, 2021

vdimir force-pushed the join-cast-types branch from 85664fd to edbd7e2 Compare January 12, 2021 13:43

Join implicitly convert key types

b705f5b

vdimir force-pushed the join-cast-types branch from edbd7e2 to b705f5b Compare January 12, 2021 13:54

akuzm reviewed Jan 12, 2021

View reviewed changes

tests/queries/0_stateless/01622_join_implicit_cast.sql Outdated Show resolved Hide resolved

akuzm reviewed Jan 12, 2021

View reviewed changes

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

akuzm reviewed Jan 12, 2021

View reviewed changes

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

akuzm reviewed Jan 12, 2021

View reviewed changes

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

akuzm self-assigned this Jan 12, 2021

vdimir force-pushed the join-cast-types branch from b055ca4 to 3abca44 Compare January 14, 2021 15:47

vdimir added 2 commits January 14, 2021 22:30

Cast right table to left in HashJoin

306d21d

Implicit column cast in join works with 'RIGHT JOIN ... USING'

923ee66

vdimir force-pushed the join-cast-types branch from 3abca44 to 923ee66 Compare January 15, 2021 11:32

Fix auto-convert join results in pipeline when type changed

6f9195d

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

vdimir force-pushed the join-cast-types branch from 47c20c0 to 455c91d Compare January 18, 2021 16:44

vdimir removed the submodule changed At least one submodule changed in this PR. label Jan 18, 2021

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

73f1d24

vdimir force-pushed the join-cast-types branch from 455c91d to 73f1d24 Compare January 19, 2021 09:16

vdimir added 3 commits January 19, 2021 18:20

Do not perform type conversion for StorageJoin

fd22ee5

Canonize 01049_join_low_card_bug according to inference type of right…

b4acfc8

… column in join with using

Cosmetic changes in join implicit cast, add comments, test queries

5db2c4d

akuzm reviewed Jan 20, 2021

View reviewed changes

vdimir mentioned this pull request Jan 31, 2021

Implicit key type conversion for join using step in pipeline #19885

Merged

vdimir closed this Mar 9, 2021

-/// Join keys not casted in some cases:
+/// We allow joins not only when the key type is the same on both sides, but
+/// also when the right side can be implicitly CAST'ed to the left type. To
+/// perform the join with implicit cast, we cast the right-side column to
+/// `Nullable(left type)`, and use this column as a join key. We substitute
+/// NULLs for non-convertible values. We then perform the join normally
+/// using this modified key column, which gets us the expected result for inner
+/// and outer joins as well. We cast the right side because it is expected
+/// to have less rows.
+/// Join keys are not casted in some cases:

Conversation

vdimir commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdimir commented Jan 11, 2021

Uh oh!

Uh oh!

akuzm Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akuzm commented Jan 12, 2021

Uh oh!

akuzm Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akuzm Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm commented Jan 20, 2021

Uh oh!

vdimir commented Jan 21, 2021

Uh oh!

olgarev commented May 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdimir commented Dec 31, 2020 •

edited

Loading