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-4057] Implement trait propagation for EnumerableBatchNestedL… #2023

Closed

Conversation

amaliujia
Copy link
Contributor

…oopJoin (Rui Wang).

@amaliujia
Copy link
Contributor Author

R: @chunweilei @hsyuan

@@ -483,7 +476,6 @@

Query.create(sql)
.removeRule(EnumerableRules.ENUMERABLE_MERGE_JOIN_RULE)
.removeRule(EnumerableRules.ENUMERABLE_BATCH_NESTED_LOOP_JOIN_RULE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. This rule is not added by default.

@@ -693,6 +678,37 @@
.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE)
.check();
}

// push sort to left input
@Test void testBatchNestedLoopJoinLeftOuterJoinPushDownSort() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not adding too many tests for EnumerableBatchNestedLoopJoin because it reuses EnumerableNestedLoopJoin's code and those code was tested.

Add two test only to verify that trait propagation in EnumerableBatchNestedLoopJoin is working.

assert childId == 0;

RelCollation collation = childTraits.getCollation();
if (collation == null || collation == RelCollations.EMPTY) {
Copy link
Member

Choose a reason for hiding this comment

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

Add check

|| joinType == JoinRelType.FULL
        || joinType == JoinRelType.RIGHT

// Push sort both to left and right inputs does not help right outer join. It's because in
// implementation, EnumerableNestedLoopJoin produces (null, right_unmatched) all together,
// which does not preserve ordering from right side.
static Pair<RelTraitSet, List<RelTraitSet>> passThroughTraitsForNestedLoopJoin(
Copy link
Member

Choose a reason for hiding this comment

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

HashJoin and Correlate can reuse it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Will migrate them too to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

The class name is misleading. Should be EnumerableTraitUtil, Enum is a different thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a commit to make HashJoin and Correlate reuse the code. Also rename this class to EnumerableTraitUtils

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1

@amaliujia amaliujia force-pushed the rw-EnumerableBatchNestedLoopJoin branch 2 times, most recently from 87fcf30 to a69d4a9 Compare June 12, 2020 22:42
@@ -148,4 +149,53 @@ private static boolean isCollationOnTrivialExpr(
return null;
}
}

// This function can be reused when a Join's traits pass-down shall only
Copy link
Contributor

Choose a reason for hiding this comment

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

minor detail: I think it would be better to use javadoc comment here (instead of line comment). Same for deriveTraitsForJoin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to javadoc. Thanks for you suggestion!

@rubenada
Copy link
Contributor

LGTM

@amaliujia amaliujia force-pushed the rw-EnumerableBatchNestedLoopJoin branch from b014563 to 53d4a5d Compare June 13, 2020 19:08
@hsyuan hsyuan closed this in 978bb7e Jun 13, 2020
@amaliujia amaliujia deleted the rw-EnumerableBatchNestedLoopJoin branch June 13, 2020 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants