-
Notifications
You must be signed in to change notification settings - Fork 140
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
Push down sort through eval #2937
base: main
Are you sure you want to change the base?
Push down sort through eval #2937
Conversation
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
Signed-off-by: Heng Qian <[email protected]>
} else return sort; | ||
} else return sort; | ||
} else return sort; |
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 you can return sort
at the end of for
block once.
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.
Then we need a flag to indicate returning sort or not for each iteration.
It could be simplified by pattern matching with java version>=14. But since we also need to support java11 for 2.x, I don't use that feature in order to keep code align.
*/ | ||
if (pair.getRight() instanceof ReferenceExpression) { | ||
ReferenceExpression ref = (ReferenceExpression) pair.getRight(); | ||
Expression replacedExpr = evalExpressionMap.getOrDefault(ref, ref); |
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.
Is it right to return ref
as default value?
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.
Logically, it's right I think, though the name of replacedExpr
may be a little bit misleading. It includes 2 cases:
- the
ref
is produced by eval operator, then it needs to be replaced - the
ref
isn't produced by eval operator, then we don't need to replace it. That's why I use the default valueref
.
How about changing the name of replacedExpr
to newExpr
?
public static final Rule<LogicalSort> PUSH_DOWN_SORT = | ||
match(sort(evalCapture())) | ||
.apply( | ||
(sort, logicalEval) -> { |
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 this Lambda is too large. Please extract to a method or a class. And extract some portion to reduce the size of method for readability.
// don't push sort if sort field is not ReferenceExpression | ||
Expression nonRefExpr = DSL.add(DSL.ref("intV", INTEGER), DSL.literal(1)); | ||
Pair<SortOption, Expression> sortExprWithNonRef = | ||
Pair.of(Sort.SortOption.DEFAULT_ASC, nonRefExpr); | ||
LogicalPlan originPlan = sort(eval(relation("schema", table), evalExpr), sortExprWithNonRef); | ||
assertEquals(originPlan, optimize(originPlan)); | ||
|
||
// don't push sort if replaced expr in eval is not ReferenceExpression | ||
Pair<ReferenceExpression, Expression> evalExprWithNonRef = Pair.of(sortRef, nonRefExpr); | ||
originPlan = sort(eval(relation("schema", table), evalExprWithNonRef), sortExpr); | ||
assertEquals(originPlan, optimize(originPlan)); | ||
|
||
// don't push sort if there are internal reference in eval | ||
Pair<ReferenceExpression, Expression> evalExpr2 = Pair.of(sortRef, evalRef); | ||
originPlan = sort(eval(relation("schema", table), evalExpr, evalExpr2), sortExpr); | ||
assertEquals(originPlan, optimize(originPlan)); |
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.
Please split the method for each test case.
Signed-off-by: Heng Qian <[email protected]>
Description
Add a rule EvalPushDown.PUSH_DOWN_SORT, this rule will push down limit under eval. Thus, sort has chance to be pushed down into TableScanBuilder later.
For now, it only supports push specific sort and eval, which has restriction:
As described in this RFC #2864 , we may have chance to extend it.
e.g.
Related Issues
Resolves #2904
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.