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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kramerul
Copy link
Contributor

@kramerul kramerul commented Dec 4, 2023

@kramerul kramerul force-pushed the batch-nested-loop-jdbc branch 2 times, most recently from 1e605b9 to 480e4ba Compare December 4, 2023 09:46
@kramerul kramerul changed the title Implement BatchNestedLoopJoin for JDBC [CALCITE-5409] Implement BatchNestedLoopJoin for JDBC Dec 4, 2023
@kramerul kramerul force-pushed the batch-nested-loop-jdbc branch 2 times, most recently from 3268a7c to 3d0e301 Compare December 8, 2023 13:55
Copy link

sonarcloud bot commented Dec 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

81.2% 81.2% Coverage
0.0% 0.0% Duplication

@zabetak zabetak self-assigned this Dec 21, 2023
Copy link
Contributor

@tanclary tanclary left a 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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs newline

Copy link
Contributor Author

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();
Copy link
Contributor

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 🤷

Copy link
Contributor Author

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() {
Copy link
Contributor

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

Copy link
Contributor Author

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"
Copy link
Contributor

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?

Copy link
Contributor Author

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;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants