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

Implement Join pushdown for JDBC connectors #6874

Merged
merged 1 commit into from
Mar 5, 2021
Merged

Implement Join pushdown for JDBC connectors #6874

merged 1 commit into from
Mar 5, 2021
+1,056 −30

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 11, 2021 •
edited
Loading

For #6620

@cla-bot cla-bot bot added the cla-signed label Feb 11, 2021
@findepi findepi mentioned this pull request Feb 11, 2021
Closed
@findepi findepi force-pushed the findepi/join-jdbc branch 2 times, most recently from 15a633f to 90a814d Compare February 11, 2021 22:04
@findepi findepi force-pushed the findepi/join-jdbc branch 3 times, most recently from 6dcb5e7 to b912651 Compare February 19, 2021 16:49
@findepi findepi mentioned this pull request Feb 19, 2021
Closed
@findepi findepi requested review from kokosing and losipiuk February 19, 2021 21:44
@findepi findepi force-pushed the findepi/join-jdbc branch 2 times, most recently from 9309fe0 to 6f64b48 Compare February 20, 2021 17:23
@findepi findepi marked this pull request as ready for review February 20, 2021 23:23
kokosing
kokosing reviewed Feb 22, 2021
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

ack

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Outdated
return !(joinCondition.getLeftColumn().getColumnType() instanceof CharType) &&
!(joinCondition.getLeftColumn().getColumnType() instanceof VarcharType) &&
!(joinCondition.getRightColumn().getColumnType() instanceof CharType) &&
!(joinCondition.getRightColumn().getColumnType() instanceof VarcharType);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to extract this as method and reuse it? As:

Remote database can be case insensitive.

is a quite common case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Need -- no.

Want -- i don't know.
I don't want to bloat BaseJdbcClient, but this can go somewhere else

...-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java Outdated
}

@Test
public void testJoinPushdown()
Copy link
Member

Choose a reason for hiding this comment

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

It looks like a feature is written in a generic way, while testing only happens for PostgreSQL. Can we somehow disable this in other connectors until the moment test coverage is added to them?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's disabled by default, and i am still thinking how to apply the test to all connectors, sanely.

@findepi findepi force-pushed the findepi/join-jdbc branch 3 times, most recently from cc69438 to 0e3aa2c Compare March 3, 2021 10:53
@findepi findepi mentioned this pull request Mar 3, 2021
Merged
@findepi findepi force-pushed the findepi/join-jdbc branch from 0e3aa2c to 82975a1 Compare March 3, 2021 12:30
@findepi
Copy link
Member Author

findepi commented Mar 3, 2021

PTAL

@findepi
Copy link
Member Author

findepi commented Mar 3, 2021

not PTAL yet

@findepi findepi force-pushed the findepi/join-jdbc branch 2 times, most recently from 9b632a2 to 6bfe97d Compare March 3, 2021 22:38
@findepi findepi mentioned this pull request Mar 3, 2021
Merged
@findepi findepi requested review from hashhar and electrum March 3, 2021 22:40
@findepi findepi requested review from losipiuk and kokosing March 3, 2021 22:40
@findepi
Copy link
Member Author

findepi commented Mar 3, 2021

May work now. PTAL

@findepi findepi force-pushed the findepi/join-jdbc branch 2 times, most recently from 1cc7e39 to fcb1a5b Compare March 3, 2021 23:23
hashhar
hashhar reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Some comments. Didn't review any changes in planner or JdbcMetadata.

plugin/trino-memsql/src/main/java/io/trino/plugin/memsql/MemSqlClient.java
Map<JdbcColumnHandle, String> rightAssignments,
Map<JdbcColumnHandle, String> leftAssignments)
{
if (joinType == JoinType.FULL_OUTER) {
Copy link
Member

Choose a reason for hiding this comment

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

static import for this and IS_DISTINCT_FROM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to static import these, because there can be multiple different ways of referring to joins and Client is not join-specific.
All am saying is that it was conscious, but still am open to suggestions

@findepi findepi force-pushed the findepi/join-jdbc branch 2 times, most recently from 5b692ff to 8b85913 Compare March 4, 2021 10:32
@findepi
Copy link
Member Author

findepi commented Mar 4, 2021

I changed the // varchar inequality along with an equality, which constitutes an equi-condition and allows filter to remain as part of the Join test case so that it exploits case sensitivity issues, if connectors pushes down to much
it now uses nation and nation_lowercase tables for this.

@findepi findepi force-pushed the findepi/join-jdbc branch from 8b85913 to 3167b44 Compare March 4, 2021 16:15
@findepi findepi mentioned this pull request Mar 4, 2021
Merged
@findepi
Copy link
Member Author

findepi commented Mar 4, 2021

@losipiuk @hashhar @kokosing PTAL

@losipiuk
Copy link
Member

losipiuk commented Mar 5, 2021

image
good to go

@findepi
Copy link
Member Author

findepi commented Mar 5, 2021

image
good to go

these are reviewed over here: #7145
and yes, i am gonna merge that

losipiuk
losipiuk approved these changes Mar 5, 2021
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Nice!

kokosing
kokosing reviewed Mar 5, 2021
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

ack.

@findepi findepi force-pushed the findepi/join-jdbc branch from 1d9ed57 to 2dca5ff Compare March 5, 2021 12:34
@findepi findepi added this to the 353 milestone Mar 5, 2021
@findepi findepi added the enhancement New feature or request label Mar 5, 2021
@findepi findepi merged commit d7c825c into trinodb:master Mar 5, 2021
@findepi findepi deleted the findepi/join-jdbc branch March 5, 2021 12:34
This was referenced Mar 5, 2021
Open
Closed
@FishermanZzhang FishermanZzhang mentioned this pull request Apr 2, 2021
Closed
@rsaw4
Copy link

rsaw4 commented Jul 6, 2021

sqlserver join pushdown across dbs blocked by #4690

@vlad-lyutenko vlad-lyutenko mentioned this pull request Oct 31, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@kokosing kokosing kokosing left review comments

@hashhar hashhar hashhar left review comments

@losipiuk losipiuk losipiuk approved these changes

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed enhancement New feature or request
Milestone
353
Development

Successfully merging this pull request may close these issues.
5 participants
@findepi @losipiuk @rsaw4 @kokosing @hashhar