-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Implement dereference pushdown for MongoDB connector #14467
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 #14467
Conversation
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.
looks good
@@ -29,28 +33,51 @@ | |||
public class MongoColumnHandle | |||
implements ColumnHandle | |||
{ | |||
private final String name; | |||
private final String baseName; |
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.
nit: you can make a record out of it in a separate commit. That way there will be more deleted lines of code and some reviewers(me) will more likely approve because of that.
|
||
MongoTableHandle mongoTableHandle = (MongoTableHandle) handle; | ||
|
||
// all references are simple variables |
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.
nit: According to clean code rules you should extract a boolean variable with a meaningful name and get rid of the comment.
@@ -29,28 +33,51 @@ | |||
public class MongoColumnHandle | |||
implements ColumnHandle | |||
{ | |||
private final String name; | |||
private final String baseName; |
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.
nit: Changing name
to baseName
in a separate commit would be easier to review. It can be squashed before the merge if you like
try (TestTable table = new TestTable( | ||
new TrinoSqlExecutor(getQueryRunner()), | ||
"projection_pushdown", | ||
format("(col_0 row(col_1 %1$s, col_2 row(col_3 %1$s, col_4 row(col_5 %1$s))))", expectedType))) { |
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.
Add some dummy irrelevant column in the middle to see if it will be ignored
Description
Fixes #3765
Non-technical explanation
Improve performance for queries on nested data through dereference pushdown.
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.
(x) Release notes are required, with the following suggested text: