Nullable primary key with correct KeyCondition by amosbird · Pull Request #12455 · ClickHouse/ClickHouse

amosbird · 2020-07-13T09:05:26Z

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

Now KeyConditions can correctly skip nullable keys, including isNull and isNotNull. #12433 Cond.

PS. this pr also piggybacks a test for Field comparison. Let's see if CI complains.

-- Update

It seems Null comparison in Field.h can have the same semantic as in SQL.

Detailed description / Documentation draft:

Nothing special. NOTE, after creating a MergeTree table with nullable primary keys, ClickHouse won't be able to rollabck to previous versions. (potential backward incompatibility)

amosbird · 2020-07-13T09:06:37Z

@zlobober btw, your NULL_LAST logic has gone (I don't know why). I've added it back https://github.com/ClickHouse/ClickHouse/pull/12455/files#diff-043ede2be3d33df96a1f9b9b355678ebL89

amosbird · 2020-07-14T03:53:52Z

After futher thoughts, two special Field types are added (-Inf and +Inf) which explicitly helps special comparisons such as NULL_LAST. Field Null should not be used as comparison argument.

zlobober · 2020-07-31T15:44:49Z

@amosbird, Sorry for the late reply. I like idea of introducing special fields '-inf' and '+inf' very much, it would make much easier expressing open/half-open intervals and rays in some situations. Do you have any plans on finishing this PR?

To make it clear: I am not a ClickHouse maintainer, I am developing an internal Yandex software using ClickHouse as computational engine. Your proposed change could actually help in my case :)

amosbird · 2020-07-31T16:02:55Z

@zlobober Hi, this pr is already finished. I've covered almost all cases related to Nullable primary keys. It's currently waiting for review.

azat · 2020-08-01T00:02:10Z

FWIW I've applied this PR into my build and I did not find any problems yet

azat · 2020-09-11T08:13:47Z

src/Core/Field.h

@amosbird can you update your PR after recent changes?

2020-09-03 05:55:34 /ClickHouse/src/Core/Field.h:462:17: error: enumeration values 'Null', 'NegativeInfinity', and 'PositiveInfinity' not handled in switch [-Werror,-Wswitch] 2020-09-03 05:55:34 switch (which) 2020-09-03 05:55:34 ^ 2020-09-03 05:55:34 /ClickHouse/src/Core/Field.h:497:17: error: enumeration values 'Null', 'NegativeInfinity', and 'PositiveInfinity' not handled in switch [-Werror,-Wswitch] 2020-09-03 05:55:34 switch (which) 2020-09-03 05:55:34 ^

azat · 2020-10-13T07:52:04Z

@alexey-milovidov what is the status of this PR? Are there some concerns about it?

alexey-milovidov · 2021-01-16T10:34:55Z

@azat I did not review this PR and no one from my team decided to take it.

alexey-milovidov · 2021-01-16T21:19:24Z

Yandex projects require manual fixing from our side due to changed interfaces...

amosbird · 2021-03-13T13:51:19Z

Weird CI errors...

azat · 2021-04-20T16:34:10Z

after creating a MergeTree table with nullable primary keys, ClickHouse won't be able to rollabck to previous versions. (potential backward incompatibility)

@amosbird BTW right now if you will run newer clickhouse and then rollback to the version w/o this patch you will not get any errors, but simply index analysis will work incorrectly (for Nullable), maybe it worth adding some guard against this?

amosbird · 2021-04-20T16:52:31Z

after creating a MergeTree table with nullable primary keys, ClickHouse won't be able to rollabck to previous versions. (potential backward incompatibility)

@amosbird BTW right now if you will run newer clickhouse and then rollback to the version w/o this patch you will not get any errors, but simply index analysis will work incorrectly (for Nullable), maybe it worth adding some guard against this?

Hmm, what kind of guards do you suggest?

azat · 2021-04-21T18:14:17Z

Hmm, what kind of guards do you suggest?

I guess new setting for MergeTree will be enough

azat · 2021-08-04T17:45:40Z

src/Storages/MergeTree/MergeTreeIndexMinMax.cpp

