Skip to content

ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function #16610

Merged
KochetovNicolai merged 7 commits intoClickHouse:masterfrom
zhang2014:fix/ISSUES-16574
Nov 16, 2020
Merged

ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function #16610
KochetovNicolai merged 7 commits intoClickHouse:masterfrom
zhang2014:fix/ISSUES-16574

Conversation

@zhang2014
Copy link
Copy Markdown
Contributor

@zhang2014 zhang2014 commented Nov 2, 2020 •
edited
Loading

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 (a user-readable short description of the changes that goes to CHANGELOG.md):
fixes #16574
fixes #16231
fix remote query failure when using 'if' suffix aggregate function

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Nov 2, 2020
@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 2, 2020 •
edited
Loading

select sumIf(42, toNullable(toInt64(1))) from DT

I'm not sure if I should fix this. because the second argument type is not UInt8

@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 3, 2020

CC: @KochetovNicolai

@KochetovNicolai
Copy link
Copy Markdown
Member

KochetovNicolai commented Nov 3, 2020

I'm not sure if I should fix this. because the second argument type is not UInt8

Agree. It is expected.

@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 6, 2020

AST fuzzer — Assertion `V && "PHI node got a null value!"' failed -- looks unrelated
test_https_replication/test.py::test_replication_after_partition -- looks unrelated
test_distributed_ddl_parallel/test.py::test_two_in_parallel_two_queued -- looks unrelated

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 7, 2020

One test has failed.

@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 9, 2020

@alexey-milovidov This is correct?

SELECT NULL > 0

Query id: a792cf05-c316-425b-a2a1-1368790b3ace

┌─greater(NULL, 0)─┐
│ ᴺᵁᴸᴸ             │
└──────────────────┘

@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 9, 2020

test have been added in 85ed639 from @KochetovNicolai

@zhang2014
Copy link
Copy Markdown
Contributor Author

zhang2014 commented Nov 11, 2020

Performance — 2 errors, 1 too long, 8 faster, 91 unstable

Some queries have unexpected duration - unrelated
The test 'read_in_order_many_parts' is too slow to run as a whole. Investigate whether the create and fill queries can be sped up - unrelated

@KochetovNicolai
Copy link
Copy Markdown
Member

KochetovNicolai commented Nov 13, 2020

Also #16588 may be affected

@KochetovNicolai KochetovNicolai self-assigned this Nov 13, 2020
@KochetovNicolai
Added test from ClickHouse#16588
257bbca 
(cherry picked from commit b5a8ef3)
@KochetovNicolai KochetovNicolai merged commit 00de84f into ClickHouse:master Nov 16, 2020
This was referenced Nov 16, 2020
Merged
Merged
Merged
Merged
robot-clickhouse pushed a commit that referenced this pull request Nov 16, 2020
Backport #16610 to 20.11: ISSUES-16574 try fix remote query failure w…
65e882e 
…hen using 'if' suffix aggregate function
@robot-clickhouse robot-clickhouse mentioned this pull request Nov 16, 2020
Merged
robot-clickhouse pushed a commit that referenced this pull request Nov 16, 2020
Backport #16610 to 20.9: ISSUES-16574 try fix remote query failure wh…
57ceb0c 
…en using 'if' suffix aggregate function
@robot-clickhouse robot-clickhouse mentioned this pull request Nov 16, 2020
Merged
robot-clickhouse pushed a commit that referenced this pull request Nov 16, 2020
Backport #16610 to 20.8: ISSUES-16574 try fix remote query failure wh…
035327e 
…en using 'if' suffix aggregate function
@robot-clickhouse robot-clickhouse mentioned this pull request Nov 16, 2020
Merged
robot-clickhouse pushed a commit that referenced this pull request Nov 16, 2020
Backport #16610 to 20.10: ISSUES-16574 try fix remote query failure w…
d025f89 
…hen using 'if' suffix aggregate function
@robot-clickhouse robot-clickhouse mentioned this pull request Nov 16, 2020
Merged
KochetovNicolai added a commit that referenced this pull request Nov 17, 2020
@KochetovNicolai
Merge pull request #17074 from ClickHouse/backport/20.11/16610
8122910 
Backport #16610 to 20.11: ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function
KochetovNicolai added a commit that referenced this pull request Nov 17, 2020
@KochetovNicolai
Merge pull request #17107 from ClickHouse/backport/20.10/16610
bec8a7f 
Backport #16610 to 20.10: ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function
KochetovNicolai added a commit that referenced this pull request Nov 19, 2020
@KochetovNicolai
Merge pull request #17106 from ClickHouse/backport/20.8/16610
801032c 
Backport #16610 to 20.8: ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function
KochetovNicolai added a commit that referenced this pull request Nov 19, 2020
@KochetovNicolai
Merge pull request #17105 from ClickHouse/backport/20.9/16610
b518a21 
Backport #16610 to 20.9: ISSUES-16574 try fix remote query failure when using 'if' suffix aggregate function
azat added a commit to azat/ClickHouse that referenced this pull request Jan 7, 2021
@azat
Fix *If combinator with Nullable types
fdcfacd 
sumIf(Nullable()) and similar unary functions (unary w/o If combinator)
was working incorrectly, since it returns "sum" from the getName()
helper, and so distributed query processing fails.

The problem is in the optimization in
AggregateFunctionIfNullUnary::add() for the unary functions. It pass
only one column to write result to, instead of all passed arguments +
result columns.
While AggregateFunctionIf::add() assumes that it accepts arguments  +
result columns, and use last column as a result.

Introduced-in: ClickHouse#16610
Fixes: ClickHouse#18210
This was referenced Jan 7, 2021
Merged
Closed
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jan 15, 2021

@KochetovNicolai The fix was not backported to 20.8:

$ clickhouse-client 
ClickHouse client version 21.1.1.5646.
Connecting to localhost:9000 as user default.
Connected to ClickHouse server version 20.8.12 revision 54438.

ClickHouse server version is older than ClickHouse client. It may indicate that the server is out of date and can be upgraded.

milovidov-desktop :) SELECT sumIf(toNullable(1), 1) FROM remote('127.0.0.{1,2}', system.one)

SELECT sumIf(toNullable(1), 1)
FROM remote('127.0.0.{1,2}', system.one)

Query id: fa832274-69c1-4686-9d47-230c2c5ba23c

→ Progress: 1.00 rows, 1.00 B (9.79 rows/s., 9.79 B/s.) 
Received exception from server (version 20.8.12):
Code: 42. DB::Exception: Received from localhost:9000. DB::Exception: Aggregate function sum requires single argument: while receiving packet from 127.0.0.2:9000: While executing Remote. 

0 rows in set. Elapsed: 0.133 sec.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Jan 15, 2021

Looks like #18806.

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

Reviewers

@KochetovNicolai KochetovNicolai KochetovNicolai approved these changes

Assignees

@KochetovNicolai KochetovNicolai

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrong behavior for sumIf with non-nullable first argument and nullable second argument -If combinator and NULLs

4 participants

@zhang2014 @KochetovNicolai @alexey-milovidov @robot-clickhouse