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
[FLINK-20756][python] Add PythonCalcSplitConditionRexFieldRule #14492
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 5e1ebe2 (Fri May 28 06:57:00 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
@HuangXingBo Thanks for your PR! There is a comment for the rule logic.
protected def containsFieldAccessAfterPythonCall(node: RexNode): Boolean = { | ||
node match { | ||
case call: RexCall => call.getOperands.exists(containsFieldAccessAfterPythonCall) | ||
case x: RexFieldAccess => isPythonCall(x.getReferenceExpr) |
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.
We should call the containsFieldAccessAfterPythonCall
here if the expression is not a python call to handle the situation like where javaRowFunc(pyRowFunc5(a)).f0 is NULL
. And maybe we can combine this logic into FunctionFinder
as the field access, java call, python call may appear alternately. That should be able to better to cover various situations.
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.
Yes, current logic can't deal with the case of where javaRowFunc(pyRowFunc5(a)).f0 is NULL
. We need to change isPythonCall
to containsPythonCall
and add the logic of visitFieldAccess
into the FunctionFinder
of PythonUtil
@WeiZhong94 Thanks a lot for the review. I have addressed the comments at the latest commit. |
de5551e
to
5e1ebe2
Compare
Thanks @HuangXingBo for the fix and thanks @WeiZhong94 for the review. Merged. |
…s of expression containing Python UDF in the condition of Calc This closes apache#14492.
What is the purpose of the change
This pull request will add
PythonCalcSplitConditionRexFieldRule
Brief change log
PythonCalcSplitConditionRexFieldRule
Verifying this change
This change added tests and can be verified as follows:
testPythonFunctionWithCompositeWhereClause
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation