fix HashMethodOneNumber with const column #40020

canhld94 · 2022-08-09T08:48:19Z

Changelog category (leave one):

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

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

fix HashMethodOneNumber get wrong key value when column is const

Close #39818

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

Avogar · 2022-08-09T09:52:33Z

Thanks! Can you add a test for it?

Avogar · 2022-08-09T09:56:17Z

Also worth fixing all HashMethod*, not only HashMethodOneNumber, for example such query leads to segfault:

select '1' from numbers(10) intersect select '1' from numbers(10)

If you have time, you can fix it in this PR, if not, I will create a separate PR.

canhld94 · 2022-08-09T09:57:51Z

Thanks! Can you add a test for it?

I will add test. In addition, I found this bug also happens for other method, e.g. HashMethodFixedString,... The following query will crash the server.

select 'aaa' from numbers(10) intersect select 'aaa' from numbers(10);

Seem a critical bug.

canhld94 · 2022-08-09T09:59:49Z

Also worth fixing all HashMethod*, not only HashMethodOneNumber, for example such query leads to segfault:
select '1' from numbers(10) intersect select '1' from numbers(10)
If you have time, you can fix it in this PR, if not, I will create a separate PR.

I will added the fix for other types.

canhld94 · 2022-08-09T10:33:31Z

Also worth fixing all HashMethod*, not only HashMethodOneNumber, for example such query leads to segfault:
select '1' from numbers(10) intersect select '1' from numbers(10)
If you have time, you can fix it in this PR, if not, I will create a separate PR.

@Avogar I added the fix fo HashMethodString and HashMethodFixedString as well as some tests. IMO only these 3 HashMethod* are affected by this bug. If others method also need adding, I think someone else should add it as I'm not quite familiar with their implementation.

canhld94 · 2022-08-10T10:50:35Z

The query no. 6 of test 'distinct_in_order' has slowed down
The query no. 7 of test 'distinct_in_order' has slowed down
The query no. 8 of test 'distinct_in_order' has slowed down
The query no. 0 of test 'distinct_in_order' has slowed down
The query no. 3 of test 'distinct_in_order' has slowed down
The query no. 4 of test 'distinct_in_order' has slowed down
The query no. 0 of test 'set' has slowed down
The query no. 1 of test 'set' has slowed down

Looks like checking const every time getting key is not a good idea. I will rewrite the implementation.

Avogar · 2022-08-10T13:10:38Z

src/Common/ColumnsHashing.h

+        if (isColumnConst(*column))
+        {
+            const_value = unalignedLoad<FieldType>(vec);
+            get_key_holder_impl = [this](size_t /*row*/) { return const_value; };
+        }
+        else
+        {
+            get_key_holder_impl = [this](size_t row) { return unalignedLoad<FieldType>(vec + row * sizeof(FieldType)); };
+        }


Let's remove code duplication from these two constructors to a separate method

alexey-milovidov · 2022-08-11T02:19:24Z

@Avogar @canhld94 The Aggregation methods don't expect to work const columns and its implementation is correct.
It is reasonable to keep code compact. The caller should materialize const columns before using them.

alexey-milovidov · 2022-08-11T02:20:41Z

The tests should check both INTERSECT and EXCEPT.

alexey-milovidov

.

canhld94 · 2022-08-11T02:52:50Z

@Avogar @canhld94 The Aggregation methods don't expect to work const columns and its implementation is correct. It is reasonable to keep code compact. The caller should materialize const columns before using them.

