Fix hashing of NULL values#58754

Closed

aiven-sal wants to merge 4 commits intoClickHouse:masterfrom

aiven-sal:aiven-sal/fix_hashnull

Contributor

aiven-sal commented Jan 12, 2024 •

edited by rschu1ze

Loading

Closes #48623

This is a re-submit of #48625 with minor changes to make it backwards-compatible (see #48625 (comment))

NULLs are assumed to have a hash value of 42, settling the question of life, the universe and everything.

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

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

It is now possible to hash NULL values in arbitrary Nullable types, e.g., SELECT xxHash32([toNullable(NULL)]);

aiven-sal and others added 2 commits

January 12, 2024 16:37


          Fix hashing on NULL

b5c5322

Rebased and squashed commits from
ClickHouse#48625
with some minor style changes.

Co-authored-by: zhanglistar <[email protected]>


          Revert backwards incompatible changes

d1f07bc

rschu1ze self-assigned this

rschu1ze added the can be tested label

robot-clickhouse-ci-1 added the pr-bugfix label

Contributor

robot-clickhouse-ci-1 commented Jan 12, 2024 •

edited by robot-ch-test-poll1

Loading

This is an automated comment for commit ca25f36 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
Docs check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ 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

Check name	Description	Status
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	❌ error
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	⏳ pending
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	⏳ pending
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	❌ failure
Mergeable Check	Checks if all other necessary checks are successful	❌ failure

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

rschu1ze added 2 commits

January 16, 2024 11:41


          Merge remote-tracking branch 'ClickHouse/master' into aiven-sal/fix_h…

7c9e0a9

…ashnull


          Cosmetics

ca25f36

rschu1ze reviewed

View reviewed changes

src/Functions/FunctionsHashing.h Outdated

                               column->getName(), getName());
                   }
+                  template <bool first>

Member

rschu1ze Jan 16, 2024

Is it possible to add a SQL test which exercised this code?

src/Functions/FunctionsHashing.h Outdated

+                  void executeNothing(const KeyColumnsType & key_cols, const IColumn * column, typename ColumnVector<ToType>::Container & vec_to) const
+                  {
+                      KeyType key{};
+                      if constexpr (Keyed)

Member

rschu1ze Jan 16, 2024

Here and in l. 1188, please make sure to add a test with a keyed hash function.

src/Functions/FunctionsHashing.h

                   }
+                  template <bool first>
+                  void executeNothing(const KeyColumnsType & key_cols, const IColumn * column, typename ColumnVector<ToType>::Container & vec_to) const

Member

rschu1ze Jan 16, 2024

Is there a SQL test which exercises this function?

src/Functions/FunctionsHashing.h

+                  void executeNothing(const KeyColumnsType & key_cols, const IColumn * column, typename ColumnVector<ToType>::Container & vec_to) const
+                  {
+                      KeyType key{};
+                      if constexpr (Keyed)

Member

rschu1ze Jan 16, 2024

For here and in l. 1188: we should have a SQL test with a keyed hash function.

src/Functions/FunctionsHashing.h

+                              vec_to.assign(vec_to.size(), static_cast<ToType>(NULL_HASH));
+                          else
+                              for (size_t i = 0; i < vec_to.size(); ++i)
+                                  vec_to[i] = combineHashes(key, vec_to[i], static_cast<ToType>(NULL_HASH));

Member

rschu1ze Jan 16, 2024

Please pull static_cast<ToType>(NULL_HASH) (here, l. 1211) out as as constant, and rewrite it to the form in l. 1201.

src/Functions/FunctionsHashing.h

+                          {
+                              const auto & nested_col = col_from->getNestedColumn();
+                              typename ColumnVector<ToType>::Container vec_temp(nested_col.size());
+                              bool nested_is_first = true;

Member

rschu1ze Jan 16, 2024

Why don't we use template parameter first (l. 1184) instead of nested_is_first?

EDIT: Ah, I see.

src/Functions/FunctionsHashing.h

+                              for (size_t i = 0; i < vec_to.size(); ++i)
+                              {
+                                  ToType hash{NULL_HASH};
+                                  if (!col_from->isNullAt(i))

Member

rschu1ze Jan 16, 2024

Non-null values are likely more common, suggest to write:

ToType hash = vec_temp[i];
if (col_from->isNullAt(i))
    hash = NULL_HASH;

tests/queries/0_stateless/02677_hash_functions_on_null.sql

		@@ -0,0 +1,15 @@
		select xxHash32(NULL);

Member

rschu1ze Jan 16, 2024

Cosmetic: could we please put SQL keywords in all-lowercase or all UPPERCASE but not a mix of both?

tests/queries/0_stateless/02677_hash_functions_on_null.sql

+              DROP TABLE IF EXISTS test_hash_on_null;
+              CREATE TABLE test_hash_on_null (a Array(Nullable(Int64))) ENGINE = Memory;
+              insert into test_hash_on_null values (NULL) ([NULL, NULL]);

Member

rschu1ze Jan 16, 2024

The column type is Array(Nullable(T)). What is the actually stored value if NULL is inserted? (also wondering, how that is even possible?)

tests/queries/0_stateless/02677_hash_functions_on_null.sql

+              DROP TABLE IF EXISTS test_hash_on_null;
+              CREATE TABLE test_hash_on_null (a Array(Nullable(Int64))) ENGINE = Memory;
+              insert into test_hash_on_null values (NULL) ([NULL, NULL]);

Member

rschu1ze Jan 16, 2024

Can we add a test case which mixes non-NULL and NULL values inside an Array?

rschu1ze mentioned this pull request

Fix hashing on NULL #48625

Closed

rschu1ze changed the title ~~Fix hashing on NULL - re-submit #48625~~ Fix hashing of NULL values

Member

rschu1ze commented Jan 16, 2024

This is a re-submit of #48625 with minor changes to make it backwards-compatible (see #48625 (comment))

@aiven-sal For future reference and for my curiosity, which backward-compatibility issues were addressed exactly and how?

Contributor Author

aiven-sal commented Jan 26, 2024

This is a re-submit of #48625 with minor changes to make it backwards-compatible (see #48625 (comment))

@aiven-sal For future reference and for my curiosity, which backward-compatibility issues were addressed exactly and how?

Currently, hashing NULL returns NULL and hashing an array containing at least 1 NULL throws and exception (while hashing arrays with non-nullable values works for some hashes).
The original PR made hashing working for arrays with nullable values, but it also changed the value returned for NULL. Going back to the original behaviour was as simple as removing useDefaultImplementationForNulls and useDefaultImplementationForNothing

Member

rschu1ze commented Jan 31, 2024

@aiven-sal Would you like to merge from master and have a look at the comments? I'd be happy to merge this one.

Member

alexey-milovidov commented Mar 24, 2024

Looks abandoned.

alexey-milovidov added the close in a month if not active label

Member

Algunenano commented Apr 29, 2024

Please reopen once the comments have been addressed

Algunenano closed this

aiven-sal deleted the aiven-sal/fix_hashnull branch

August 13, 2024 13:17

al13n321 mentioned this pull request

Fix hashing of NULLs, attempt 3 #79008

Merged

1 task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested close in a month if not active pr-bugfix