[BEAM-10925] Convert ScalarFn to Method. #13593
Conversation
The Method will be passed to createUdfOperator in a future PR.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.beam.sdk.extensions.sql.zetasql.translation.impl; |
There was a problem hiding this comment.
Why is this in zetasql? How will it work for the calcite dialect?
There was a problem hiding this comment.
I think the PR description mentions the reason: The Method will be passed to createUdfOperator in a future PR.
There was a problem hiding this comment.
How will that work for the calcite dialect?
There was a problem hiding this comment.
There's nothing specific to ZetaSQL in this class, I can move it somewhere else.
There was a problem hiding this comment.
createUdfOperator is actually in ZetaSQL only now. But I agree we can move this code to the sql module and later might bring createUdfOperator to that module as well.
There was a problem hiding this comment.
Great. My concern here is that there already exists code to do this for Calcite SQL, if so we should just reuse that. It looks like Calcite SQL support just wasn't part of the prototype?
There was a problem hiding this comment.
My concern here is that there already exists code to do this for Calcite SQL, if so we should just reuse that.
We do something similar in Calcite SQL, but not exactly because the interfaces are different.
It looks like Calcite SQL support just wasn't part of the prototype?
Yeah, how (if at all) we want to integrate the new APIs into Calcite SQL is an open question.
There was a problem hiding this comment.
What I hope happens it that the UDFs are loaded into the schema in BeamSqlEnvBuilder, then both Calcite and ZetaSQL dialects pull them from there.
There was a problem hiding this comment.
What I hope happens it that the UDFs are loaded into the schema in BeamSqlEnvBuilder, then both Calcite and ZetaSQL dialects pull them from there.
I will try doing that for the next PR in this series.
.../src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/impl/ScalarFnImpl.java
Outdated
Show resolved
Hide resolved
| // or it should be an extension of the other matches (which came from parent | ||
| // classes). | ||
| Method first = matches.iterator().next(); | ||
| for (Method other : matches) { |
There was a problem hiding this comment.
What will happen if we choose one to execute (can print which is chosen and other methods into log)?
There was a problem hiding this comment.
This is standard inheritance, it shouldn't be any surprise to the user that a child class's implementation of a method overrides a parent class.
| ScalarFn.ApplyMethod.class.getSimpleName(), clazz.getName())); | ||
| } | ||
|
|
||
| // If we have at least one match, then either it should be the only match |
There was a problem hiding this comment.
Is there a test you could add to validate inheritance works as expected here? (Will the outer most class always be first?)
The Method will be passed to createUdfOperator in a future PR.
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.