[BEAM-10925] Add rule to replace Calc with BeamCalcRel for ZetaSQL UDFs. #13841
Conversation
| root.rel.getCluster().invalidateMetadataQuery(); | ||
| return (BeamRelNode) plannerImpl.transform(0, desiredTraits, root.rel); | ||
| BeamRelNode beamRelNode = (BeamRelNode) plannerImpl.transform(0, desiredTraits, root.rel); | ||
| LOG.info("BEAMPlan>\n" + RelOptUtil.toString(beamRelNode)); |
There was a problem hiding this comment.
Nit: do we need this LOG.info?
There was a problem hiding this comment.
Not really, but it may be useful someday. I actually left this in from your original PR (#12398).
There was a problem hiding this comment.
I guess that was added for the purpose of debugging.
Keep this LOG.info is fine. I find that in CalciteQueryPlanner there is also a such LOG.info.
There was a problem hiding this comment.
I guess LOG.debug would probably be better. I might change both planners in a separate PR.
|
|
||
| @Override | ||
| public boolean matches(RelOptRuleCall x) { | ||
| return ZetaSQLQueryPlanner.hasUdfInProjects(x); |
There was a problem hiding this comment.
This is insufficient and likely introduces data corruption bugs if UDFs and regular operators are mixed. I plan to review this pull request on Monday.
apilloud
left a comment
apilloud
left a comment
There was a problem hiding this comment.
This needs tests. What happens if ZetaSQL is mixed with UDFs? It should fail to plan, but as implemented everything will BeamCalcRel.
| import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.rel.logical.LogicalCalc; | ||
|
|
||
| /** {@link ConverterRule} to replace {@link Calc} with {@link BeamCalcRel}. */ | ||
| public class BeamJavaUdfCalcRule extends ConverterRule { |
There was a problem hiding this comment.
nit: You can drop the convert method if you extend BeamCalcRule instead...
| } | ||
|
|
||
| /** Returns true if the argument contains any user-defined Java functions. */ | ||
| static boolean hasUdfInProjects(RelOptRuleCall x) { |
There was a problem hiding this comment.
nit: I would probably stick this method in BeamZetaSqlCalcRule. What you have here are operators not supported by ZetaSqlCalc.
|
We will need to reject the mixed UDF and builtin functions queries before calc/rule splitting happen. |
3 participants
The rule isn't used yet because there are no
USER_DEFINED_JAVA_SCALAR_FUNCTIONSyet.Most of this code was originally written by @amaliujia.
R: @amaliujia @apilloud
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.