Allow to enable case insenstive predicate pushdown in JDBC connectors #7022

kokosing · 2021-02-24T13:29:06Z

Allow to enable case insenstive predicate pushdown in JDBC connectors

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataConfig.java

findepi · 2021-02-24T13:49:02Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataConfig.java

+        return caseInsensitivePredicatePushdownEnabled;
+    }
+
+    @Config("case-insensitive-predicate-pushdown.enabled")


Consider the case where someone sets their MySQL instance to use case sensitive collation by default and they want to leverage the full pushdown capabilities. In such a situation, the "insensitive" in the flag name is not reasonable, as this is exactly what they are not doing.

Maybe unsafe-varchar-predicate-pushdown.enabled ?

cc @losipiuk @electrum

Hm... in case of PostgreSQL or other case sensitive database one case change collation to use case insensitive comparisons. Then we might need to do something reverse to match the remote database collation.

How about varchar-predicate-pushdown.mode with values FULL, PUSHDOWN_AND_KEEP and CASE_INSENSITIVE? Then depending on connector we could have different defaults. WDYT?

That's an option, but IMO exposes too much internal detail, while being also pretty technical and I am not yet convinced it's the right way.
Also, why would we have the toggle for a connector which doesn't need it, because remote has just one collation?

ok. I did it your way.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java