@alexey-milovidov done materializing column in IntersectOrExceptTransform and add more test. In addition, as your suggestion, I remove some code in HashMethodString to handle const column (added in #37738 to fix server crashing while intersect const column, this PR should fix that as well).

Avogar

Now it looks much better

Avogar · 2022-08-11T10:25:42Z

Let's wait for the rest of checks and merge it

Dismiss to merge

Backport #40020 to 22.6: fix HashMethodOneNumber with const column

Backport #40020 to 22.3: fix HashMethodOneNumber with const column

amosbird · 2022-08-30T12:00:04Z

tests/queries/0_stateless/02381_intersect_except_const_column.sql

+-- Test: except return incorrect result for const column
+SELECT 2 FROM numbers(10) EXCEPT SELECT 1 FROM numbers(5);
+SELECT toString(2) FROM numbers(10) EXCEPT SELECT toString(1) FROM numbers(5);
+SELECT '2' FROM numbers(10) EXCEPT SELECT '1' FROM numbers(5);


Why doesn't CI complain about missing EOL?

I think the CI only run style check for code file not test file 😂

OK. There is a well-known adage: "all text files should end with a newline". POSIX also has a definition of what is a line . I think it's better to apply this rule to all text files.

Ok, I will notice it for future PRs.

Backport #40020 to 22.7: fix HashMethodOneNumber with const column

Backport ClickHouse#40020 to 22.5: fix HashMethodOneNumber with const column

robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Aug 9, 2022

Avogar self-assigned this Aug 9, 2022

qoega added the can be tested Allows running workflows for external contributors label Aug 9, 2022

Avogar approved these changes Aug 9, 2022

View reviewed changes

canhld94 added 5 commits August 10, 2022 18:59

fix HashMethodOneNumber with const column

cdd5b32

simplify solution

1a08161

fix other hash methods and add tests

7910a93

increase test size

9e865d4

better implementation

17fa807

canhld94 force-pushed the ch_canh_fix_hash branch from 2085583 to 17fa807 Compare August 10, 2022 11:00

canhld94 added 2 commits August 10, 2022 19:28

fix style

ec49f7b

fix hashMethodOneNumber constructor

bf5e917

Avogar reviewed Aug 10, 2022

View reviewed changes

alexey-milovidov previously requested changes Aug 11, 2022

View reviewed changes

alexey-milovidov self-assigned this Aug 11, 2022

materialize column instead of handling column in hash method

84cd867

Avogar reviewed Aug 11, 2022

View reviewed changes

Avogar approved these changes Aug 11, 2022

View reviewed changes

Avogar added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Aug 11, 2022

canhld94 requested a review from alexey-milovidov August 12, 2022 02:10

Avogar merged commit 4c7222d into ClickHouse:master Aug 12, 2022

robot-ch-test-poll mentioned this pull request Aug 12, 2022

Backport #40020 to 22.3: fix HashMethodOneNumber with const column #40160

Merged

robot-clickhouse pushed a commit that referenced this pull request Aug 12, 2022

Backport #40020 to 22.3: fix HashMethodOneNumber with const column

4baa611

robot-ch-test-poll mentioned this pull request Aug 12, 2022

Backport #40020 to 22.5: fix HashMethodOneNumber with const column #40161

Merged

robot-clickhouse pushed a commit that referenced this pull request Aug 12, 2022

Backport #40020 to 22.5: fix HashMethodOneNumber with const column

4d942a4

robot-ch-test-poll mentioned this pull request Aug 12, 2022

Backport #40020 to 22.6: fix HashMethodOneNumber with const column #40162

Merged

robot-clickhouse pushed a commit that referenced this pull request Aug 12, 2022

Backport #40020 to 22.6: fix HashMethodOneNumber with const column

4fefab4

robot-ch-test-poll mentioned this pull request Aug 12, 2022

Backport #40020 to 22.7: fix HashMethodOneNumber with const column #40163

Merged

robot-clickhouse pushed a commit that referenced this pull request Aug 12, 2022

Backport #40020 to 22.7: fix HashMethodOneNumber with const column

4404504

robot-clickhouse added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Aug 18, 2022

alesapin pushed a commit that referenced this pull request Aug 22, 2022

Merge pull request #40162 from ClickHouse/backport/22.6/40020

237f084

Backport #40020 to 22.6: fix HashMethodOneNumber with const column

alesapin pushed a commit that referenced this pull request Aug 26, 2022

Merge pull request #40160 from ClickHouse/backport/22.3/40020

9f46539

Backport #40020 to 22.3: fix HashMethodOneNumber with const column

amosbird reviewed Aug 30, 2022

View reviewed changes

Felixoid pushed a commit that referenced this pull request Sep 2, 2022

Merge pull request #40163 from ClickHouse/backport/22.7/40020

6776353

Backport #40020 to 22.7: fix HashMethodOneNumber with const column

ch-devops pushed a commit to ClibMouse/ClickHouse that referenced this pull request Feb 1, 2023

Merge pull request ClickHouse#40161 from ClickHouse/backport/22.5/40020

51d2688

Backport ClickHouse#40020 to 22.5: fix HashMethodOneNumber with const column

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix HashMethodOneNumber with const column #40020

fix HashMethodOneNumber with const column #40020

canhld94 commented Aug 9, 2022 •

edited by Avogar

Loading

Avogar commented Aug 9, 2022

Avogar commented Aug 9, 2022

canhld94 commented Aug 9, 2022

canhld94 commented Aug 9, 2022

canhld94 commented Aug 9, 2022 •

edited

Loading

canhld94 commented Aug 10, 2022

Avogar Aug 10, 2022 •

edited

Loading

alexey-milovidov commented Aug 11, 2022 •

edited

Loading

alexey-milovidov commented Aug 11, 2022

alexey-milovidov left a comment

canhld94 commented Aug 11, 2022

Avogar left a comment

Avogar commented Aug 11, 2022

amosbird Aug 30, 2022

canhld94 Aug 30, 2022 •

edited

Loading

amosbird Aug 30, 2022

canhld94 Aug 30, 2022

fix HashMethodOneNumber with const column #40020

fix HashMethodOneNumber with const column #40020

Conversation

canhld94 commented Aug 9, 2022 • edited by Avogar Loading

Changelog category (leave one):

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

Avogar commented Aug 9, 2022

Avogar commented Aug 9, 2022

canhld94 commented Aug 9, 2022

canhld94 commented Aug 9, 2022

canhld94 commented Aug 9, 2022 • edited Loading

canhld94 commented Aug 10, 2022

Avogar Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

alexey-milovidov commented Aug 11, 2022 • edited Loading

alexey-milovidov commented Aug 11, 2022

alexey-milovidov left a comment

Choose a reason for hiding this comment

canhld94 commented Aug 11, 2022

Avogar left a comment

Choose a reason for hiding this comment

Avogar commented Aug 11, 2022

amosbird Aug 30, 2022

Choose a reason for hiding this comment

canhld94 Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

amosbird Aug 30, 2022

Choose a reason for hiding this comment

canhld94 Aug 30, 2022

Choose a reason for hiding this comment

canhld94 commented Aug 9, 2022 •

edited by Avogar

Loading

canhld94 commented Aug 9, 2022 •

edited

Loading

Avogar Aug 10, 2022 •

edited

Loading

alexey-milovidov commented Aug 11, 2022 •

edited

Loading

canhld94 Aug 30, 2022 •

edited

Loading