-
-        if (!type->isNullable())
-        {
-            serialization->serializeBinary(hyperrectangle[i].left, ostr);
-            serialization->serializeBinary(hyperrectangle[i].right, ostr);
-        }
-        else
-        {
-            bool is_null = hyperrectangle[i].left.isNull() || hyperrectangle[i].right.isNull(); // one is enough
-            writeBinary(is_null, ostr);
-            if (!is_null)
-            {
-                serialization->serializeBinary(hyperrectangle[i].left, ostr);
-                serialization->serializeBinary(hyperrectangle[i].right, ostr);
-            }
-        }
+        serialization->serializeBinary(hyperrectangle[i].left, ostr);
+        serialization->serializeBinary(hyperrectangle[i].right, ostr);


This change breaks on disk format (and the relevant change in the MergeTreeIndexGranuleMinMax::deserializeBinary)

Too bad that I checked this patchset only for compatibility after reverting this patch - #12455 (comment) (use case: I've applied it manually, then revert it, and data skipping indexes over Nullable column had been broken)

But this patchset actually breaks compatibility with older versions of clickhouse for Nullable data skipping indexes after simple upgrade:

Here is a simple reproducer:

-- -- run this with 21.6 or similar (i.e. w/o this patch) -- CREATE TABLE data ( `key` Int, `value` Nullable(Int), INDEX value_index value TYPE minmax GRANULARITY 1 ) ENGINE = MergeTree ORDER BY key; INSERT INTO data SELECT number, number FROM numbers(10000); SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1;

-- now upgrade and run the query again SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1;

And it will fail because of on disk format changes:

$ ll --time-style=+ data/*/data/all_1_1_0/skp*.idx -rw-r----- 1 azat azat 36 data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx -rw-r----- 1 azat azat 37 data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx $ md5sum data/*/data/all_1_1_0/skp*.idx a19c95c4a14506c65665a1e30ab404bf data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx e50e2fcfa873b232196623d56ab26105 data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx

Nice that there is no stable release with this patch included yet.
But personally I, don't use stable releases and how I wish this patchset created new on disk format...

Note that you may create data skipping indexes over Nullable column even before this patchset

Would you mind sending a PR restoring old on disk format?

