Fix logical functions with UInt8 numbers, when they not equal to 1. by CurtizJ · Pull Request #7795 · ClickHouse/ClickHouse

CurtizJ · 2019-11-15T16:33:47Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Bug Fix

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):

Fix logical functions with UInt8 numbers, when they not equal to 1.

Fixes #7772.

alexey-milovidov · 2019-11-16T05:02:06Z

@Akazz Ok, but need to check if there is any difference in performance.

alexey-milovidov · 2019-11-16T05:03:20Z

I suspect that usual WHERE clause with comparison and logical expressions may slow down.

Akazz

I suspect that usual WHERE clause with comparison and logical expressions may slow down.

I also admit that some minor performance decline is to be expected here, although all of the column conversion is easily vectorized by compiler.

Akazz · 2019-11-19T14:43:34Z

dbms/tests/queries/0_stateless/01035_logical_functions_with_uint8.sql

@@ -0,0 +1 @@
+select toUInt8(number) as n from numbers(100) where bitAnd(n, 1) and bitAnd(n, 2) and bitAnd(n, 4);


Okay, so right now we have a problem with UInt8 representation of bolean values. I suggest that we add a few more tests and a comment may be, because this line is far from being self explanatory. It is not clear from the line why we need 3 arguments for and in where expression and exactly why this is being checked. Probably it'd be nice to have the same checks for constants as well.

select materialize(1) and materialize(2) and materialize(4)
expected to be true

alexey-milovidov · 2019-12-03T02:05:15Z

@Akazz have you checked the performance manually?

CurtizJ · 2019-12-03T12:17:05Z

I've ran not very presice benchmark on 19.17 vs version with this patch and it showed about 10% performance degradation.

Query with 100 AND operands:

select count() from numbers(10000000) where number !=  96594 AND number !=  18511 AND number !=  98085 AND number !=  84177 AND number !=  70314 AND number !=  28083 AND number !=  54202 AND number !=  66522 AND number !=  66939 AND number !=  99469 AND number !=  65776 AND number !=  22876 AND number !=  42151 AND number !=  19924 AND number !=  66681 AND number !=  63022 AND number !=  17487 AND number !=  83914 AND number !=  59754 AND number !=  968 AND number !=  73334 AND number !=  68569 AND number !=  49853 AND number !=  33155 AND number !=  31777 AND number !=  99698 AND number !=  26708 AND number !=  76409 AND number !=  42191 AND number !=  55397 AND number !=  25724 AND number !=  39170 AND number !=  22728 AND number !=  98238 AND number !=  86052 AND number !=  12756 AND number !=  13948 AND number !=  57774 AND number !=  82511 AND number !=  11337 AND number !=  23506 AND number !=  11875 AND number !=  58536 AND number !=  56919 AND number !=  25986 AND number !=  80710 AND number !=  61797 AND number !=  99244 AND number !=  11665 AND number !=  15758 AND number !=  82899 AND number !=  63150 AND number !=  7198 AND number !=  40071 AND number !=  46310 AND number !=  78488 AND number !=  9273 AND number !=  91878 AND number !=  57904 AND number !=  53941 AND number !=  75675 AND number !=  12093 AND number !=  50090 AND number !=  59675 AND number !=  41632 AND number !=  81448 AND number !=  46821 AND number !=  51919 AND number !=  49028 AND number !=  71059 AND number !=  15673 AND number !=  6132 AND number !=  15473 AND number !=  32527 AND number !=  63842 AND number !=  33121 AND number !=  53271 AND number !=  86033 AND number !=  96807 AND number !=  4791 AND number !=  80089 AND number !=  51616 AND number !=  46311 AND number !=  82844 AND number !=  59353 AND number !=  63538 AND number !=  64857 AND number !=  58471 AND number !=  29870 AND number !=  80209 AND number !=  61000 AND number !=  75991 AND number !=  44506 AND number !=  11283 AND number !=  6335 AND number !=  73502 AND number !=  22354 AND number !=  72816 AND number !=  66399 AND number !=  61703

This patch result:

{
"localhost:9000": {
"statistics": {
"QPS": 0.7945934000204867,
"RPS": 7967394.378752621,
"MiBPS": 63739155.03002097,
"RPS_result": 0.7945934000204867,
"MiBPS_result": 6.356747200163894,
"num_queries": 30
},
"query_time_percentiles": {
"0": 1.174314468,
"10": 1.1959964470999997,
"20": 1.2092565354,
"30": 1.2305292104999999,
"40": 1.2402944656,
"50": 1.2500725035000002,
"60": 1.2570026738,
"70": 1.2682743527000002,
"80": 1.3010615592,
"90": 1.3207626721,
"95": 1.3883882848,
"99": 1.39261084439,
"99.9": 1.393185934139,
"99.99": 1.3932434431139
}
}
}

