Skip to content

Improve Clickhouse varchar equalities pushdown #23455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ssheikin wants to merge 2 commits into from
Closed

Improve Clickhouse varchar equalities pushdown #23455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3047ad6
Allow to RewriteIn only for specific types
ssheikin Sep 17, 2024
f1a8cf4
Improve Clickhouse varchar connector expression equalities
ssheikin Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.plugin.jdbc.expression;

Copy link
Copy Markdown
Contributor Author

@ssheikin ssheikin Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#23455 (comment)

also I see clickhouse connector can both expose FixedString/String as Trino VARCHAR and VARBINARY (default).

So do we need to test in both modes the pushdown? Does it matter?

@hashhar @Praveen2112 considering

// TODO (#7102) reconsider default behavior

Support for VARBINARY may be considered as a separate effort.

import com.google.common.base.Joiner;
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import io.trino.matching.Capture;
import io.trino.matching.Captures;
Expand All @@ -22,6 +23,7 @@
import io.trino.plugin.jdbc.QueryParameter;
import io.trino.spi.expression.Call;
import io.trino.spi.expression.ConnectorExpression;
import io.trino.spi.type.Type;

import java.util.List;
import java.util.Optional;
Expand All @@ -40,6 +42,7 @@
import static io.trino.spi.expression.StandardFunctions.IN_PREDICATE_FUNCTION_NAME;
import static io.trino.spi.type.BooleanType.BOOLEAN;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class RewriteIn
implements ConnectorExpressionRule<Call, ParameterizedExpression>
Expand All @@ -54,6 +57,18 @@ public class RewriteIn
.with(argument(0).matching(expression().capturedAs(VALUE)))
.with(argument(1).matching(call().with(functionName().equalTo(ARRAY_CONSTRUCTOR_FUNCTION_NAME)).with(arguments().capturedAs(EXPRESSIONS))));

private final Predicate<Type> typePredicate;

public RewriteIn()
{
this(_ -> true);
}

public RewriteIn(Predicate<Type> typePredicate)
{
this.typePredicate = requireNonNull(typePredicate, "typePredicate is null");
}

