Improve IS NULL pushdown for ClickHouse complex expression #23459

ssheikin · 2024-09-17T13:28:11Z

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
() Release notes are required. Please propose a release note for me.
(x ) Release notes are required, with the following suggested text:

# Section
* Improve IS NULL pushdown for ClickHouse complex expression. ({issue}`23459`)

ssheikin · 2024-09-27T22:45:12Z

@Praveen2112 @hashhar @ebyhr please review.

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

krvikash

Overall lgtm.

krvikash · 2024-10-03T05:47:51Z

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

+    {
+        checkState(!testCases.isEmpty(), "No test cases");
+        for (int specialColumn = 0; specialColumn < SPECIAL_COLUMNS; specialColumn++) {
+            checkArgument(!"NULL".equalsIgnoreCase(testCases.get(specialColumn).inputLiteral()));


What we are verifying here actually? It is verifying for only first test case that inputLiteral should not be null.
It seems that before adding the round trip test case, we should be knowing that 1st test case should not have NULL as input literal.

After going through the code, I got the intention of why we needed SPECIAL_COLUMNS. May be NON_NULL_COLUMNS makes more sense here. WDYT?

I had TestClickHouseConnectorTest#testTextualPredicatePushdown in mind, where SPECIAL_COLUMNS are

unsupported_1 Point, unsupported_2 Point, some_column String,

What if we could handle this special columns as part of this framework ?

I think it's possible however I'm not sure how to make it look better that it is right now, because the type of the special columns may differ for different cases:

.addTestCase("String", "'z'", VARCHAR, "CAST('z' AS varchar)") // special, non null column .addTestCase("String", "'z'", VARBINARY, "CAST('z' AS varbinary)") // special, non null column

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

Praveen2112 · 2024-10-11T15:00:14Z

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

+                + withConnectorExpression;
+
+        // Closing QueryAssertions would close the QueryRunner
+        QueryAssertions queryAssertions = new QueryAssertions(queryRunner);


Can we inject queryAssertions instead of query runner ?

It conforms to the api of SqlDataTypeTest if we later want to unify these assertions.

Also not sure if it gives benefits.

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

Praveen2112 · 2024-10-11T15:29:53Z

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

+    {
+        checkState(!testCases.isEmpty(), "No test cases");
+        for (int specialColumn = 0; specialColumn < SPECIAL_COLUMNS; specialColumn++) {
+            checkArgument(!"NULL".equalsIgnoreCase(testCases.get(specialColumn).inputLiteral()));


What if we could handle this special columns as part of this framework ?

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java

ssheikin · 2024-10-16T08:59:09Z

@Praveen2112 please approve and merge.

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

Praveen2112 · 2024-10-16T10:49:12Z

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/NullPushdownDataTypeTest.java

+        try {
+            assertPushdown(expectPushdown,
+                    assertResult(isNull ? Optional.of(firstCase.expectedLiteral()) : Optional.empty(),
+                            assertThat(queryAssertions.query(session, queryWithAll))));
+        }
+        catch (AssertionError e) {
+            // if failed - identify exact column which caused the failure
+            for (int column = SPECIAL_COLUMNS; column < testCases.size(); column++) {
+                String queryWithSingleColumnPredicate = "SELECT " + firstColumnName + " FROM " + temporaryRelation.getName() + " WHERE " + getPredicate(column, isNull) + withConnectorExpression;
+                assertPushdown(expectPushdown,
+                        assertResult(isNull ? Optional.of(firstCase.expectedLiteral()) : Optional.empty(),
+                                assertThat(queryAssertions.query(session, queryWithSingleColumnPredicate))));
+            }
+            throw new IllegalStateException("Single column assertion should fail for at least one column, if query of all column failed", e);
+        }
+    }
+


Instead of asserting them twice like once with all columns AND ? Can we assert them each column at a time ? Like

for (int column = SPECIAL_COLUMNS; column < testCases.size(); column++) { String queryWithSingleColumnPredicate = "SELECT " + firstColumnName + " FROM " + temporaryRelation.getName() + " WHERE " + getPredicate(column, isNull) + withConnectorExpression; assertPushdown(expectPushdown, assertResult(isNull ? Optional.of(firstCase.expectedLiteral()) : Optional.empty(), assertThat(queryAssertions.query(session, queryWithSingleColumnPredicate)))); }

This would allow us to identify the specific columns which would cause failure in the future

Not sure if I follow your suggestion. We already asserting each column at a time, in case of failure.

Instead of handling twice can we try asserting only once for each column so that it would be easier to debug in case of failures

The first check for all columns is N times faster than the second for single columns. So it's an improvement for sunny-day scenario.
In case of failure there is overhead of 1 additional check, performing N additional checks for single columns.
This was copied from SqlDataTypeTest.
Please let me know if you want to simplify code.

Makes sense !! Let's have it like this.

Praveen2112 · 2024-10-16T10:51:55Z

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/NullPushdownDataTypeTest.java

+    private String getPredicate(int column, boolean isNull)
+    {
+        checkArgument(column >= SPECIAL_COLUMNS, "Special columns should not be a part of a predicate, as they are helpers and do not participate in the assertions");
+        String columnName = "col_" + column;
+        checkArgument("NULL".equalsIgnoreCase(testCases.get(column).inputLiteral()));
+        return isNull
+                ? columnName + " IS NULL"
+                : columnName + " IS NOT NULL";
+    }
+
+    private static QueryAssert assertResult(Optional<String> value, QueryAssert assertion)
+    {
+        return value.isPresent()
+                ? assertion.matches("VALUES %s".formatted(value.get()))
+                : assertion.returnsEmptyResult();
+    }


If we could inline IS NULL and IS NOT NULL and the assertion could be inlined right ? Ideally for the test data - we for IS NULL we get a value and for IS NOT NULL we get an empty value - Do we need this dedicated method for each assertion.

Not sure if I follow. I'm ok with any solution, just to merge this PR.

ssheikin · 2024-10-16T14:49:17Z

@Praveen2112 do you think we are good to merge this PR?
It has a nice score of modified lines:

Praveen2112

LGTM

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/NullPushdownDataTypeTest.java

ssheikin · 2024-10-17T13:18:43Z

@Praveen2112 ptal

Praveen2112

Thanks for working on this

Praveen2112 · 2024-10-17T13:29:16Z

@ssheikin Can we update the PR description.

cla-bot bot added the cla-signed label Sep 17, 2024

ssheikin requested review from Praveen2112 and hashhar September 17, 2024 13:29

ssheikin mentioned this pull request Sep 17, 2024

Running branch for Clickhouse pushdowns #23460

Open

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 7e00f41 to 4195edb Compare September 20, 2024 20:47

ssheikin requested review from ebyhr, sylph-eu and raunaqmorarka September 20, 2024 20:48

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 4195edb to c0d94e2 Compare September 21, 2024 15:11

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from c0d94e2 to 6be9498 Compare September 27, 2024 22:44

ssheikin changed the title ~~Pushdown NOT IN, IS NOT NULL for ClickHouse~~ Improve IS NULL pushdown for ClickHouse complex expression Sep 27, 2024

Praveen2112 reviewed Sep 30, 2024

View reviewed changes

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java Outdated Show resolved Hide resolved

krvikash reviewed Oct 1, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch 2 times, most recently from 5982fe3 to a8c8d0c Compare October 1, 2024 14:00

krvikash reviewed Oct 3, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch 2 times, most recently from 9d6bf97 to 48e2843 Compare October 3, 2024 10:35

krvikash approved these changes Oct 3, 2024

View reviewed changes

...in/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/IsNullPushdownDataTypeTest.java Outdated Show resolved Hide resolved

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 48e2843 to 6c17091 Compare October 3, 2024 12:21

krvikash approved these changes Oct 3, 2024

View reviewed changes

Praveen2112 reviewed Oct 11, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 6c17091 to 15d804f Compare October 16, 2024 08:57

Praveen2112 reviewed Oct 16, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 15d804f to 2360265 Compare October 16, 2024 14:05

Praveen2112 reviewed Oct 17, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 2360265 to 2ed946e Compare October 17, 2024 13:16

Praveen2112 approved these changes Oct 17, 2024

View reviewed changes

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch 3 times, most recently from b8c37ac to 0a81434 Compare October 17, 2024 14:37

Improve IS NULL pushdown for ClickHouse complex expression

cd7ddc4

ssheikin force-pushed the ssheikin/54/trino/clickhouse-pushdown-predicate-not-null branch from 0a81434 to cd7ddc4 Compare October 17, 2024 14:42

Praveen2112 merged commit 5936d81 into trinodb:master Oct 17, 2024
16 checks passed

github-actions bot added this to the 463 milestone Oct 17, 2024

mosabua mentioned this pull request Oct 17, 2024

Add Trino 463 release notes #23810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve IS NULL pushdown for ClickHouse complex expression #23459

Improve IS NULL pushdown for ClickHouse complex expression #23459

ssheikin commented Sep 17, 2024 •

edited

Loading

ssheikin commented Sep 27, 2024

krvikash left a comment

krvikash Oct 3, 2024

krvikash Oct 3, 2024

ssheikin Oct 3, 2024

Praveen2112 Oct 11, 2024

ssheikin Oct 16, 2024

Praveen2112 Oct 11, 2024

ssheikin Oct 16, 2024

ssheikin Oct 16, 2024

Praveen2112 Oct 11, 2024

ssheikin commented Oct 16, 2024

Praveen2112 Oct 16, 2024

ssheikin Oct 16, 2024

Praveen2112 Oct 16, 2024

ssheikin Oct 17, 2024

Praveen2112 Oct 17, 2024

Praveen2112 Oct 16, 2024

ssheikin Oct 16, 2024

ssheikin commented Oct 16, 2024 •

edited

Loading

Praveen2112 left a comment

ssheikin commented Oct 17, 2024

Praveen2112 left a comment

Praveen2112 commented Oct 17, 2024

Improve IS NULL pushdown for ClickHouse complex expression #23459

Improve IS NULL pushdown for ClickHouse complex expression #23459

Conversation

ssheikin commented Sep 17, 2024 • edited Loading

Description

Additional context and related issues

Release notes

ssheikin commented Sep 27, 2024

krvikash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Oct 16, 2024 • edited Loading

Praveen2112 left a comment

Choose a reason for hiding this comment

ssheikin commented Oct 17, 2024

Praveen2112 left a comment

Choose a reason for hiding this comment

Praveen2112 commented Oct 17, 2024

ssheikin commented Sep 17, 2024 •

edited

Loading

ssheikin commented Oct 16, 2024 •

edited

Loading