v.19.17 result:

{
"localhost:9000": {
"statistics": {
"QPS": 0.8789235246925504,
"RPS": 8812973.213480402,
"MiBPS": 70503785.70784321,
"RPS_result": 0.8789235246925504,
"MiBPS_result": 7.031388197540403,
"num_queries": 30
},
"query_time_percentiles": {
"0": 1.092902824,
"10": 1.0999579718,
"20": 1.1073646743999999,
"30": 1.1196764797,
"40": 1.1238697316,
"50": 1.1321403755000001,
"60": 1.1359128695999998,
"70": 1.1404816986,
"80": 1.1483627204000002,
"90": 1.1713929098000002,
"95": 1.2149165078,
"99": 1.2782067257,
"99.9": 1.29603257417,
"99.99": 1.2978151590169997
}
}
}

alexey-milovidov · 2019-12-03T12:25:34Z

@CurtizJ Let's add this query to performance tests in separate PR?

alexey-milovidov · 2019-12-03T12:27:51Z

Fix logical functions with UInt8 numbers, when they not equal to 1.

What was the cause of this error? Maybe we are using bit ANDs instead of logical ANDs to avoid less efficient short-curcuit code in multiple AND chains?

CurtizJ · 2019-12-03T13:07:01Z

Maybe we are using bit ANDs instead of logical ANDs

Yes, we use them, but for another reason. I've tried to expalin it in comment.

alexey-milovidov · 2019-12-03T13:25:45Z

This performance regression is almost acceptable but not quite.

alexey-milovidov · 2020-02-24T18:17:08Z

Still relevant.

alexey-milovidov · 2020-02-26T21:38:18Z

@blinkov This PR is significant.

qoega · 2020-06-03T11:05:36Z

@CurtizJ @Akazz Do you have ideas how we can lose less performance? Do you plan to return to this issue?

Akazz · 2020-06-03T15:31:22Z

I have reworked it somewhat and I am to push new changes within this week.

fix logical functions with UInt8

28a08ac

CurtizJ changed the title ~~Fix logical functions with UInt8 numbers, when they not equals 1.~~ Fix logical functions with UInt8 numbers, when they not equal to 1. Nov 15, 2019

alexey-milovidov requested a review from Akazz November 16, 2019 05:01

Akazz reviewed Nov 19, 2019

View reviewed changes

Akazz mentioned this pull request Nov 19, 2019

Performance tests for logical functions #7847

Closed

stavrolia added the pr-bugfix Pull request with bugfix, not backported by default label Nov 26, 2019

qoega added the no-docs-needed label Feb 6, 2020

alexey-milovidov assigned Akazz Feb 24, 2020

mergify bot added the pr-non-significant label Feb 26, 2020

alexey-milovidov removed the pr-non-significant label Feb 26, 2020

alexey-milovidov removed the no-docs-needed label Feb 27, 2020

blinkov added the no-docs-needed label Feb 27, 2020

alexey-milovidov force-pushed the master branch from 6c77191 to 09b9a30 Compare June 9, 2020 02:04

Akazz mentioned this pull request Jul 7, 2020

Fixed logical functions for UInt8 values when they are not equal to 0 or 1. #12196

Merged

alexey-milovidov closed this in #12196 Jul 7, 2020

CurtizJ deleted the fix-logical-functions branch July 31, 2020 19:01

		@@ -0,0 +1 @@
		select toUInt8(number) as n from numbers(100) where bitAnd(n, 1) and bitAnd(n, 2) and bitAnd(n, 4);

Conversation

CurtizJ commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Nov 16, 2019

Uh oh!

alexey-milovidov commented Nov 16, 2019

Uh oh!

Akazz left a comment

Choose a reason for hiding this comment

Uh oh!

Akazz Nov 19, 2019

Choose a reason for hiding this comment

Uh oh!

filimonov Nov 19, 2019 • 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 commented Dec 3, 2019

Uh oh!

CurtizJ commented Dec 3, 2019

Uh oh!

alexey-milovidov commented Dec 3, 2019

Uh oh!

alexey-milovidov commented Dec 3, 2019

Uh oh!

CurtizJ commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Dec 3, 2019

Uh oh!

alexey-milovidov commented Feb 24, 2020

Uh oh!

alexey-milovidov commented Feb 26, 2020

Uh oh!

qoega commented Jun 3, 2020

Uh oh!

Akazz commented Jun 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CurtizJ commented Nov 15, 2019 •

edited

Loading

filimonov Nov 19, 2019 •

edited

Loading

CurtizJ commented Dec 3, 2019 •

edited

Loading