Support range predicate pushdown for string columns with collation in PostgreSQL connector

[takezoe]

No description provided.

[kokosing]

Please update io.trino.plugin.postgresql.TestPostgreSqlConnectorTest#hasBehavior to support SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_INEQUALITY

[takezoe]

Updated.

[hashhar]

The impl looks good. Please adjust the declaration in tests to make sure the tests pass with the new assumption.

[takezoe]

Additional updates are necessary to support aggregation pushdown for varchar columns. Since I would focus on support range predicates pushdown in this pull request, I just introduced a new behavior that indicates whether aggregation pushdown is supported for varchar columns.

[hashhar]

Let's create a GitHub issue and add a TODO so that we can remove the additional behaviour once it's no longer needed (and other people know that this is something they can work on).

[takezoe]

Yeah, that makes sense (as long as this fix can be merged) and I would work on it once this pull request is completed. By the way, supporting type sensitive aggregation pushdown in JDBC plugins doesn't seem easy. Fundamental interface changes may be required.

[hashhar]

Yeah, I ran into some similar issues in #7320.

There were some ideas floated in #7320 (comment) (which we didn't end up doing since it looked like a one-off need at that time).

If you already have some direction in your mind it might be helpful to discuss it on #dev on Slack too (if you think the changes will be large and touch the SPI).

[takezoe]

Oh, will take a look at #7320 first. Thanks.

[takezoe]

TestPostgreSqlConnectorTest#testSystemTable() is still failing with the following error:

io.trino.spi.TrinoException: ERROR: collations are not supported by type name

I'm trying to find a way to fix it.

[takezoe]

Looks like collations have been supported by name type since PostgreSQL 12. I would allow pushdown only for column types that isCollatable() returns true for now.

[takezoe]

@kokosing @hashhar Could you take a look again?

[findepi]

why still isNotFullyPushedDown?

[takezoe]

With the domain compaction, the original predicate seems to remain on Trino side although the compacted predicate is pushed down to PostgreSQL:

SQL on PostgreSQL:

SELECT "nationkey", "name", "regionkey" FROM "tpch"."nation" WHERE ("name" >= ? COLLATE "C" AND "name" <= ? COLLATE "C")

Plan on Trino:

Output[regionkey, nationkey, name]
│   Layout: [regionkey:bigint, nationkey:bigint, name:varchar(25)]
└─ RemoteExchange[GATHER]
   │   Layout: [nationkey:bigint, name:varchar(25), regionkey:bigint]
   └─ ScanFilter[table = postgresql:tpch.nation tpch.nation constraint on [name] columns=[nationkey:bigint:int8, name:varchar(25):varchar, regionkey:bigint:int8], filterPredicate = ("name" IN (CAST('POLAND' AS varchar(25)), CAST('ROMANIA' AS varchar(25)), CAST('VIETNAM' AS varchar(25))))]
          Layout: [nationkey:bigint, name:varchar(25), regionkey:bigint]
          nationkey := nationkey:bigint:int8
          regionkey := regionkey:bigint:int8
          name := name:varchar(25):varchar

[hashhar]

Let's create a GitHub issue and add a TODO so that we can remove the additional behaviour once it's no longer needed (and other people know that this is something they can work on).

[takezoe]

@findepi Finished updating for now. Could you take a look again?

[hashhar]

Some minor comments.

Can we detect (or apply some heuristics) to determine during runtime whether a given predicate needs the COLLATION applied or not? For example for equality predicates adding the collation will lead to a definite performance regression because no indexes can be used anymore.

If it's not possible to do dynamically during runtime then let's add an experimental. prefixed config property to enable this behaviour and keep it opt in for now?

WDYT @wendigo ?

[takezoe]

For example for equality predicates adding the collation will lead to a definite performance regression because no indexes can be used anymore.

I think it's possible in some cases but 100% is impossible. But anyway it should be better than full-scan?

If it's not possible to do dynamically during runtime then let's add an experimental. prefixed config property to enable this behaviour and keep it opt in for now?

I'm fine to make this optional as long as we can enable this by configuration. Is adding experimental.postgresql.enable-string-pushdown-with-collate ok?

[hashhar]

For example for equality predicates adding the collation will lead to a definite performance regression because no indexes can be used anymore.

I think it's possible in some cases but 100% is impossible. But anyway it should be better than full-scan?

If it's not possible to do dynamically during runtime then let's add an experimental. prefixed config property to enable this behaviour and keep it opt in for now?

I'm fine to make this optional as long as we can enable this by configuration. Is adding experimental.postgresql.enable-string-pushdown-with-collate ok?

@takezoe You can add a session property with matching config property in PostgreSqlConfig. I think experimental.postgresql.string-pushdown-with-collate is fine.

cc: @findepi @kokosing @wendigo @ebyhr Any opinions? I'd prefer to have this behind a configuration toggle (at-least for now) until we can verify that there isn't a performance concern in actual practical usage?

[hashhar]

Looks good % confirmation about config from other maintainers.

[takezoe]

Looks like postgresql.experimental.string-pushdown-with-collate is more consistent because PostgreSqlConfig used to have another experimental setting postgresql.experimental.array-mapping:

trino/plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlConfig.java

Line 40 in f7aac0d

@LegacyConfig("postgresql.experimental.array-mapping")

[hashhar]

LGTM % comments.

Mostly about possible simplifications.

[takezoe]

@hashhar Finished updating the pull request. Could you take a look again?

[hashhar]

Looks good to me % comment. Thanks for working on this.

Also it looks the first two commits should be squashed together.

[hashhar]

LGTM % minor testing change.

Thanks for working on this @takezoe. You might also be interested in #10059 which we plan to use as the base for allowing join pushdown too using COLLATE.

[takezoe]

Looks like hasBehavior() has to behave differently depending on whether enable_string_pushdown_with_collate is true or false. And it seems annoying to cover by a single test class.

For example, having a flag as a field and changing its value inside testStringPushdownWithCollate() is simple but I don't think it's an ideal way. Accepting Session at hasBehavior() might be smarter but this will require many changes even in other JDBC connectors' test cases.

Does splitting TestPostgreSqlConnectorTest to create another test class just for testing string pushdown with collate make sense?

[takezoe]

having a flag as a field and changing its value inside testStringPushdownWithCollate()

I adopted this way for now and added a case for join on VARCHAR columns.

[hashhar]

I adopted this way for now and added a case for join on VARCHAR columns.

Since only PostgreSQL has this behaviour today (and not enabled by default) I think it's fine to not add a new behaviour to TestingConnectorBehaviour just yet. That way you don't need the flag field (especially because we aren't using hasBehaviour in the new test at all).

In terms of future evolution I think once we prove it out with PostgreSQL we can lift the config to BaseJdbcConfig and then add a test in BaseJdbcConnectorTest which sets the session property - at that time we can revisit the best way to structure the TestingConnectorBehaviour (or maybe by that time we'll have more formal ways of handling collations).

@findepi @ebyhr @wendigo @takezoe Any thoughts?

[findepi]

@hashhar sounds good. except i am not convinced we need a config just yet, except for trying things out (a temporary kill switch)

[takezoe]

Ah, I see. That makes sense. I updated the test case.

[hashhar]

Closing and re-opening to get ci to run (some glitch on GitHub end).

[hashhar]

Unrelated failure. Thanks @takezoe for the feature. Merging it.