@Override
public Pattern<Call> getPattern()
{
Expand All @@ -73,6 +88,9 @@ public Optional<ParameterizedExpression> rewrite(Call call, Captures captures, R
// We don't want to push down too long IN query text
return Optional.empty();
}
if (!expressions.stream().map(ConnectorExpression::getType).allMatch(typePredicate)) {
return Optional.empty();
}

ImmutableList.Builder<QueryParameter> parameters = ImmutableList.builder();
parameters.addAll(value.get().parameters());
Expand Down
13 changes: 13 additions & 0 deletions plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,15 @@
import io.trino.plugin.jdbc.aggregation.ImplementSum;
import io.trino.plugin.jdbc.expression.JdbcConnectorExpressionRewriterBuilder;
import io.trino.plugin.jdbc.expression.ParameterizedExpression;
import io.trino.plugin.jdbc.expression.RewriteIn;
import io.trino.plugin.jdbc.logging.RemoteQueryModifier;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.AggregateFunction;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.connector.ConnectorSession;
import io.trino.spi.connector.ConnectorTableMetadata;
import io.trino.spi.expression.ConnectorExpression;
import io.trino.spi.type.CharType;
import io.trino.spi.type.DecimalType;
import io.trino.spi.type.Decimals;
Expand Down Expand Up @@ -224,6 +226,11 @@ public ClickHouseClient(
JdbcTypeHandle bigintTypeHandle = new JdbcTypeHandle(Types.BIGINT, Optional.of("bigint"), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty());
this.connectorExpressionRewriter = JdbcConnectorExpressionRewriterBuilder.newBuilder()
.addStandardRules(this::quoted)
.map("$equal(left: varchar, right: varchar)").to("left = right")
.add(new RewriteIn(type -> type instanceof VarcharType))
.map("$like(value: varchar, pattern: varchar): boolean").to("value LIKE pattern")
.map("$is_null(value: varchar)").to("value IS NULL")
.map("$nullif(first: varchar, second: varchar)").to("NULLIF(first, second)")
.build();
this.aggregateFunctionRewriter = new AggregateFunctionRewriter<>(
this.connectorExpressionRewriter,
Expand All @@ -248,6 +255,12 @@ public Optional<JdbcExpression> implementAggregation(ConnectorSession session, A
return aggregateFunctionRewriter.rewrite(session, aggregate, assignments);
}

@Override
public Optional<ParameterizedExpression> convertPredicate(ConnectorSession session, ConnectorExpression expression, Map<String, ColumnHandle> assignments)
{
return connectorExpressionRewriter.rewrite(session, expression, assignments);
}

@Override
public boolean supportsTopN(ConnectorSession session, JdbcTableHandle handle, List<JdbcSortItem> sortOrder)
{
Expand Down
101 changes: 101 additions & 0 deletions ...rino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
SUPPORTS_AGGREGATION_PUSHDOWN_CORRELATION,
SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_EQUALITY,
SUPPORTS_TOPN_PUSHDOWN,
SUPPORTS_PREDICATE_EXPRESSION_PUSHDOWN,
SUPPORTS_PREDICATE_EXPRESSION_PUSHDOWN_WITH_LIKE,
SUPPORTS_TRUNCATE -> true;
case SUPPORTS_AGGREGATION_PUSHDOWN_REGRESSION,
SUPPORTS_AGGREGATION_PUSHDOWN_STDDEV,
Expand All @@ -74,6 +76,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
SUPPORTS_DELETE,
SUPPORTS_DROP_NOT_NULL_CONSTRAINT,
SUPPORTS_NEGATIVE_DATE,
SUPPORTS_PREDICATE_ARITHMETIC_EXPRESSION_PUSHDOWN,
SUPPORTS_ROW_TYPE,
SUPPORTS_SET_COLUMN_TYPE,
SUPPORTS_UPDATE -> false;
Expand Down Expand Up @@ -903,6 +906,104 @@ public void testTextualPredicatePushdown()
.isFullyPushedDown();
}

@Test
public void testOrPredicatePushdown()
{
assertThat(query("SELECT * FROM nation WHERE name = 'ALGERIA' OR comment = 'comment'")).isFullyPushedDown();
assertThat(query("SELECT * FROM nation WHERE name IS NULL OR comment IS NULL")).isFullyPushedDown();
}

@Test
public void testLikePredicatePushdown()
{
assertThat(query("SELECT nationkey FROM nation WHERE name LIKE '%A%'"))
.isFullyPushedDown();

try (TestTable table = new TestTable(
getQueryRunner()::execute,
"test_like_predicate_pushdown",
"(id integer, a_varchar varchar)",
List.of(
"1, 'A'",
"2, 'a'",
"3, 'B'",
"4, 'ą'",
"5, 'Ą'"))) {
assertThat(query("SELECT id FROM " + table.getName() + " WHERE a_varchar LIKE '%A%'"))
.isFullyPushedDown();
assertThat(query("SELECT id FROM " + table.getName() + " WHERE a_varchar LIKE '%ą%'"))
.isFullyPushedDown();
}
}

@Test
public void testIsNullPredicatePushdown()
{
assertThat(query("SELECT nationkey FROM nation WHERE name IS NULL")).isFullyPushedDown();
assertThat(query("SELECT nationkey FROM nation WHERE name IS NULL OR comment = 'comment'")).isFullyPushedDown();

try (TestTable table = new TestTable(
getQueryRunner()::execute,
"test_is_null_predicate_pushdown",
"(a_int integer, a_varchar varchar(1), a_varchar2 varchar(1))",
List.of(
"1, 'A', ''",
"2, 'B', NULL",
"1, NULL, 'C'",
"2, NULL, 'D'"))) {
assertThat(query("SELECT a_int FROM " + table.getName() + " WHERE a_varchar IS NULL")).isFullyPushedDown();
assertThat(query("SELECT a_int FROM " + table.getName() + " WHERE a_varchar IS NULL OR a_varchar2 = 'D'")).isFullyPushedDown();
}
}

@Test
public void testNullIfPredicatePushdown()
{
assertThat(query("SELECT nationkey FROM nation WHERE NULLIF(name, 'ALGERIA') IS NULL"))
.matches("VALUES BIGINT '0'")
.isFullyPushedDown();

assertThat(query("SELECT nationkey FROM nation WHERE NULLIF(name, 'Algeria') IS NULL"))
.returnsEmptyResult()
.isFullyPushedDown();

// NULLIF returns the first argument because arguments aren't the same
assertThat(query("SELECT nationkey FROM nation WHERE NULLIF(name, 'Name not found') = name"))
.matches("SELECT nationkey FROM nation")
.isFullyPushedDown();
}

@Test
public void testInPredicatePushdown()
{
try (TestTable table = new TestTable(
getQueryRunner()::execute,
"test_in_predicate_pushdown",
"(id varchar(1), id2 varchar(1), id3 double)",
List.of(
"'a', 'b', 1",
"'b', 'c', 2",
"'c', 'c', 3",
"'d', 'd', 4",
"'a', 'f', 5"))) {
// IN pushdowns only varchar
assertThat(query(
"SELECT id3 FROM " + table.getName() + " WHERE id3 IN (1, 2, 3, 4, 5) or id IN ('a', 'B')"))
.isNotFullyPushedDown(FilterNode.class);

// IN values cannot be represented as a domain
assertThat(query("SELECT id FROM " + table.getName() + " WHERE id IN ('a', 'b') OR id2 IN ('c', 'd')"))
.isFullyPushedDown();

assertThat(query("SELECT id FROM " + table.getName() + " WHERE id IN ('a', 'B') OR id2 IN ('c', 'D')"))
.isFullyPushedDown();

assertThat(query("SELECT id FROM " + table.getName() + " WHERE id IN ('a', 'B', NULL) OR id2 IN ('C', 'd')"))
// NULL constant value is currently not pushed down
.isNotFullyPushedDown(FilterNode.class);
}
}

@Test
@Override // Override because ClickHouse doesn't follow SQL standard syntax
public void testExecuteProcedure()
Expand Down