[1] breaks on disk format (and the relevant change in the: [1]: ClickHouse#12455 (comment) Too bad that I checked this patchset only for compatibility after reverting this patch [2] (use case: I've applied it manually, then revert it, and data skipping indexes over Nullable column had been broken) [2]: ClickHouse#12455 (comment) But this patchset actually breaks compatibility with older versions of clickhouse for Nullable data skipping indexes after simple upgrade: Here is a simple reproducer: -- -- run this with 21.6 or similar (i.e. w/o this patch) -- CREATE TABLE data ( `key` Int, `value` Nullable(Int), INDEX value_index value TYPE minmax GRANULARITY 1 ) ENGINE = MergeTree ORDER BY key; INSERT INTO data SELECT number, number FROM numbers(10000); SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1; Now upgrade and run the query again: SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1; And it will fail because of on disk format changes: $ ll --time-style=+ data/*/data/all_1_1_0/skp*.idx -rw-r----- 1 azat azat 36 data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx -rw-r----- 1 azat azat 37 data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx $ md5sum data/*/data/all_1_1_0/skp*.idx a19c95c4a14506c65665a1e30ab404bf data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx e50e2fcfa873b232196623d56ab26105 data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx Note, that there is no stable release with this patch included yet, so no need to backport. Also note that you may create data skipping indexes over Nullable column even before [3]. [3]: ClickHouse#12455 v2: break cases when granulas has Null in values due to backward compatibility

robot-clickhouse added the pr-improvement Pull request with some product improvements label Jul 13, 2020

amosbird force-pushed the npc branch 3 times, most recently from abc2ba5 to cbafe08 Compare July 14, 2020 02:57

amosbird force-pushed the npc branch 2 times, most recently from f081a12 to 345db99 Compare July 14, 2020 13:37

Akazz self-assigned this Jul 14, 2020

amosbird force-pushed the npc branch 4 times, most recently from bfb1212 to b820ace Compare July 21, 2020 02:35

alexey-milovidov added the st-waiting-for-fix label Jul 21, 2020

amosbird force-pushed the npc branch 2 times, most recently from e257ee2 to 0824256 Compare July 28, 2020 15:37

azat mentioned this pull request Aug 2, 2020

Backport #13205 to 20.6: Fix DateTime64 conversion functions with constant argument #13222

Merged

amosbird force-pushed the npc branch from 0824256 to 76e6540 Compare August 3, 2020 06:13

amosbird mentioned this pull request Aug 10, 2020

Better MergeTree readFromParts debug log #13566

Closed

amosbird force-pushed the npc branch 3 times, most recently from d1ac83f to c9e2f2d Compare August 19, 2020 09:30

alexey-milovidov unassigned Akazz Aug 26, 2020

alexey-milovidov removed the st-waiting-for-fix label Sep 3, 2020

alexey-milovidov mentioned this pull request Sep 4, 2020

Allow a nullable column to be used as primary key/sorting key #5319

Closed

azat reviewed Sep 11, 2020

View reviewed changes

amosbird force-pushed the npc branch from 2f07aff to 0673a5a Compare September 13, 2020 04:08

azat mentioned this pull request Nov 8, 2020

Fix optimize_trivial_count_query with partition predicate #16767

Merged

amosbird force-pushed the npc branch from 746ef39 to 1c70660 Compare November 12, 2020 16:06

amosbird force-pushed the npc branch 3 times, most recently from 42414c9 to fabefff Compare January 4, 2021 08:57

filimonov mentioned this pull request Jan 4, 2021

Poor performance of IS NULL / IS NOT NULL queries against table with allow_nullable_key #18728

Closed

amosbird force-pushed the npc branch from a3c2b83 to 7cfdbee Compare March 13, 2021 06:56

amosbird force-pushed the npc branch from 7cfdbee to 776214d Compare May 12, 2021 04:12

amosbird mentioned this pull request Jun 4, 2021

Allow not in be used in partition pruning #24894

Merged

Nullable primary key with correct KeyCondition

f2ed5ef

amosbird force-pushed the npc branch from 776214d to f2ed5ef Compare June 18, 2021 15:05

amosbird added 2 commits June 19, 2021 14:41

Update

972e3a5

Update again

c56df3e

alexey-milovidov self-assigned this Jun 21, 2021

alexey-milovidov merged commit b52411a into ClickHouse:master Jul 18, 2021

azat reviewed Aug 4, 2021

View reviewed changes

azat mentioned this pull request Aug 4, 2021

Fix on-disk format breakage for secondary indices over Nullable column #27197

Merged

azat mentioned this pull request Aug 23, 2021

Add new index data skipping minmax index format for proper Nullable support #27250

Merged

amosbird mentioned this pull request Sep 9, 2021

[Nullable primary key] Clickhouse crashes periodically #28769

Closed

alexey-milovidov mentioned this pull request Dec 15, 2021

Consider removing the "nullable primary key" feature #32780

Closed

amosbird mentioned this pull request Dec 17, 2021

[Nullable primary key] Repeatable Segfault crash with IN list on 21.8.11.4 recently upgraded from v20.3. #32765

Closed

Conversation

amosbird commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amosbird commented Jul 13, 2020

Uh oh!

amosbird commented Jul 14, 2020

Uh oh!

zlobober commented Jul 31, 2020

Uh oh!

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

Uh oh!

Uh oh!

azat commented Aug 1, 2020

Uh oh!

azat Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

amosbird Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

azat commented Oct 13, 2020

Uh oh!

alexey-milovidov commented Jan 16, 2021

Uh oh!

alexey-milovidov commented Jan 16, 2021

Uh oh!

amosbird commented Mar 13, 2021

Uh oh!

azat commented Apr 20, 2021

Uh oh!

amosbird commented Apr 20, 2021

Uh oh!

azat commented Apr 21, 2021

Uh oh!

azat Aug 4, 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!

alexey-milovidov Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

azat Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

amosbird commented Jul 13, 2020 •

edited

Loading

amosbird commented Jul 31, 2020 •

edited

Loading

azat Aug 4, 2021 •

edited

Loading