Fix hashing on NULL by zhanglistar · Pull Request #48625 · ClickHouse/ClickHouse

zhanglistar · 2023-04-11T07:21:55Z

Fixes #48623

Changelog category (leave one):

Improvement

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

Fix issue #48623

…nto fixhashnull

tests/queries/0_stateless/02677_hash_functions_on_null.reference

src/Functions/FunctionsHashing.h

robot-clickhouse-ci-2 · 2023-05-16T10:02:40Z

This is an automated comment for commit cc141d4 with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🔴 failure

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	🟢 success
Bugfix validate check	Checks that either a new test (functional or integration) or there some changed tests that fail with the binary built on master branch	🟢 success
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	🟢 success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	🟢 success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	🟢 success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	🟢 success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	🟢 success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	🟢 success
Install packages	Checks that the built packages are installable in a clear environment	🟢 success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	🟢 success
Mergeable Check	Checks if all other necessary checks are successful	🟢 success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	🟢 success
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	🟢 success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	🟢 success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	🟢 success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🟢 success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🔴 failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	🟢 success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	🟢 success
Unit tests	Runs the unit tests for different release types	🟢 success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	🟢 success

…nto fixhashnull

zhanglistar · 2023-05-23T02:55:46Z

@al13n321 @aiven-sal Does this PR will break back compatibility? Besides, the failures seem not related to this PR.

aiven-sal · 2023-05-23T08:09:13Z

src/Functions/FunctionsHashing.h

+            else
+                for (size_t i = 0, size = vec_to.size(); i < size; ++i)
+                    vec_to[i] = combineHashes(key, vec_to[i], h);
+        }


This should simply use i instead of j.

No, j is the index of nested column while i is the index of ColumnNullable that's not the same.

The size of a ColumnNullable and the size of a nested column are the same. You have to skip the nested column's elements that are "null". Hence you should drop the j and just use i.
Just test your code with a table the mixes nullables and NULLs and you'll see that the result is incorrect.
In fact it would be a good idea to add some more tests to this PR.

@aiven-sal Sorry for mistake the meaning of index i, which is vec_to's index, not ColumnNullable. As 'Just test your code with a table the mixes nullables and NULLs and you'll see that the result is incorrect.', do you have some tests? I will add it to my tests.

do you have some tests?

Sure, here is an example of what I meant:

CREATE TABLE test_mix_null (a Nullable(Int64)) ENGINE = Memory; insert into test_mix_null values (NULL) (toNullable(4)) (NULL) (toNullable(4454559)); select xxHash32(a), a from test_mix_null;

The correct result should be

42 \N 4160678787 4 42 \N 443946719 4454559

@aiven-sal Got it. Since there is default data in nested column if corresponding position is NULL, actually there is no need to store. Maybe we should delete the default data in another PR if it worth doing.

Avogar · 2023-05-26T11:35:48Z

This change is backward-incompatible, we should do it under a setting.
Also, I think we should make these two hashes different:

:) select sipHash64(42::Nullable(UInt32)) as h1, sipHash64(42::UInt32) as h2

┌──────────────────h1─┬──────────────────h2─┐
│ 4686640835114562322 │ 4686640835114562322 │
└─────────────────────┴─────────────────────┘

Because columns are actually different (one is Nullable, other is not). In case of not-nullable Nullable(...) value we can combine hash of nested value with some magic number (or just value from nullmap column).

aiven-sal · 2023-05-26T12:28:56Z

This change is backward-incompatible, we should do it under a setting.

+1

Also, I think we should make these two hashes different:
:) select sipHash64(42::Nullable(UInt32)) as h1, sipHash64(42::UInt32) as h2

┌──────────────────h1─┬──────────────────h2─┐
│ 4686640835114562322 │ 4686640835114562322 │
└─────────────────────┴─────────────────────┘
Because columns are actually different (one is Nullable, other is not). In case of not-nullable Nullable(...) value we can combine hash of nested value with some magic number.

Isn't the function supposed to be applied to the values stored in the column and not on "the column" itself?
I mean, if we apply the same logic here:

:) select sipHash64(42::UInt32) as h1, sipHash64(42::Int32) as h2

┌──────────────────h1─┬──────────────────h2─┐
│ 4686640835114562322 │ 4686640835114562322 │
└─────────────────────┴─────────────────────┘

These two hashes should be different because the two columns are different: one column is signed and the other is not.
Did I misunderstand what you meant?

Avogar · 2023-05-26T12:32:03Z

Isn't the function supposed to be applied to the values stored in the column and not on "the column" itself?

Yes, you are right. But in Nullable column we store two columns, nested column and nullmap column. I just think that we should reflect both columns in the hash, not only nested column (but it's just my opinion, It can be done in different ways).

aiven-sal · 2023-05-26T12:49:49Z

Isn't the function supposed to be applied to the values stored in the column and not on "the column" itself?

Yes, you are right. But in Nullable column we store two columns, nested column and nullmap column. I just think that we should reflect both columns in the hash, not only nested column (but it's just my opinion, It can be done in different ways).

Yes, I understand what you mean. It makes sense.
My only concern is that it is not always super-obvious for a user if something will be cast to Nullable or not
e.g.

SELECT
    (
        SELECT number
        FROM system.numbers
        LIMIT 1
    ) AS subq_number,
    toTypeName(subq_number),
    number,
    toTypeName(number)
FROM system.numbers
LIMIT 1

┌─subq_number─┬─toTypeName(subq_number)─┬─number─┬─toTypeName(number)─┐
│           0 │ Nullable(UInt64)        │      0 │ UInt64             │
└─────────────┴─────────────────────────┴────────┴────────────────────┘

As a user, I wouldn't expect the hashes of subq_number and number to be different.

al13n321 · 2023-05-29T19:35:43Z

Also, I think we should make these two hashes different:

That seems less consistent with existing hash behavior in other cases. Currently hashes are usually the same for the "same" value of different data types:

SELECT
    cityHash64(CAST('1', 'Int8')),
    cityHash64(CAST('1', 'UInt64')),
    cityHash64(CAST('1970-01-02', 'Date')),
    cityHash64(CAST('1970-01-01 00:00:01', 'DateTime'))
FORMAT Vertical

Query id: b6819231-6346-4a95-93b5-17da01589d9b

Row 1:
──────
cityHash64(CAST('1', 'Int8')):                       10577349846663553072
cityHash64(CAST('1', 'UInt64')):                     10577349846663553072
cityHash64(CAST('1970-01-02', 'Date')):              10577349846663553072
cityHash64(CAST('1970-01-01 00:00:01', 'DateTime')): 10577349846663553072

I think it would make sense to either:
(a) always make hashes different for different data types, or
(b) make hashes match across types as much as possible.

Currently we do approximately (b) (idk if intentionally or not). (b) seems more flexible because (a) can be emulated with something like cityHash64(x) + cityHash64(toTypeName(x)).

In practice, I've never needed (a) so far (but I'm new and not a user, so idk), and needed (b) once (in parquet tests, because our parquet decoder makes everything nullable and changes some data types to other similar data types).

EDIT: To be more clear: making these two hashes different would make this PR mostly useless for the original use case (parquet tests) that motivated (?) this PR in the first place (through #48365 ).

al13n321 · 2023-05-29T19:41:25Z

This change is backward-incompatible, we should do it under a setting.

Fwiw, a possible compatible version would be to use new behavior only inside tuples or arrays (where previously an exception would be thrown). Then SELECT cityHash64(tuple(*)) would work the new way, while SELECT cityHash64(*) would work the old way.

I'm not sure this is worth the effort, we should probably just switch everything to the new behavior by default.

zhanglistar · 2023-05-31T06:16:59Z

There is no related fails, maybe can merge now?

UnamedRus · 2023-07-17T08:03:49Z

Fwiw, a possible compatible version would be to use new behavior only inside tuples or arrays (where previously an exception would be thrown). Then SELECT cityHash64(tuple()) would work the new way, while SELECT cityHash64() would work the old way.

It will not help, people used tuples in hashes to get value and not NULL when they have null columns

https://fiddle.clickhouse.com/90b31ab2-ef3d-4dbd-a4ab-d6e2964485b5

clickhouse-ci · 2023-10-23T18:16:57Z

This is an automatic comment. The PR descriptions does not match the template.

Please, edit it accordingly.

The error is: Changelog entry required for category 'Bug Fix (user-visible misbehavior in an official stable release)'

clickhouse-ci · 2023-10-23T18:18:04Z

This is an automatic comment. The PR descriptions does not match the template.

Please, edit it accordingly.

The error is: Changelog entry required for category 'Bug Fix (user-visible misbehavior in an official stable release)'

aiven-sal · 2023-10-25T10:46:11Z

Hi

@zhanglistar Are you still interested in working on this PR?

@al13n321 Does this change look good to you in its current shape? How do you want to deal regarding breaking backwards compatibility?

alexey-milovidov · 2023-12-31T12:55:49Z

src/Functions/FunctionsHashing.h

 {
 public:
    static constexpr auto name = Impl::name;
+    static constexpr int null_magic_number = 42;


I don't like that. Not fundamental enough.
Zero is better.

But, it allow to distinguish Zero value and Nulls, which are really common default values for data types.

Kinda make sense to have it different from zero for nulls.

clickhouse-ci · 2023-12-31T12:57:05Z

This is an automatic comment. The PR descriptions does not match the template.

Please, edit it accordingly.

The error is: Changelog entry required for category 'Improvement'

alexey-milovidov · 2023-12-31T13:00:13Z

@aiven-sal, first we have to fix the issue #48623 alone, without introducing backward compatibility. It means we should correctly hash arrays with NULL, tuples with NULL, etc, but not change the hashing of NULL itself.

After that, we can proceed with the second step to changing the behavior for NULL under a setting.

If you are interested, you can pull the commits from this PR into your branch and submit another PR.

Rebased and squashed commits from ClickHouse#48625 with some minor style changes. Co-authored-by: zhanglistar <[email protected]>

rschu1ze · 2024-01-16T16:04:12Z

Moved to #58754

zhanglistar added 4 commits April 11, 2023 15:20

Fix hashing on NULL

b04854f

Restore blank line

2978000

Merge branch 'master' into fixhashnull

47cc966

Merge branch 'fixhashnull' of https://github.com/bigo-sg/ClickHouse i…

eed2730

…nto fixhashnull

zhanglistar mentioned this pull request Apr 11, 2023

AnyHash of arrays or tuples of nullable values #48365

Closed

evillique added the can be tested Allows running workflows for external contributors label Apr 11, 2023

nickitat reviewed Apr 11, 2023

View reviewed changes

tests/queries/0_stateless/02677_hash_functions_on_null.reference Show resolved Hide resolved

nickitat self-assigned this Apr 11, 2023

Hash on NULL value equals ()

17d1a96

robot-ch-test-poll4 added the pr-bugfix Pull request with bugfix, not backported by default label Apr 12, 2023

zhanglistar added 8 commits April 12, 2023 11:49

Merge branch 'master' into fixhashnull

75e6129

Merge branch 'master' into fixhashnull

8a4c01e

Merge branch 'master' into fixhashnull

a41a0e8

Fix tests fail on hash null

ba120a9

Merge branch 'master' into fixhashnull

b3f5db5

No change hashjoin.cpp

ca2f333

Fix tests

d6bcf6c

Merge branch 'master' into fixhashnull

722ed5a

nickitat reviewed Apr 19, 2023

View reviewed changes

src/Functions/FunctionsHashing.h Outdated Show resolved Hide resolved

zhanglistar added 2 commits April 23, 2023 10:33

magic number to common var.

2c9e914

Merge branch 'master' into fixhashnull

2cbe7f3

aiven-sal suggested changes Apr 27, 2023

View reviewed changes

src/Functions/FunctionsHashing.h Outdated Show resolved Hide resolved

src/Functions/FunctionsHashing.h Outdated Show resolved Hide resolved

al13n321 reviewed May 9, 2023

View reviewed changes

src/Functions/FunctionsHashing.h Outdated Show resolved Hide resolved

al13n321 mentioned this pull request May 11, 2023

Partially reimplement Parquet encoder to make it faster and parallelizable #49367

Merged

zhanglistar added 2 commits May 16, 2023 17:59

Fix hash on null

643ae2d

Merge branch 'master' into fixhashnull

289a003

zhanglistar added 3 commits May 16, 2023 18:28

Merge branch 'master' into fixhashnull

04f0913

Merge branch 'master' into fixhashnull

1066cac

fix tests

90c96a2

zhanglistar added 3 commits May 22, 2023 11:07

Merge branch 'master' into fixhashnull

6d0b3be

suport cal on lowcardinality

449e194

Merge branch 'fixhashnull' of https://github.com/bigo-sg/ClickHouse i…

b81922b

…nto fixhashnull

aiven-sal suggested changes May 23, 2023

View reviewed changes

zhanglistar added 2 commits May 29, 2023 11:11

Fix columnullable hashing

42cd949

Merge branch 'master' into fixhashnull

5840f05

Merge branch 'master' into fixhashnull

cc141d4

alexey-milovidov reviewed Dec 31, 2023

View reviewed changes

alexey-milovidov marked this pull request as draft December 31, 2023 13:00

alexey-milovidov assigned alexey-milovidov and unassigned nickitat Dec 31, 2023

aiven-sal added a commit to aiven-sal/ClickHouse that referenced this pull request Jan 12, 2024

Fix hashing on NULL

b5c5322

Rebased and squashed commits from ClickHouse#48625 with some minor style changes. Co-authored-by: zhanglistar <[email protected]>

aiven-sal mentioned this pull request Jan 12, 2024

Fix hashing of NULL values #58754

Closed

rschu1ze closed this Jan 16, 2024

al13n321 mentioned this pull request Apr 11, 2025

Fix hashing of NULLs, attempt 3 #79008

Merged

1 task

Conversation

zhanglistar commented Apr 11, 2023 • edited by alexey-milovidov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robot-clickhouse-ci-2 commented May 16, 2023 • edited by robot-clickhouse-ci-1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhanglistar commented May 23, 2023

Uh oh!

aiven-sal May 23, 2023

Choose a reason for hiding this comment

Uh oh!

zhanglistar May 24, 2023 • 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!

aiven-sal May 24, 2023 • 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!

zhanglistar May 26, 2023

Choose a reason for hiding this comment

Uh oh!

aiven-sal May 26, 2023

Choose a reason for hiding this comment

Uh oh!

zhanglistar May 29, 2023 • 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!

Avogar commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiven-sal commented May 26, 2023

Uh oh!

Avogar commented May 26, 2023

Uh oh!

aiven-sal commented May 26, 2023

Uh oh!

al13n321 commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

al13n321 commented May 29, 2023

Uh oh!

zhanglistar commented May 31, 2023

Uh oh!

UnamedRus commented Jul 17, 2023

Uh oh!

clickhouse-ci bot commented Oct 23, 2023

Uh oh!

clickhouse-ci bot commented Oct 23, 2023

Uh oh!

aiven-sal commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov Dec 31, 2023

Choose a reason for hiding this comment

Uh oh!

UnamedRus Jan 6, 2024

Choose a reason for hiding this comment

Uh oh!

clickhouse-ci bot commented Dec 31, 2023

Uh oh!

alexey-milovidov commented Dec 31, 2023

Uh oh!

rschu1ze commented Jan 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

zhanglistar commented Apr 11, 2023 •

edited by alexey-milovidov

Loading

robot-clickhouse-ci-2 commented May 16, 2023 •

edited by robot-clickhouse-ci-1

Loading

zhanglistar May 24, 2023 •

edited

Loading

aiven-sal May 24, 2023 •

edited

Loading

zhanglistar May 29, 2023 •

edited

Loading

Avogar commented May 26, 2023 •

edited

Loading

al13n321 commented May 29, 2023 •

edited

Loading

aiven-sal commented Oct 25, 2023 •

edited

Loading