-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Implement dereference pushdown for MongoDB connector #17710
Implement dereference pushdown for MongoDB connector #17710
Conversation
b38b744
to
ad5cd4b
Compare
305397a
to
83d9bd8
Compare
6cc226e
to
851554e
Compare
851554e
to
8baaf45
Compare
rebased to resolve conflicts |
8baaf45
to
777382a
Compare
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoPageSource.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Show resolved
Hide resolved
777382a
to
03877e3
Compare
Addressed comments and added |
03877e3
to
7f8da8b
Compare
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoColumnHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java
Outdated
Show resolved
Hide resolved
* Creates a set of sufficient columns for the input projected columns. For example, | ||
* if input {@param columns} include columns "a.b" and "a.b.c", then they will be projected from a single column "a.b". | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (mongoColumnHandle.isBaseColumn()) { | ||
return value; | ||
} | ||
if (value instanceof DBRef dbRefValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the engine to send only DBRef
to the underlying MongoDB and it could apply the projection pushdown at its end ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the projection pushdown, MongoPageSource
will have all dereferenced columns.
I could not find a way to send only DBRef
here. Let me know if you have some suggestions.
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorSmokeTest.java
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Show resolved
Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/projection/ApplyProjectionUtil.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,295 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the plan assertion as a part of ConnectorTest/ConnectorSmokeTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to what TestDeltaLakeProjectionPushdownPlans
and other connectors' Projection Pushdown Plan tests.
table -> { | ||
MongoTableHandle mongoTableHandle = (MongoTableHandle) table; | ||
TupleDomain<ColumnHandle> constraint = mongoTableHandle.getConstraint(); | ||
return mongoTableHandle.getProjectedColumns().equals(ImmutableSet.of(column0Handle, columnY)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to do an exact equals here - We need to ensure that - we project till the parent column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e6e59ac
to
8866ee9
Compare
Can we squash the fixup commits |
8866ee9
to
4d3f73f
Compare
Done |
Name Base class test relevant so that it can be use for non-file format based connector test.
4d3f73f
to
7f7cac3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
% comments
...no-plugin-toolkit/src/test/java/io/trino/plugin/base/projection/TestApplyProjectionUtil.java
Outdated
Show resolved
Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/projection/ApplyProjectionUtil.java
Outdated
Show resolved
Hide resolved
.setCatalogSessionProperty(CATALOG, "projection_pushdown_enabled", "false") | ||
.build(); | ||
|
||
getQueryRunner().execute("CREATE TABLE " + tableName + " (col0) AS SELECT CAST(row(5, 6) AS row(a bigint, b bigint)) AS col0 WHERE false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace them with TestTable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test class uses LocalQueryRunner
which does not allow DROP TABLE
. TestTable
execute 'DROP TABLE' after executing on close. So can not use TestTable
here.
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoTableHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoColumnHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoPageSource.java
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
|
||
private static boolean isPushDownSupported(ConnectorExpression connectorExpression) | ||
{ | ||
if (!(connectorExpression instanceof FieldDereference fieldDereference)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tricky - now it means we support non FieldDereference pushdown expression too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated method to return false
for connectorExpression
other than Variable
and FieldDereference
.
Thanks, @Praveen2112 for reviewing. I have addressed the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits.
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java
Outdated
Show resolved
Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/projection/ApplyProjectionUtil.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java
Outdated
Show resolved
Hide resolved
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
...in/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoProjectionPushdownPlans.java
Outdated
Show resolved
Hide resolved
Co-authored-by: praveenkrishna <[email protected]> Co-authored-by: Mateusz "Serafin" Gajewski <[email protected]>
06b3390
to
8a77330
Compare
Thanks, @Praveen2112 for reviewing. Addressed comments. |
CI (https://github.com/trinodb/trino/actions/runs/5420554711/jobs/9854904606?pr=17710) is failing. Same issue #10631 |
Thank you all for reviewing the PR and merging it. |
Thanks for working on this. |
Description
This PR implements dereference pushdown for Mongodb connector(similar to #17085).
This adds significant performance improvements for queries accessing nested fields inside struct/row columns. They have been optimized through the pushdown of dereference expressions. With this feature, the query execution prunes structural data eagerly, extracting the necessary fields.
More Details about dereference pushdown: https://trino.io/blog/2020/08/14/dereference-pushdown.html
Note: This PR merges the work of #14467 and #16790
Additional context and related issues
The feature is enabled by default.
The feature can be disabled by setting
mongodb.projection-pushdown-enabled
configuration property ormongodb.projection_pushdown_enabled
session property tofalse
.Release notes
(X) Release notes are required, with the following suggested text: