Skip to content

Commit a1bf3f1

Browse files
alexey-milovidovalexey-milovidov
authored
Merge pull request ClickHouse#55761 from ClickHouse/backport/23.9/55678
Backport ClickHouse#55678 to 23.9: Fix filtering by virtual columns with OR filter in query (resubmit)
2 parents 6f8349c + 064d8d2 commit a1bf3f1

5 files changed

Lines changed: 93 additions & 11 deletions

File tree

‎src/Storages/VirtualColumnUtils.cpp‎

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include <algorithm>
12
#include <memory>
23
#include <Core/NamesAndTypes.h>
34
#include <Core/TypeId.h>
@@ -81,14 +82,33 @@ bool extractFunctions(const ASTPtr & expression, const std::function<bool(const
8182
}
8283
else if (function->name == "or")
8384
{
84-
bool ret = true;
85+
bool ret = false;
8586
ASTs or_args;
8687
for (const auto & child : function->arguments->children)
87-
ret &= extractFunctions(child, is_constant, or_args);
88-
/// We can keep condition only if it still OR condition (i.e. we
89-
/// have dependent conditions for columns at both sides)
90-
if (or_args.size() == 2)
88+
ret |= extractFunctions(child, is_constant, or_args);
89+
90+
if (!or_args.empty())
91+
{
92+
/// In case of there are less number of arguments for which
93+
/// is_constant() == true, we need to add always-true
94+
/// implicitly to avoid breaking AND invariant.
95+
///
96+
/// Consider the following:
97+
///
98+
/// ((value = 10) OR (_table = 'v2')) AND ((_table = 'v1') OR (value = 20))
99+
///
100+
/// Without implicit always-true:
101+
///
102+
/// (_table = 'v2') AND (_table = 'v1')
103+
///
104+
/// With:
105+
///
106+
/// (_table = 'v2' OR 1) AND (_table = 'v1' OR 1) -> (_table = 'v2') OR (_table = 'v1')
107+
///
108+
if (or_args.size() != function->arguments->children.size())
109+
or_args.push_back(std::make_shared<ASTLiteral>(Field(1)));
91110
result.push_back(makeASTForLogicalOr(std::move(or_args)));
111+
}
92112
return ret;
93113
}
94114
}
@@ -165,8 +185,10 @@ bool prepareFilterBlockWithQuery(const ASTPtr & query, ContextPtr context, Block
165185
if (!select.where() && !select.prewhere())
166186
return unmodified;
167187

168-
// Provide input columns as constant columns to check if an expression is constant.
169-
std::function<bool(const ASTPtr &)> is_constant = [&block, &context](const ASTPtr & node)
188+
// Provide input columns as constant columns to check if an expression is
189+
// constant and depends on the columns from provided block (the last is
190+
// required to allow skipping some conditions for handling OR).
191+
std::function<bool(const ASTPtr &)> is_constant = [&block, &context](const ASTPtr & expr)
170192
{
171193
auto actions = std::make_shared<ActionsDAG>(block.getColumnsWithTypeAndName());
172194
PreparedSetsPtr prepared_sets = std::make_shared<PreparedSets>();
@@ -178,13 +200,26 @@ bool prepareFilterBlockWithQuery(const ASTPtr & query, ContextPtr context, Block
178200
context, SizeLimits{}, 1, source_columns, std::move(actions), prepared_sets, true, true, true,
179201
{ aggregation_keys, grouping_set_keys, GroupByKind::NONE });
180202

181-
ActionsVisitor(visitor_data).visit(node);
203+
ActionsVisitor(visitor_data).visit(expr);
182204
actions = visitor_data.getActions();
205+
auto expr_column_name = expr->getColumnName();
206+
207+
const auto * expr_const_node = actions->tryFindInOutputs(expr_column_name);
208+
if (!expr_const_node)
209+
return false;
210+
auto filter_actions = ActionsDAG::buildFilterActionsDAG({expr_const_node}, {}, context);
211+
const auto & nodes = filter_actions->getNodes();
212+
bool has_dependent_columns = std::any_of(nodes.begin(), nodes.end(), [&](const auto & node)
213+
{
214+
return block.has(node.result_name);
215+
});
216+
if (!has_dependent_columns)
217+
return false;
218+
183219
auto expression_actions = std::make_shared<ExpressionActions>(actions);
184220
auto block_with_constants = block;
185221
expression_actions->execute(block_with_constants);
186-
auto column_name = node->getColumnName();
187-
return block_with_constants.has(column_name) && isColumnConst(*block_with_constants.getByName(column_name).column);
222+
return block_with_constants.has(expr_column_name) && isColumnConst(*block_with_constants.getByName(expr_column_name).column);
188223
};
189224

190225
/// Create an expression that evaluates the expressions in WHERE and PREWHERE, depending only on the existing columns.

‎tests/integration/test_system_merges/test.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ def test_mutation_simple(started_cluster, replicated):
186186
# ALTER will sleep for 3s * 3 (rows) = 9s
187187
def alter():
188188
node1.query(
189-
f"ALTER TABLE {name} UPDATE a = 42 WHERE sleep(3) OR 1",
189+
f"ALTER TABLE {name} UPDATE a = 42 WHERE sleep(9) = 0",
190190
settings=settings,
191191
)
192192

‎tests/queries/0_stateless/02840_merge__table_or_filter.sql.j2‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ create view v2 as select * from d2;
1818

1919
create table m as v1 engine=Merge(currentDatabase(), '^(v1|v2)$');
2020

21+
{# -- FIXME:
22+
select _table, key from m where (value = 10 and _table = 'v1') or (value = 20 and _table = 'v1') or 0 or 0 settings {{ settings }};
23+
select _table, key from m where (value = 10 and _table = 'v3') or (value = 20 and _table = 'v3') or 0 or 0 settings {{ settings }};
24+
#}
25+
2126
-- avoid reorder
2227
set max_threads=1;
2328
-- { echoOn }

‎tests/queries/0_stateless/02896_multiple_OR.reference‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
-- { echoOn }
2+
SELECT * FROM or_bug WHERE (key = 1) OR false OR false;
3+
1
4+
SELECT * FROM or_bug WHERE (key = 1) OR false;
5+
1
6+
SELECT * FROM or_bug WHERE (key = 1);
7+
1
8+
-- { echoOn }
9+
select * from forms where text_field like '%this%' or 0 = 1 or 0 = 1;
10+
5840ead423829c1eab29fa97 this is a test
11+
select * from forms where text_field like '%this%' or 0 = 1;
12+
5840ead423829c1eab29fa97 this is a test
13+
select * from forms where text_field like '%this%';
14+
5840ead423829c1eab29fa97 this is a test

‎tests/queries/0_stateless/02896_multiple_OR.sql‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
-- https://github.com/ClickHouse/ClickHouse/pull/52653
2+
DROP TABLE IF EXISTS or_bug;
3+
CREATE TABLE or_bug (key UInt8) ENGINE=MergeTree ORDER BY key;
4+
INSERT INTO TABLE or_bug VALUES (0), (1);
5+
6+
-- { echoOn }
7+
SELECT * FROM or_bug WHERE (key = 1) OR false OR false;
8+
SELECT * FROM or_bug WHERE (key = 1) OR false;
9+
SELECT * FROM or_bug WHERE (key = 1);
10+
-- { echoOff }
11+
12+
-- https://github.com/ClickHouse/ClickHouse/issues/55288
13+
DROP TABLE IF EXISTS forms;
14+
CREATE TABLE forms
15+
(
16+
`form_id` FixedString(24),
17+
`text_field` String
18+
)
19+
ENGINE = MergeTree
20+
PRIMARY KEY form_id
21+
ORDER BY form_id;
22+
insert into forms values ('5840ead423829c1eab29fa97','this is a test');
23+
24+
-- { echoOn }
25+
select * from forms where text_field like '%this%' or 0 = 1 or 0 = 1;
26+
select * from forms where text_field like '%this%' or 0 = 1;
27+
select * from forms where text_field like '%this%';
28+
-- { echoOff }

0 commit comments

Comments
 (0)