CompileExpressions comparison function constant case fix by kitaisreal · Pull Request #24023 · ClickHouse/ClickHouse

kitaisreal · 2021-05-11T19:02:42Z

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

Fixes #24020.

Changelog category (leave one):

Not for changelog (changelog entry is not required)

qoega · 2021-05-12T07:11:58Z

src/Interpreters/ExpressionJIT.cpp

typo? Noomparision

alesapin · 2021-05-12T10:21:16Z

SQLancer found more:

--java.lang.AssertionError: SELECT SUM((t0.c1) or (((((t0.c0) IS NOT NULL)) IS NOT NULL))), MAX((((t0.c1)>=(((t0.c1) IS NULL)))) or (t0.c1)), COUNT(((((((t0.c1) IS NULL)) IS NULL))==(((t0.c1)>=(t0.c0))))), COUNT((t0.c0) and ((- (((t0.c0)<=(t0.c1)))))), SUM((- (t0.c1))) FROM t0 GROUP BY t0.c1, t0.c0 HAVING (NOT (t0.c1)) UNION ALL SELECT SUM((t0.c1) or (((((t0.c0) IS NOT NULL)) IS NOT NULL))), MAX((((t0.c1)>=(((t0.c1) IS NULL)))) or (t0.c1)), COUNT(((((((t0.c1) IS NULL)) IS NULL))==(((t0.c1)>=(t0.c0))))), COUNT((t0.c0) and ((- (((t0.c0)<=(t0.c1)))))), SUM((- (t0.c1))) FROM t0 GROUP BY t0.c1, t0.c0 HAVING (NOT ((NOT (t0.c1)))) UNION ALL SELECT SUM((t0.c1) or (((((t0.c0) IS NOT NULL)) IS NOT NULL))), MAX((((t0.c1)>=(((t0.c1) IS NULL)))) or (t0.c1)), COUNT(((((((t0.c1) IS NULL)) IS NULL))=(((t0.c1)>=(t0.c0))))), COUNT((t0.c0) and ((- (((t0.c0)<=(t0.c1)))))), SUM((- (t0.c1))) FROM t0 GROUP BY t0.c1, t0.c0 HAVING (((NOT (t0.c1))) IS NULL) SETTINGS aggregate_functions_null_for_empty=1, enable_optimize_predicate_expression=0;
--	at sqlancer.common.query.SQLQueryAdapter.checkException(SQLQueryAdapter.java:100)
--	at sqlancer.common.query.SQLQueryAdapter.executeAndGet(SQLQueryAdapter.java:131)
--	at sqlancer.ComparatorHelper.getResultSetFirstColumnAsString(ComparatorHelper.java:54)
--	at sqlancer.clickhouse.oracle.tlp.ClickHouseTLPHavingOracle.check(ClickHouseTLPHavingOracle.java:66)
--	at sqlancer.ProviderAdapter.generateAndTestDatabase(ProviderAdapter.java:49)
--	at sqlancer.Main$DBMSExecutor.run(Main.java:326)
--	at sqlancer.Main$2.run(Main.java:510)
--	at sqlancer.Main$2.runThread(Main.java:488)
--	at sqlancer.Main$2.run(Main.java:478)
--	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
--	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
--	at java.base/java.lang.Thread.run(Thread.java:832)
--Caused by: ru.yandex.clickhouse.except.ClickHouseException: ClickHouse exception, code: 44, host: localhost, port: 8123; Code: 44, e.displayText() = DB::Exception: Cannot convert column `equals(isNull(isNull(c1)), greaterOrEquals(c1, c0))` because it is non constant in source stream but must be constant in result (version 21.6.1.6812)```

qoega · 2021-05-12T15:35:46Z

Why Cannot convert column is not a LOGICAL_ERROR? We should find bugs like this in AST Fuzzer

akuzm · 2021-05-12T20:17:12Z

src/Interpreters/ExpressionJIT.cpp

This should be done by the expression analyzer at about the same stage where it evaluates constants.

src/Functions/FunctionsComparison.h

KochetovNicolai · 2021-05-14T09:26:41Z

src/Interpreters/TautologicalComparisonOptimizer.cpp

Ok, but maybe we can replace the whole function to literal 1 or literal 0

@KochetovNicolai right now seems easier to understand what is going on.

KochetovNicolai · 2021-05-14T09:28:07Z

src/Interpreters/TreeRewriter.cpp

pls remove debug output

…optimization

alesapin · 2021-05-17T07:18:03Z

tests/queries/0_stateless/00911_tautological_compare.sql

-SELECT count() FROM system.numbers WHERE number != number;
-SELECT count() FROM system.numbers WHERE number < number;
-SELECT count() FROM system.numbers WHERE number > number;
+-- SELECT count() FROM system.numbers WHERE number != number;


I hope we will revert this?

It is not so easy to do.
It's actually not so clear why reading from infinite table must return at all.
We can go farter and want a query SELECT count() FROM system.numbers WHERE number < 10 return 10. But is it necessary?

This optimizations is implemented in a not obvious way for now. We expected that a function != for two same non-constant arguments will return constant 0, and assume that constant will remain. It could have been ok if we had some meta info that columns are really same. But we just compare two ptrs. I don't think there is a guarantee that it is true for all other blocks. (However, I could not find counterexample. Queries like where x != if(blockNumber() == 0, x, y) work correctly only because blockNumber does not return constant.)

alesapin · 2021-05-18T08:45:12Z

Stress test:

zgrep -a '8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1' clickhouse-server.log.gz
2021.05.18 00:09:15.537857 [ 35027 ] {8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1} <Debug> executeQuery: (from [::1]:50154, using production parser) (comment: /usr/share/clickhouse-test/queries/0_stateless/00429_long_http_bufferization.sh) SELECT greatest(toUInt8(1), toUInt8(intHash64(number))) FROM system.numbers LIMIT 2500000 FORMAT RowBinary
2021.05.18 00:09:15.544607 [ 35027 ] {8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1} <Trace> ContextAccess (default): Access granted: SELECT(number) ON system.numbers
2021.05.18 00:09:15.546831 [ 35027 ] {8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1} <Trace> InterpreterSelectQuery: FetchColumns -> Complete
2021.05.18 00:09:18.726076 [ 35027 ] {8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1} <Information> executeQuery: Read 2500000 rows, 19.07 MiB in 3.187516281 sec., 784309 rows/sec., 5.98 MiB/sec.
2021.05.18 00:09:18.819026 [ 35027 ] {8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1} <Debug> DynamicQueryHandler: Done processing query
2021.05.18 00:09:37.379188 [ 78460 ] {} <Fatal> BaseDaemon: (version 21.6.1.6862, build id: 902A04D557495CBDC22EAA45AEF413357E72A75C) (from thread 35027) (query_id: 8c05de33-7dd9-47ef-a4b0-d7ee11ddbac1) Received signal Aborted (6)
2021.05.18 00:09:37.378816 [ 78460 ] {} <Fatal> BaseDaemon: ########################################
2021.05.18 00:09:37.379421 [ 78460 ] {} <Fatal> BaseDaemon: 
2021.05.18 00:09:37.498338 [ 78460 ] {} <Fatal> BaseDaemon: Stack trace: 0x7f08fdd2b18b 0x7f08fdd0a859 0x8f7cf6e 0x921335d 0x1931b354 0x1931b247 0x9009a3b 0x143c12a8 0x143488fe 0x1433fd0f 0x143ba4e1 0x16fa81a3 0x16fa8930 0x1711fb02 0x1711df90 0x1711c708 0x8f7776d 0x7f08fdee0609 0x7f08fde07293
2021.05.18 00:09:37.498812 [ 78460 ] {} <Fatal> BaseDaemon: 5. raise @ 0x4618b in /usr/lib/x86_64-linux-gnu/libc-2.31.so
2021.05.18 00:09:37.499068 [ 78460 ] {} <Fatal> BaseDaemon: 6. abort @ 0x25859 in /usr/lib/x86_64-linux-gnu/libc-2.31.so
2021.05.18 00:09:50.095824 [ 78460 ] {} <Fatal> BaseDaemon: 7. abort @ 0x8f7cf6e in /usr/bin/clickhouse
2021.05.18 00:09:50.598741 [ 78460 ] {} <Fatal> BaseDaemon: 8. ./obj-x86_64-linux-gnu/../base/daemon/BaseDaemon.cpp:0: terminate_handler() @ 0x921335d in /usr/bin/clickhouse
2021.05.18 00:09:50.648051 [ 78460 ] {} <Fatal> BaseDaemon: 9. ./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/cxa_handlers.cpp:61: std::__terminate(void (*)()) @ 0x1931b354 in /usr/bin/clickhouse
2021.05.18 00:09:50.695754 [ 78460 ] {} <Fatal> BaseDaemon: 10.1. inlined from ./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/include/atomic_support.h:78: void (*std::__1::(anonymous namespace)::__libcpp_atomic_load<void (*)()>(void (* const*)(), int))()
2021.05.18 00:09:50.695959 [ 78460 ] {} <Fatal> BaseDaemon: 10.2. inlined from ../contrib/libcxxabi/src/cxa_handlers.cpp:49: std::get_terminate()
2021.05.18 00:09:50.696065 [ 78460 ] {} <Fatal> BaseDaemon: 10. ../contrib/libcxxabi/src/cxa_handlers.cpp:92: std::terminate() @ 0x1931b247 in /usr/bin/clickhouse
2021.05.18 00:10:03.808964 [ 292 ] {} <Fatal> Application: Child process was terminated by signal 6.

addr2line:

$ addr2line -iCe ./clickhouse 0x7f08fdd2b18b 0x7f08fdd0a859 0x8f7cf6e 0x921335d 0x1931b354 0x1931b247 0x9009a3b 0x143c12a8 0x143488fe 0x1433fd0f 0x143ba4e1 0x16fa81a3 0x16fa8930 0x1711fb02 0x1711df90 0x1711c708 0x8f7776d 0x7f08fdee0609 0x7f08fde07293
??:0
??:0
??:?
BaseDaemon.cpp:?
./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/cxa_handlers.cpp:61
./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/include/atomic_support.h:78
./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/cxa_handlers.cpp:49
./obj-x86_64-linux-gnu/../contrib/libcxxabi/src/cxa_handlers.cpp:92
main.cpp:?
./obj-x86_64-linux-gnu/../src/IO/BufferWithOwnMemory.h:48
./obj-x86_64-linux-gnu/../src/IO/BufferWithOwnMemory.h:137
./obj-x86_64-linux-gnu/../src/Server/HTTP/WriteBufferFromHTTPServerResponse.cpp:201
./obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:2615
./obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:2518
./obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3212
./obj-x86_64-linux-gnu/../src/Server/HTTPHandler.h:43
./obj-x86_64-linux-gnu/../src/Server/HTTPHandler.cpp:933
??:?
./obj-x86_64-linux-gnu/../contrib/poco/Net/src/TCPServerConnection.cpp:57
./obj-x86_64-linux-gnu/../contrib/poco/Net/src/TCPServerDispatcher.cpp:114
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/include/Poco/ScopedLock.h:36
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/src/ThreadPool.cpp:213
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/src/Thread.cpp:56
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/include/Poco/SharedPtr.h:277
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/include/Poco/SharedPtr.h:156
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/include/Poco/SharedPtr.h:208
./obj-x86_64-linux-gnu/../contrib/poco/Foundation/src/Thread_POSIX.cpp:360
crtstuff.c:?
??:0
??:0

alesapin · 2021-05-18T08:52:46Z

Wait for perf tests and merge.

alesapin · 2021-05-18T13:11:23Z


+1.249x | 0.249 | 0.247 | agg_functions_min_max_any
-- | -- | -- | --

Flap?

alesapin · 2021-05-18T14:14:45Z

I'll retry the task.

alesapin · 2021-05-19T11:22:54Z

column_column_comparison | xml.etree.ElementTree.ParseError: no element found: line 39, column 0

This will avoid possible std::terminate, like in [1]: [1]: ClickHouse#24023 (comment)

azat · 2021-09-29T13:34:39Z

@kitaisreal looks like this should be backported?

robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 11, 2021

qoega reviewed May 12, 2021

View reviewed changes

alesapin approved these changes May 12, 2021

View reviewed changes

alesapin self-assigned this May 12, 2021

akuzm reviewed May 12, 2021

View reviewed changes

kitaisreal force-pushed the compile-expressions-comparison-function-constant-case-with-alias-fix branch from 1f48e58 to af8e0d7 Compare May 13, 2021 14:19

KochetovNicolai reviewed May 14, 2021

View reviewed changes

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

KochetovNicolai reviewed May 14, 2021

View reviewed changes

KochetovNicolai approved these changes May 14, 2021

View reviewed changes

kitaisreal and others added 5 commits May 15, 2021 00:03

CompileExpressions comparison function constant case fix

26be39f

Update ExpressionJIT.cpp

f42b25e

Update ExpressionJIT.cpp

1889400

FunctionComparison remove constant result for non constant arguments …

0b4b5eb

…optimization

Disable broken test

6c33480

kitaisreal force-pushed the compile-expressions-comparison-function-constant-case-with-alias-fix branch 2 times, most recently from c2b324f to 6c33480 Compare May 15, 2021 15:30

alesapin reviewed May 17, 2021

View reviewed changes

kitaisreal added 3 commits May 17, 2021 10:25

Added comments

3a822d2

Updated comment message

448fe5a

Updated comment message

8ef916a

KochetovNicolai mentioned this pull request May 18, 2021

Log exception in Allocator::free #24256

Merged

Remove performance test

0ca9ba1

kitaisreal merged commit af78649 into ClickHouse:master May 20, 2021

azat mentioned this pull request Sep 3, 2021

Proper buffer flush while handling HTTP queries (avoids std::terminate) #28604

Merged

azat added a commit to azat/ClickHouse that referenced this pull request Sep 16, 2021

Always flush buffer while handling HTTP queries

230f29a

This will avoid possible std::terminate, like in [1]: [1]: ClickHouse#24023 (comment)

kitaisreal added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Sep 29, 2021

robot-clickhouse mentioned this pull request Sep 29, 2021

Cherry pick #24023 to 21.3: CompileExpressions comparison function constant case fix #29542

Closed

Conversation

kitaisreal commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin commented May 12, 2021

Uh oh!

qoega commented May 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alesapin commented May 18, 2021

Uh oh!

alesapin commented May 18, 2021

Uh oh!

alesapin commented May 18, 2021

Uh oh!

alesapin commented May 19, 2021

Uh oh!

azat commented Sep 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alesapin commented May 18, 2021 •

edited

Loading