findepi · 2021-02-24T13:52:34Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/PredicatePushdownController.java

    PredicatePushdownController CASE_INSENSITIVE_CHARACTER_PUSHDOWN = (session, domain) -> {
        checkArgument(
                domain.getType() instanceof VarcharType || domain.getType() instanceof CharType,
                "CASE_SENSITIVE_PUSHDOWN can be used only for chars and varchars");

+        if (isCaseInsensitivePredicatePushdownEnabled(session)) {
+            return PUSHDOWN_AND_KEEP.apply(session, domain);


or FULL_PUSHDOWN. I see one could argue both ways.

maybe add a comment and we may need to revisit this later, to unlock e.g. agg or join pushdown on top of varchar columns

I think it is easier for understand for user if we FULL_PUSHDOWN here, otherwise he might say that pushdown does not work

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataConfig.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/PredicatePushdownController.java

ksobolew · 2021-02-24T14:08:27Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java

@@ -46,6 +47,11 @@ public JdbcMetadataSessionProperties(JdbcMetadataConfig jdbcMetadataConfig, @Max
                        "Enable aggregation pushdown",
                        jdbcMetadataConfig.isAggregationPushdownEnabled(),
                        false))
+                .add(booleanProperty(
+                        CASE_INSENSTIVE_PREDICATE_PUSHDOWN_ENABLED,
+                        "Enable case insensitive predicate pushdown",


I'm wondering if this is a good description for this option. As I understand it, pushdown is disabled on character columns (for all operators?), on databases which are doing case insensitive string comparison by default, right? Maybe something "Always do potentially unsafe (var)char predicate pushdown on databases which do case insensitive string comparison"?

Also, this is a tricky setting, or perhaps I should say "esoteric", so can it be documented somewhere is better detail?

As I understand it, pushdown is disabled on character columns (for all operators?), on databases which are doing case insensitive string comparison by default, right?

It is more complicated than that. By default we pushdown a = 'value' predicate when remote data source is doing case insensitive string comparison by default and we also keep the predicate on Trino side.

I updated the message, please take a look.

It's OK now, though one could argue that "potentially unsafe" already implies "may cause incorrect results"

kokosing

Comments, addressed.

kokosing · 2021-03-01T11:39:56Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataConfig.java

+        return caseInsensitivePredicatePushdownEnabled;
+    }
+
+    @Config("case-insensitive-predicate-pushdown.enabled")


ok. I did it your way.

kokosing · 2021-03-01T11:42:59Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java

@@ -46,6 +47,11 @@ public JdbcMetadataSessionProperties(JdbcMetadataConfig jdbcMetadataConfig, @Max
                        "Enable aggregation pushdown",
                        jdbcMetadataConfig.isAggregationPushdownEnabled(),
                        false))
+                .add(booleanProperty(
+                        CASE_INSENSTIVE_PREDICATE_PUSHDOWN_ENABLED,
+                        "Enable case insensitive predicate pushdown",


As I understand it, pushdown is disabled on character columns (for all operators?), on databases which are doing case insensitive string comparison by default, right?

It is more complicated than that. By default we pushdown a = 'value' predicate when remote data source is doing case insensitive string comparison by default and we also keep the predicate on Trino side.

I updated the message, please take a look.

kokosing · 2021-03-01T11:43:46Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/PredicatePushdownController.java

    PredicatePushdownController CASE_INSENSITIVE_CHARACTER_PUSHDOWN = (session, domain) -> {
        checkArgument(
                domain.getType() instanceof VarcharType || domain.getType() instanceof CharType,
                "CASE_SENSITIVE_PUSHDOWN can be used only for chars and varchars");

+        if (isCaseInsensitivePredicatePushdownEnabled(session)) {
+            return PUSHDOWN_AND_KEEP.apply(session, domain);


I think it is easier for understand for user if we FULL_PUSHDOWN here, otherwise he might say that pushdown does not work

findepi · 2021-03-01T12:25:02Z

Since PostgreSQL collation is just different (although case sensitive by default), i am not sure there is a point in treating PostgreSQL predicate pushdown from MySQL's or SQL Server's.

That would allow us to remove io.trino.plugin.postgresql.TestPostgreSqlConnectorTest#testCaseSensitiveDataMapping override.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/VarcharPredicatePushdownConfig.java

...-base-jdbc/src/main/java/io/trino/plugin/jdbc/VarcharPredicatePushdownSessionProperties.java

findepi · 2021-03-01T12:27:32Z

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

@@ -735,6 +735,7 @@ public void testCaseSensitiveDataMapping(DataMappingTestSetup dataMappingTestSet
        throw new SkipException("PostgreSQL has different collation than Trino");
    }

+


findepi · 2021-03-01T12:28:20Z

...ino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

@@ -599,6 +600,40 @@ private String getLongInClause(int start, int length)
        return "orderkey IN (" + longValues + ")";
    }

+    @Test(dataProvider = "testCaseInsensitivePredicatePushdownDataProvider")
+    public void testCaseInsensitivePredicatePushdown(String type)


BCT candidate?

Or we need BaseJdbcConnectorTest for cases like this?

BCT candidate?

Sure these tests should be ported, but it is out of the scope of this PR.

Or we need BaseJdbcConnectorTest for cases like this?

Not yet. This feature only exists in few JDBC connectors. So I think it is too specific to have generic tests.

Sure these tests should be ported, but it is out of the scope of this PR.

sure, let's have an issue

we also need something generic for TopN pushdows, see #7031

kokosing · 2021-03-02T12:16:23Z

Since PostgreSQL collation is just different (although case sensitive by default), i am not sure there is a point in treating PostgreSQL predicate pushdown from MySQL's or SQL Server's.

That is in line with my thinking. I just wanted to keep the scope of this PR limited and PostgreSQL is next in line in my queue. I am wondering if it is possible to do anything better for PostgreSQL, for example != should work with PUSHDOWN_AND_KEEP there while it does not for MySQL and co.

findepi · 2021-03-02T13:28:35Z

I am wondering if it is possible to do anything better for PostgreSQL, for example != should work with PUSHDOWN_AND_KEEP there while it does not for MySQL and co.

Good point, i didn't think about this.
I am not sure it's worthwhile to keep separate impls just for != case.

findepi · 2021-03-03T22:40:02Z

The PostgreSQL case is changing in #7145.
We need to somehow make sure that place gets updated too.

findepi · 2021-03-05T09:46:25Z

The PostgreSQL case is changing in #7145.
We need to somehow make sure that place gets updated too.

Let's automate this.

Can you please add a test in BaseJdbcConnectorTest that verifies that:

either varchar = and < predicate is pushed down into the connector (with correct results)
OR session toggle CASE_INSENSTIVE_PREDICATE_PUSHDOWN_ENABLED is supported and results in filter being fully pushed down (ignoring result correctness)?

If we test this on a table where cases doesn't matter (eg nation.name), we can express this easily.

kokosing · 2021-03-08T10:01:35Z

PTAL

Adding a test to BaseJdbcConnectorTest requires me to run tests for all JDBC connectors. I run just few, so there might be some issues still.

findepi · 2021-03-08T11:16:20Z

Can you please update PostgreSqlClient as well?

kokosing · 2021-03-08T11:43:37Z

Can you please update PostgreSqlClient as well?

I did. This one is tricky, it compares values differently. So I no longer validate incorrect results, as they are different for PostgreSQL than for MySQL.

findepi · 2021-03-08T11:45:26Z

You still can validate incorrect results in a case like b vs A, if you want to.

kokosing · 2021-03-08T12:00:20Z

You still can validate incorrect results in a case like b vs A, if you want to.

I think such assertion is overly complicated. Let's leave it as it is.

hashhar · 2021-03-08T12:27:41Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+                .isNotFullyPushedDown(FilterNode.class)
+                .skippingTypesCheck()
+                .matches("VALUES 'A', 'b', 'B'");
+        assertThat(query(format("SELECT value FROM %s WHERE value > 'A'", tableName)))


I would prefer if you tested that ordering is correct too.

Some databases/collations can sort A, a, B, b or a, b, A, B or A, B, a, b.
Your test would pass for collations that sort as A, B, a, b too when I think it should fail.

Or am I broadening the scope by introducing ordering with predicate pushdown?

Or am I broadening the scope by introducing ordering with predicate pushdown?

You are. However it sounds like a good idea to have a single toggle to control both pushdown at once.

BTW same goes to aggregation push down. Notice that pushing down DISTINCT or GROUP BY on varchar column may produce different results. Is it handled today anyhow? It sounds to me that it could be also controlled with the same switch.

@findepi ?

It should, but we also need a reusable test for aggregations as well.

Reusable tests for order by and aggregations are out of my scope. Will create tickets for that, possibly as bugs as I would expect now wrong results. Also I am going to rename toggle from unsafe-varchar-predicate-pushdown.enabled to unsafe-varchar-pushdown.enabled.

#7320, order by is handled by io.trino.plugin.jdbc.BaseJdbcConnectorTest#testCaseSensitiveTopNPushdown

kokosing

PTAL

kokosing · 2021-03-17T12:41:30Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+                .isNotFullyPushedDown(FilterNode.class)
+                .skippingTypesCheck()
+                .matches("VALUES 'A', 'b', 'B'");
+        assertThat(query(format("SELECT value FROM %s WHERE value > 'A'", tableName)))


#7320, order by is handled by io.trino.plugin.jdbc.BaseJdbcConnectorTest#testCaseSensitiveTopNPushdown

kokosing · 2021-03-31T11:46:27Z

@findepi ping

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

findepi · 2021-04-02T20:53:55Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+                // possibly incorrect results with unexpected 'A'
+                .isFullyPushedDown();


I don't understand. isFullyPushedDown verifies results correctness, this shouldn't pass.

That is because allow_pushdown_into_connectors does not control predicate pushdown.

findepi · 2021-04-02T20:54:58Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java

+        assertThat(query(format("SELECT value FROM %s WHERE value != 'a'", tableName)))
+                .skippingTypesCheck()
+                .matches("VALUES 'A', 'b', 'B'")
+                .isNotFullyPushedDown(FilterNode.class);


For postgres we can pushdown != but cannot pushdown >.

Let's use order-related (<, >, <=, etc.) here

This is added below.

hashhar · 2021-04-04T15:05:52Z

FYI Postgres has a solution available which I've started implementing in #7494.

kokosing · 2021-04-04T18:20:57Z

FYI Postgres has a solution available which I've started implementing in #7494.

If it is a case then I think I am going to close this PR. It is better to have correct predicate pushdown instead work arounds. Same option exists for MySQL (https://dev.mysql.com/doc/refman/8.0/en/case-sensitivity.html) and in SqlServer.

hashhar · 2021-04-04T20:32:36Z

@kokosing It would be useful to keep this branch around because from my quick attempt today it looks like the query generation for predicate pushdown is done within the engine itself (QueryBuilder) and connectors don't have any control over the syntax/clauses being used. I'll take a deeper look again though.

Thanks for the info about MySQL and SQLServer - I'll incorporate them in my PR too.

findepi · 2021-04-07T10:27:21Z

FYI Postgres has a solution available

That's not a news to me

which I've started implementing in #7494.

that's definitely new.

it will take time to deliver this though, and may or may not be possible for all connectors where we had to disable varchar pushdown by default.

therefore i do think this PR is still valuable

cla-bot bot added the cla-signed label Feb 24, 2021

kokosing requested review from findepi, ssheikin and ksobolew February 24, 2021 13:29

findepi reviewed Feb 24, 2021

View reviewed changes

ssheikin approved these changes Feb 24, 2021

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadataSessionProperties.java Outdated Show resolved Hide resolved

ksobolew reviewed Feb 24, 2021

View reviewed changes

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from fcd75e1 to 7614d5a Compare March 1, 2021 12:07

kokosing commented Mar 1, 2021

View reviewed changes

findepi reviewed Mar 1, 2021

View reviewed changes

findepi mentioned this pull request Mar 5, 2021

LIKE predicate pushdown #1580

Open

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 7614d5a to 87ea72e Compare March 8, 2021 10:00

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 87ea72e to 364eac3 Compare March 8, 2021 11:43

hashhar requested changes Mar 8, 2021

View reviewed changes

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 364eac3 to 3bb8248 Compare March 17, 2021 12:44

kokosing commented Mar 17, 2021

View reviewed changes

findepi mentioned this pull request Mar 17, 2021

Support TopN pushdown in MySQL and MemSQL #7326

Merged

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 3bb8248 to 355c292 Compare March 21, 2021 20:12

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 355c292 to 7b63c5f Compare March 31, 2021 11:45

findepi reviewed Apr 2, 2021

View reviewed changes

kokosing added 2 commits April 3, 2021 09:02

Extract PUSHDOWN_AND_KEEP

f48a72f

Allow to enable case insenstive predicate pushdown in JDBC connectors

acc1cbe

kokosing force-pushed the origin/master/284_unsafe_pushdown branch from 7b63c5f to acc1cbe Compare April 3, 2021 07:02

hashhar mentioned this pull request Apr 4, 2021

Support TopN pushdown for text types in PostgreSQL #7494

Merged

kokosing closed this Apr 4, 2021

kokosing deleted the origin/master/284_unsafe_pushdown branch April 4, 2021 18:21

hashhar mentioned this pull request Apr 5, 2021

Support predicate pushdown for text types for connectors whose default collation differs from Trino #7496

Open

kokosing restored the origin/master/284_unsafe_pushdown branch April 7, 2021 10:31

kokosing reopened this Apr 7, 2021

hashhar mentioned this pull request Jul 19, 2021

Fix incorrect results for aggregation functions on case-sensitive types #8551

Merged

kokosing closed this Jul 26, 2021

kokosing deleted the origin/master/284_unsafe_pushdown branch July 26, 2021 09:41

		@@ -735,6 +735,7 @@ public void testCaseSensitiveDataMapping(DataMappingTestSetup dataMappingTestSet
		throw new SkipException("PostgreSQL has different collation than Trino");
		}

		// possibly incorrect results with unexpected 'A'
		.isFullyPushedDown();

Allow to enable case insenstive predicate pushdown in JDBC connectors #7022

Allow to enable case insenstive predicate pushdown in JDBC connectors #7022

Conversation

kokosing commented Feb 24, 2021

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

kokosing 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

findepi commented Mar 1, 2021

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

kokosing commented Mar 2, 2021

findepi commented Mar 2, 2021

findepi commented Mar 3, 2021

findepi commented Mar 5, 2021

kokosing commented Mar 8, 2021

findepi commented Mar 8, 2021

kokosing commented Mar 8, 2021

findepi commented Mar 8, 2021

kokosing commented Mar 8, 2021

hashhar Mar 8, 2021 • edited Loading

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

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Mar 31, 2021

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

hashhar commented Apr 4, 2021

kokosing commented Apr 4, 2021

hashhar commented Apr 4, 2021 • edited Loading

findepi commented Apr 7, 2021

hashhar Mar 8, 2021 •

edited

Loading

hashhar commented Apr 4, 2021 •

edited

Loading