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

DRILL-6545: Projection Push down into Lateral Join operator. #1347

Closed
wants to merge 2 commits into from

Conversation

HanumathRao
Copy link
Contributor

@amansinha100 Please review this PR.

This PR includes.

  1. Two new Rules for Push Project into lateral join and push project past lateral join( this rule is only to avoid triggering for un-necessary cases).
  2. Change in LateralPop so as to pass the excluded columns to runtime operator.
  3. Constructing row type for LateralJoin based after removing the correlated column.

List<RelDataType> fields = new ArrayList<>();
List<String> fieldNames = new ArrayList<>();
if (excludeCorrelateColumn) {
int corrVariable = this.requiredColumns.nextSetBit(0);

Choose a reason for hiding this comment

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

An assumption here is that there is only 1 correlated column which is true for Drill's Lateral-Unnest implementation but I believe Calcite allows multiple. You could just put a Preconditions check here to be safe.

public boolean matches(RelOptRuleCall call) {
Correlate correlate = call.rel(1);

if (correlate instanceof DrillLateralJoinRel && ((DrillLateralJoinRel)correlate).excludeCorrelateColumn) {

Choose a reason for hiding this comment

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

Pls add a comment why this should not match if excludeCorrelateColumn is true.

@@ -104,6 +118,12 @@ private RelNode rename(RelNode input, List<RelDataTypeField> inputFields, List<S
return proj;
}

@Override
public RelWriter explainTerms(RelWriter pw) {
return super.explainTerms(pw).item("correlate Column: ", this.excludeCorrelateColumn ? this.getColumn() : "None");

Choose a reason for hiding this comment

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

Will this confuse users in the sense that 'correlate column' is actually always needed for the semantics of lateral join but what excludeCorrelateColumn does is it prevents materialization of that column from the left side of the lateral.


private Map<Integer, Integer> buildMapWithoutCorrColumn(RelNode corr, int correlationIndex) {
int index = 0;
Map<Integer, Integer> result = new HashedMap();

Choose a reason for hiding this comment

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

Any reason for using a HashedMap instead of just HashMap ? (unless you are using the MapIterator it seems the latter should suffice).

}
}

public static class RexFieldsTransformer {

Choose a reason for hiding this comment

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

Is this a generic transformer for Rex exprs ? If so, pls add a java doc for future use of this class.

Copy link

@amansinha100 amansinha100 left a comment

Choose a reason for hiding this comment

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

+1

@HanumathRao
Copy link
Contributor Author

@amansinha100 Thank you for the review.

vvysotskyi pushed a commit to vvysotskyi/drill that referenced this pull request Jul 1, 2018
@asfgit asfgit closed this in 8ec2dc6 Jul 2, 2018
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.

None yet

2 participants