[BEAM-11747] Reject the mixed Java UDF and ZetaSQL builtin operator cases #13912
Conversation
|
R: @ibzib |
|
We can certainly use #13898 if that covers BEAM-11747 |
|
I believe #13898 is not intended to fix BEAM-11747 given the current status. What do you think? |
ibzib
left a comment
ibzib
left a comment
There was a problem hiding this comment.
Just a couple style/documentation nits.
Basic idea LGTM. I wonder if there are still other possible cases we don't handle here. But I believe this is a strict improvement already.
| /** Returns true if the argument contains any user-defined Java functions. */ | ||
| static boolean hasUdfInProjects(RelOptRuleCall x) { | ||
| /** | ||
| * Returns true if the arguments only contain any user-defined Java functions, otherwise returns |
There was a problem hiding this comment.
We should add a few more sentences explaining exactly what this means too. It's kind of confusing right now because not all instances of SqlUserDefinedFunction are actually user defined, but someone who reads this code might not realize that unless we clarify.
| * Returns true if the arguments only contain any user-defined Java functions, otherwise returns | |
| * Returns true if the arguments only contain user-defined Java functions, otherwise returns |
There was a problem hiding this comment.
Added to comment to clarify what is user-defined java functions.
| /** | ||
| * Returns false if the argument contains any user-defined Java functions, otherwise returns true. | ||
| */ | ||
| static boolean hasNoJavaUdfInProjects(RelOptRuleCall x) { |
There was a problem hiding this comment.
Nit: positive phrasing avoids double negatives and makes the logic easier to follow, so consider inverting this (and renaming to something like hasJavaUdfInProjects or hasAnyJavaUdfInProjects)
There was a problem hiding this comment.
I agreed this might be better. But I think it's bit tricky to implement the positive version of hasNoJavaUdfInProjects . The hasNoJavaUdfInProjects is more error prone at this moment (we only need to consider java udf).
|
I think this is a strict constraint. Maybe we could loose the constraint in the future we can prove that Java UDF can execute with other operators together. But ultimately, we should go for Calc splitting to solve the problem. |
2 participants
Please add a meaningful description for your change here
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.