Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Push CAST into PostgreSQL #11522

Closed
wants to merge 1 commit into from
Closed

Push CAST into PostgreSQL #11522

wants to merge 1 commit into from

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 16, 2022

Let's discuss if it is safe approach to let decide connector if it can cast on its side.

@wendigo wendigo requested a review from findepi March 16, 2022 16:51
@wendigo wendigo force-pushed the serafin/translate-cast branch from 78076b2 to 4df9597 Compare March 16, 2022 16:52
@cla-bot cla-bot bot added the cla-signed label Mar 16, 2022
@findepi findepi requested review from martint, losipiuk and hashhar March 16, 2022 18:52
@findepi
Copy link
Member

findepi commented Mar 16, 2022

Adding this to connector expressions seems like right thing to do.
Regarding rewrites in various connectors, i'm concerned about type systems being not quite the same (because they usually are not). I don't have any clever alternatives. Maybe we can defer that part for a bit and see what pains do we see and what areas we need to address. Then, maybe, we can realize we don't need explicit CAST support, but we do need something else.

@wendigo
Copy link
Contributor Author

wendigo commented Mar 16, 2022

@findepi that's why I've left it up to connector to decide how this should be mapped so proper type semantic can be guaranteed

@findepi
Copy link
Member

findepi commented Mar 17, 2022

@wendigo would you be OK separating Rewrite and push down CAST in PostgreSQL connector into a new PR?

@wendigo
Copy link
Contributor Author

wendigo commented Mar 17, 2022

@findepi sure

@findepi findepi mentioned this pull request Mar 17, 2022
Merged
@wendigo wendigo force-pushed the serafin/translate-cast branch from 4df9597 to 7ca6319 Compare March 17, 2022 18:42
@wendigo wendigo mentioned this pull request Mar 17, 2022
Merged
@wendigo
Copy link
Contributor Author

wendigo commented Mar 17, 2022

This will be rebased or reworked when #11551 is merged

@findepi findepi marked this pull request as draft March 31, 2022 10:35
@findepi findepi changed the title Translate CAST to connector expression Push CAST into PostgreSQL Mar 31, 2022
@findepi
Copy link
Member

findepi commented Mar 31, 2022

Marked as draft, as it depends on #11551

@wendigo wendigo force-pushed the serafin/translate-cast branch from 7ca6319 to 6f07eee Compare April 8, 2022 13:25
@wendigo wendigo marked this pull request as ready for review April 8, 2022 13:25
@wendigo wendigo requested a review from findepi April 8, 2022 13:25
@wendigo
Copy link
Contributor Author

wendigo commented Apr 8, 2022

@findepi ptal

findepi
findepi reviewed Apr 13, 2022
View reviewed changes
@wendigo wendigo force-pushed the serafin/translate-cast branch from 6f07eee to 2c15e41 Compare April 15, 2022 08:01
@wendigo wendigo requested a review from findepi April 15, 2022 08:01
findepi
findepi requested changes Apr 15, 2022
View reviewed changes
plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java
}

try {
return Optional.ofNullable(this.toWriteMapping(connectorSession, type).getDataType());
Copy link
Member

Choose a reason for hiding this comment

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

Delegating to toWriteMapping isn't generally safe correct, hence the exclusions above.
The code may be producing correct results today (i haven't checked), but is error prone: any new addition or change in toWriteMapping may the code to work incorrectly, without any test failing. Thus, the delegation here is error-prone.

For example, let's say i add support for TIMESTAMP_PICOS in toWriteMapping... (actually, it's already there...).

Instead,

  1. let's call this method mapTypeForCast
  2. let's explicitly define which types we support for CAST pushdown; for these, we don't need to delegate to toWriteMapping, since we know the mapping
  3. let's have a test coverage for every supported type.
  4. Maybe io.trino.testing.BaseConnectorTest#testDataMappingSmokeTest could exercise query correctness (obvious when pushdown doesn't happen, but important to test when it does)

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java
{BIGINT, "bigint"},
{TIMESTAMP_TZ_SECONDS, "timestamptz(0)"},
{TIMESTAMP_TZ_MICROS, "timestamptz(6)"},
{JsonType.JSON, "jsonb"},
Copy link
Member

Choose a reason for hiding this comment

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

can this be imported statically?

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java
@Test
public void testCastPushdown()
{
assertThat(query("SELECT name FROM nation WHERE CAST(nationkey + regionkey AS varchar) = '3'"))
Copy link
Member

Choose a reason for hiding this comment

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

nice test case!

@bitsondatadev
Copy link
Member

👋 @wendigo - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open.

@wendigo wendigo deleted the serafin/translate-cast branch November 19, 2022 08:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@findepi findepi findepi requested changes

@martint martint Awaiting requested review from martint

@losipiuk losipiuk Awaiting requested review from losipiuk

@hashhar hashhar Awaiting requested review from hashhar

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wendigo @findepi @bitsondatadev