Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add projection pushdown support to MongoDB #16790

Closed
wants to merge 1 commit into from
Closed

Add projection pushdown support to MongoDB #16790

wants to merge 1 commit into from

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 29, 2023

Description

Additional context and related issues

Release notes

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

# Section
* Fix some things. ({issue}`issuenumber`)

@wendigo wendigo requested review from ebyhr and hashhar March 29, 2023 14:17
@github-actions github-actions bot added the mongodb MongoDB connector label Mar 29, 2023
@wendigo wendigo force-pushed the serafin/mongo-projection-pushdown branch from d19cabc to b338dac Compare March 30, 2023 14:38
@wendigo wendigo requested a review from ebyhr March 30, 2023 14:38
@cla-bot cla-bot bot added the cla-signed label Mar 30, 2023
ebyhr
ebyhr reviewed Apr 3, 2023
View reviewed changes
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java Outdated
for (MongoColumnHandle column : columns) {
output.append(column.getName(), 1);

if (tableHandle.getProjectedColumns().isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this condition instead of just relying on columns passed by MongoPageSourceProvider?

Copy link
Member

Choose a reason for hiding this comment

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

Reminder. It would be nice to leave a comment why the below code doesn't work if this condition is really needed.

        for (MongoColumnHandle column : columns) {
            output.append(column.getName(), 1);
        }

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoTableHandle.java Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Apr 25, 2023
ssheikin
ssheikin reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@ssheikin ssheikin left a comment

Choose a reason for hiding this comment

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

lgtm

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java Outdated Show resolved Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the stale label Apr 26, 2023
@wendigo wendigo force-pushed the serafin/mongo-projection-pushdown branch from b338dac to 7741186 Compare May 8, 2023 06:29
@wendigo wendigo force-pushed the serafin/mongo-projection-pushdown branch from 7741186 to eb85632 Compare May 8, 2023 06:45
@wendigo
Copy link
Contributor Author

wendigo commented May 8, 2023

@ebyhr PTAL

ebyhr
ebyhr approved these changes May 8, 2023
View reviewed changes
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
{
assertThat(query("SELECT orderdate, clerk FROM orders")).isFullyPushedDown();
}

Copy link
Member

Choose a reason for hiding this comment

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

Could you add isFullyPushedDown to testNativeQueryProjection in this class?

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java Outdated
for (MongoColumnHandle column : columns) {
output.append(column.getName(), 1);

if (tableHandle.getProjectedColumns().isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

Reminder. It would be nice to leave a comment why the below code doesn't work if this condition is really needed.

        for (MongoColumnHandle column : columns) {
            output.append(column.getName(), 1);
        }

@wendigo wendigo closed this May 9, 2023
@wendigo wendigo deleted the serafin/mongo-projection-pushdown branch May 9, 2023 13:47
@wendigo
Copy link
Contributor Author

wendigo commented May 9, 2023

I'm dropping this effort as there is no consistent way of implementing projection pushdown across connectors. There is a different approach in BigQuery, Iceberg, Delta Lake and others.

In BigQuery and Delta Lake we are passing projected columns as Optional<Set<ColumnHandle>> projectedColumns. In Iceberg we are following slightly different approach where the projectedColumns is just Set<ColumnHandle>. I don't have enough knowledge to determine which approach is superior to other or more "complete". Hence I'm closing this PR until this is decided to reduce back-and-forth with the changes.

@krvikash krvikash mentioned this pull request May 24, 2023
Closed
@krvikash
Copy link
Contributor

Addressed comments in #17618

@krvikash krvikash mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssheikin ssheikin ssheikin left review comments

@hashhar hashhar hashhar approved these changes

@ebyhr ebyhr ebyhr approved these changes

Assignees
No one assigned
Labels
cla-signed mongodb MongoDB connector
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wendigo @krvikash @ssheikin @ebyhr @hashhar