-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[CALCITE-5409] Implement BatchNestedLoopJoin for JDBC #3562
base: main
Are you sure you want to change the base?
Conversation
1e605b9
to
480e4ba
Compare
3268a7c
to
3d0e301
Compare
3d0e301
to
b1f5867
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
Thanks for the PR!
this.delegate = delegate; | ||
this.parameters = parameters; | ||
} | ||
@Override public @Nullable SchemaPlus getRootSchema() { |
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.
Needs newline
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.
OK
if (context != null) { | ||
return context; | ||
} | ||
List<RelDataTypeField> fieldList = variable.getType().getFieldList(); |
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.
Maybe I'm just not very familiar with JDBC but could we potentially add a comment or two explaining what is going on here. (If it's obvious to everyone but me then maybe not 🤷
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.
I added a comment
@@ -1147,6 +1153,52 @@ private LockWrapper exclusiveCleanDb(Connection c) throws SQLException { | |||
}); | |||
} | |||
|
|||
@Test void testBatchNestedLoopJoinPlan() { |
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.
You could add comment with link to JIRA
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.
Sure
+ "LEFT OUTER JOIN \"foodmart\".\"store\" B ON A.\"empid\" = B.\"store_id\""; | ||
final String explain = "JdbcFilter(condition=[OR(=($cor0.empid0, $0), =($cor1.empid0, $0)"; | ||
final String jdbcSql = "SELECT *\n" | ||
+ "FROM \"foodmart\".\"store\"\n" |
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.
Are there any other edge cases or situations we should test? Asked another way: Is one test enough to ensure that this works the way we intend?
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.
I think one test should be enough, as only one additional execution path is added to the code.
Context context = correlTableMap.get(variable.id);
if (context != null) {
return context;
}
See CALCITE-5409