[BEAM-10925] Create interface for SQL Java aggregate function. #13306
Conversation
| import org.apache.beam.sdk.transforms.Combine; | ||
|
|
||
| /** An aggregate function that can be executed as part of a SQL query. */ | ||
| public class AggregateFn implements Serializable { |
There was a problem hiding this comment.
Do you think whether we should start by adding @Experimental?
|
Just checking: the interface for a SQL aggregation function is "can give you a CombineFn"? I understand this is the current status of UDAFs. Is that where we want to leave it? I think the main value of a separate interface (style nit: prefer to separate the interface from this particular implementation) would be a slight decoupling. (non-blocking, just questions & comments & I'll leave it to others) |
|
If we don't expose CombineFn in interface, I think what we will end is a similar interface design to current CombineFn: we will ask users to offer implementations The benefits for this decoupling seems little given CombineFn is a stable API in Beam. |
|
Also from potential user's requetes, there are existing CombinFn implementation and they want use those as UDAF. This is a different case from |
|
CombineFn is stable, but UDAF is not. One example is that a UDAF has to have a SQL type. Right now this is not represented on the UDAF object but is implied. That might change. Personally, I would make the type part of the UDAF object. If you look at any other SQL product's definition of UDAFs you will see lots of extra metadata (postgresql, mssql). A decoupled interface and trivial wrapper have obvious benefits and no real downside. |
|
I am not sure if I have understand:
This interface is the implementation interface of a UDAF, but not function definition. Function definition should provide SQL types, but definition in our case is the CREATE FUNCTION statement.
I don't agree in this case that creating a wrapper is trivial. For UDAF/CombeFn, I think it is non-trival work to create the wrapper. |
There was a problem hiding this comment.
How do we determine the name of the UDAF? Should there be a name method? Is there another interface for discovering these?
This mostly matches the interface of registerUdaf, so LGTM. I agree with Kenn that we will eventually want to offer a version of this interface that doesn't have a dependency on Beam.
I will add another interface to discover these after this PR and #13305 are merged. Something like this: |
|
As requested I rewrote this without a dependency on Beam Java core. Right now it is a shameless copy-paste of some stuff from
There already is a wrapper that's basically the same as what we're doing here: https://github.com/apache/beam/blob/release-2.25.0/sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/transform/agg/AggregationCombineFnAdapter.java Anyway, I will figure it out :) |
| * @param <AccumT> type of mutable accumulator values | ||
| * @param <OutputT> type of output values | ||
| */ | ||
| public interface AggregateFn< |
Experimental itself is a part of Beam java core: https://github.com/apache/beam/blob/486d23007dac513f9f90bb08c2a039cd72a7ec94/sdks/java/core/src/main/java/org/apache/beam/sdk/annotations/Experimental.java I added a note to the javadoc instead. |
R: @amaliujia @kennknowles
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.