You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by shaoxuan-wang <gi...@git.apache.org> on 2017/05/26 02:55:55 UTC
[GitHub] flink pull request #3993: [FLINK-6725][table] make requiresOver as a contrac...
GitHub user shaoxuan-wang opened a pull request:
https://github.com/apache/flink/pull/3993
[FLINK-6725][table] make requiresOver as a contracted method in udagg
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [x] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shaoxuan-wang/flink F6725-submit
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3993.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3993
----
commit 5117a297c089de36723ac836b85502201c927dba
Author: shaoxuan-wang <ws...@gmail.com>
Date: 2017-05-26T02:52:31Z
[FLINK-6725][table] make requiresOver as a contracted method in udagg
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/3993
Regarding to (3), I agree with Fabian. From a user's perspective, it is not easy to implement a `getAccumulatorType()` or `getResultType()` contract function, as the signature and return type is complicated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by shaoxuan-wang <gi...@git.apache.org>.
Github user shaoxuan-wang commented on the issue:
https://github.com/apache/flink/pull/3993
Thanks for the review @fhueske @sunjincheng121 .
Fabian, yes, I do not think this will be a blocker for release. In fact, this is a similar interface clean-up issue as FLINK-6457 that we want to solve. But certainly, we can just merge this to master and include this in the next release.
Jincheng, yes, it is one's flavor about how to implement an interface with contracted method. From my point of view:
1. A contract method is usually to be very useful as a flag to indicate the characteristic of an aggregator, and this information will help the optimizer to decide the plan. Such methods are retract/merge/resetAccumulator.
2. I prefer we should just expose the user interface without any default implementation (an interface method with default implementation will anyway not control the user behavior). The user defined interface IMO should purely ask users to provide minimum required methods that can let the UDX work.
3. We will call getAccumulatorType and getResultType also during the compilation to generate the plan I think. If we put requiresOver into the interface, we should also consider to put getAccumulatorType and getResultType into interface as well.
Also, I think we may want to provide all the agg that requiresOver as the built-in agg in the future. Thereby, we may not need to expose this interface in the UDAGG at that point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3993: [FLINK-6725][table] make requiresOver as a contrac...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/3993#discussion_r119520730
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/UserDefinedFunctionUtils.scala ---
@@ -329,6 +330,21 @@ object UserDefinedFunctionUtils {
}
/**
+ * Returns the value (boolean) of AggregateFunction#requiresOver() that indicates if this
+ * AggregateFunction can only be used for over window aggregate. If method requiresOver is not
+ * provided, return false as default value.
+ */
+ def getRequiresOverConfig(aggregateFunction: AggregateFunction[_, _]): Boolean = {
+ try {
+ val method: Method = aggregateFunction.getClass.getMethod("requiresOver")
--- End diff --
Yes, I think we should check the return type of `requiresOver` as well , in order to give an explicit exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3993
The change itself is OK. However, I added a comment about breaking the API and the change itself to the corresponding JIRA issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3993: [FLINK-6725][table] make requiresOver as a contrac...
Posted by shaoxuan-wang <gi...@git.apache.org>.
Github user shaoxuan-wang closed the pull request at:
https://github.com/apache/flink/pull/3993
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3993
How do we continue with this PR @shaoxuan-wang?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3993
Hi @shaoxuan-wang and @sunjincheng121,
Ad 2) I think it is OK to have this as a method with a default implementation. If we choose the contract function design, we have the same default behavior as the default implementation if the method is not implemented, so IMO there is not really a difference. Due to the default implementation, a user does not need to implement the method unless it is necessary. However in that case, it can be overridden in a safe way with IDE support. A contract function, does not offer this safety because a user needs to lookup the exact signature in the documentation.
Ad 3) I think it would indeed make sense to add `getAccumulatorType()` and `getResultType()` to `AggregateFunction` and return `null` by default. This would also be more consistent with `ScalarFunction` and `TableFunction`. This would also help with type and compile safety, because we can enforce the type of the returned `TypeInformation` as `T` and `ACC` without going through reflection magic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3993
No worries. Thanks for the update!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3993
Hi @shaoxuan-wang, the suggestions of @fhueske make sense to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3993: [FLINK-6725][table] make requiresOver as a contrac...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/3993#discussion_r118806216
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/UserDefinedFunctionUtils.scala ---
@@ -329,6 +330,21 @@ object UserDefinedFunctionUtils {
}
/**
+ * Returns the value (boolean) of AggregateFunction#requiresOver() that indicates if this
+ * AggregateFunction can only be used for over window aggregate. If method requiresOver is not
+ * provided, return false as default value.
+ */
+ def getRequiresOverConfig(aggregateFunction: AggregateFunction[_, _]): Boolean = {
+ try {
+ val method: Method = aggregateFunction.getClass.getMethod("requiresOver")
--- End diff --
I think the method supported to check the `modifiers` as well. Just like `checkAndExtractMethods`'s method logic. So I suggest using `checkAndExtractMethods` directly. Changes as follows:
1. Add boolean param for `checkAndExtractMethods`
2. Change this method looks like :
```
val methods = checkAndExtractMethods(aggregateFunction, "requiresOver2", true)
if (methods.isEmpty || methods.length > 1) {
return false
} else {
methods(0).invoke(aggregateFunction).asInstanceOf[Boolean]
}
```
In addition, If you want to strictly check, you can check the input parameters and return type. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3993: [FLINK-6725][table] make requiresOver as a contracted met...
Posted by shaoxuan-wang <gi...@git.apache.org>.
Github user shaoxuan-wang commented on the issue:
https://github.com/apache/flink/pull/3993
@fhueske @sunjincheng121 @wuchong, thanks for the valuable inputs. We talked offline and get an agreement that we'd better to put `getAccumulatorType()`, `getResultType()` etc. in `AggregateFunction`. Sorry that I forgot to update this Jira. Let us move this change to [FLINK-7194](https://issues.apache.org/jira/browse/FLINK-7194). I will close this PR and [FLINK-6725](https://issues.apache.org/jira/browse/FLINK-6725).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---