Skip to content

nullIf return now the correct result when both arguments are constants#6446

Merged
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
PerformanceVision:null_if
Aug 12, 2019
Merged

nullIf return now the correct result when both arguments are constants#6446
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
PerformanceVision:null_if

Conversation

@YiuRULE
Copy link
Copy Markdown
Contributor

@YiuRULE YiuRULE commented Aug 12, 2019

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

For changelog. Remove if this is non-significant change.

Category (leave one):

  • Bug Fix

Short description (up to few sentences):

Resolve a bug with nullIf function when we send a NULL argument on the second argument.

Detailed description (optional):

Previously, SELECT nullIf(1, NULL) returned NULL when both side were constants. (the bugs weren't reproducible when one of this arguments were not constant).

Changing the logic of the function from if(equals(col_1, col_2), NULL, col_1) to if(notEquals(col_1, col_2), col_1, NULL) seem to fix the problem.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Aug 12, 2019

Ok.

PS. For reference: http://sqlfiddle.com/#!9/0a28f18/361

@alexey-milovidov alexey-milovidov merged commit 39ef3e3 into ClickHouse:master Aug 12, 2019
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Aug 14, 2019

Something is wrong:

milovidov@milovidov-Pro-P30:~/work/ClickHouse/dbms/tests/queries/0_stateless$ for i in {1..100}; do clickhouse-client --query "SELECT nullIf(1, NULL)"; done
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1
1
1
\N
1
1
1

@YiuRULE
Copy link
Copy Markdown
Contributor Author

YiuRULE commented Aug 16, 2019

Ok weird, thanks ! I will take a look

@YiuRULE
Copy link
Copy Markdown
Contributor Author

YiuRULE commented Aug 16, 2019

@alexey-milovidov How is exactly the configuration you used for your test ?

I tried both on this branch and on the latest master branch, with building clickhouse locally inside a build folder, with the default configuration we can find at dbms/programs/server/, and I tried the test locally.

for i in {1..100}; do build/dbms/programs/clickhouse client --query "SELECT nullIf(1,NULL)"; done
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1
1

Can you provide some input about it ? I really tried as much as I can to reproduce the bug, without success :/

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Aug 16, 2019

clang, debug build.
It also reproduces in our CI checks in various build configurations, example: https://clickhouse-test-reports.s3.yandex.net/0/b1d1a23318758c74bb7a883bf39022db584474b5/functional_stateless_tests_(ubsan).html

@YiuRULE YiuRULE mentioned this pull request Aug 16, 2019
Closed
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Aug 21, 2019

Previous implementation was correct in terms of idea but technically wrong.

@KochetovNicolai KochetovNicolai added pr-bugfix Pull request with bugfix, not backported by default pr-improvement Pull request with some product improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

No one assigned

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@YiuRULE @alexey-milovidov @KochetovNicolai