You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by fhueske <gi...@git.apache.org> on 2017/07/20 15:37:55 UTC
[GitHub] flink pull request #4379: [FLINK-7194] [table] Add methods for type hints to...
GitHub user fhueske opened a pull request:
https://github.com/apache/flink/pull/4379
[FLINK-7194] [table] Add methods for type hints to UDAGG interface.
- [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 for UDAGGs will be provided by FLINK-6751
- [X] 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/fhueske/flink tableUDAGG
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4379.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 #4379
----
commit 22a56c0a2c7e4017b2c3bda56d07cdd6c5d39144
Author: Fabian Hueske <fh...@apache.org>
Date: 2017-07-20T13:09:06Z
[FLINK-7194] [table] Add default implementations for type hints to UDAGG interface.
----
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128662028
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/UserDefinedFunctionUtils.scala ---
@@ -314,7 +314,28 @@ object UserDefinedFunctionUtils {
aggregateFunction: AggregateFunction[_, _],
extractedType: TypeInformation[_] = null)
: TypeInformation[_] = {
- getParameterTypeOfAggregateFunction(aggregateFunction, "getResultType", 0, extractedType)
+
+ val resultType = aggregateFunction.getResultType
+ if (resultType != null) {
+ resultType
+ } else if (extractedType != null) {
+ extractedType
+ } else {
+ try {
+ TypeExtractor
+ .createTypeInfo(aggregateFunction,
+ classOf[AggregateFunction[_, _]],
+ aggregateFunction.getClass,
+ 0)
+ .asInstanceOf[TypeInformation[_]]
+ } catch {
+ case ite: InvalidTypesException =>
+ throw new TableException(
+ s"Cannot infer generic type of ${aggregateFunction.getClass}. " +
+ s"You can override AggregateFunction.getResultType() to specify the type.",
+ ite)
+ }
+ }
--- End diff --
Just a suggestion(I am not sure the suggestion is better or not):
Extract common code for `getResultTypeOfAggregateFunction` and `getAccumulatorTypeOfAggregateFunction`. Something like following:
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128773463
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/UserDefinedFunctionUtils.scala ---
@@ -314,7 +314,28 @@ object UserDefinedFunctionUtils {
aggregateFunction: AggregateFunction[_, _],
extractedType: TypeInformation[_] = null)
: TypeInformation[_] = {
- getParameterTypeOfAggregateFunction(aggregateFunction, "getResultType", 0, extractedType)
+
+ val resultType = aggregateFunction.getResultType
+ if (resultType != null) {
+ resultType
+ } else if (extractedType != null) {
+ extractedType
+ } else {
+ try {
+ TypeExtractor
+ .createTypeInfo(aggregateFunction,
+ classOf[AggregateFunction[_, _]],
+ aggregateFunction.getClass,
+ 0)
+ .asInstanceOf[TypeInformation[_]]
+ } catch {
+ case ite: InvalidTypesException =>
+ throw new TableException(
+ s"Cannot infer generic type of ${aggregateFunction.getClass}. " +
+ s"You can override AggregateFunction.getResultType() to specify the type.",
+ ite)
+ }
+ }
--- End diff --
Sounds good.
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128728039
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/AggregateFunction.scala ---
@@ -17,6 +17,8 @@
*/
package org.apache.flink.table.functions
--- End diff --
Good catch, thanks
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to UDAGG ...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/4379
Thanks for the review @sunjincheng121.
I updated the PR.
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128657888
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/AggregateFunction.scala ---
@@ -136,8 +137,26 @@ abstract class AggregateFunction[T, ACC] extends UserDefinedFunction {
*/
def getValue(accumulator: ACC): T
- /**
- * whether this aggregate only used in OVER clause
+ /**
+ * Returns true if this AggregateFunction can only be applied in an OVER window.
+ *
+ * @return true if the AggregateFunction requires an OVER window, false otherwise.
*/
def requiresOver: Boolean = false
+
+ /**
+ * Returns the TypeInformation for the result of the AggregateFunction.
--- End diff --
`TypeInformation for the result ` -> `TypeInformation of the result `
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128745444
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/UserDefinedFunctionUtils.scala ---
@@ -314,7 +314,28 @@ object UserDefinedFunctionUtils {
aggregateFunction: AggregateFunction[_, _],
extractedType: TypeInformation[_] = null)
: TypeInformation[_] = {
- getParameterTypeOfAggregateFunction(aggregateFunction, "getResultType", 0, extractedType)
+
+ val resultType = aggregateFunction.getResultType
+ if (resultType != null) {
+ resultType
+ } else if (extractedType != null) {
+ extractedType
+ } else {
+ try {
+ TypeExtractor
+ .createTypeInfo(aggregateFunction,
+ classOf[AggregateFunction[_, _]],
+ aggregateFunction.getClass,
+ 0)
+ .asInstanceOf[TypeInformation[_]]
+ } catch {
+ case ite: InvalidTypesException =>
+ throw new TableException(
+ s"Cannot infer generic type of ${aggregateFunction.getClass}. " +
+ s"You can override AggregateFunction.getResultType() to specify the type.",
+ ite)
+ }
+ }
--- End diff --
I moved the TypeExtractor part into a separate method
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4379
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to UDAGG ...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/4379
Merging
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128658053
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/AggregateFunction.scala ---
@@ -136,8 +137,26 @@ abstract class AggregateFunction[T, ACC] extends UserDefinedFunction {
*/
def getValue(accumulator: ACC): T
- /**
- * whether this aggregate only used in OVER clause
+ /**
+ * Returns true if this AggregateFunction can only be applied in an OVER window.
+ *
+ * @return true if the AggregateFunction requires an OVER window, false otherwise.
*/
def requiresOver: Boolean = false
+
+ /**
+ * Returns the TypeInformation for the result of the AggregateFunction.
+ *
+ * @return The TypeInformation of the result of the AggregateFunction or null if the result type
+ * should be automatically inferred.
+ */
+ def getResultType: TypeInformation[T] = null
+
+ /**
+ * Returns the TypeInformation for the accumulator of the AggregateFunction.
--- End diff --
` Returns the TypeInformation for` -> ` Returns of TypeInformation for`
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128660377
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/aggfunctions/AvgAggFunction.scala ---
@@ -80,11 +80,11 @@ abstract class IntegralAvgAggFunction[T] extends AggregateFunction[T, IntegralAv
acc.f1 = 0L
}
- def getAccumulatorType: TypeInformation[_] = {
+ override def getAccumulatorType: TypeInformation[IntegralAvgAccumulator] = {
new TupleTypeInfo(
new IntegralAvgAccumulator().getClass,
--- End diff --
How about classOf[IntegralAvgAccumulator], Although classOf[T] is equivalent to the class literal T.class in Java. but I think it is more concise.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 pull request #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128659642
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/AggregateFunction.scala ---
@@ -17,6 +17,8 @@
*/
package org.apache.flink.table.functions
--- End diff --
Suggest to remove the useless java doc, something like:
1. line 35 `- getAccumulatorType.`
2. line 102 to line 112 `def getResultType: TypeInformation[_]`
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to UDAGG ...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/4379
Loos good to me. +1 to merge
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4379#discussion_r128658352
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/aggfunctions/SumWithRetractAggFunction.scala ---
@@ -191,10 +191,10 @@ class DecimalSumWithRetractAggFunction
acc.f1 = 0L
}
- def getAccumulatorType(): TypeInformation[_] = {
+ override def getAccumulatorType(): TypeInformation[DecimalSumWithRetractAccumulator] = {
--- End diff --
`getAccumulatorType()`->`getAccumulatorType`
---
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 #4379: [FLINK-7194] [table] Add methods for type hints to UDAGG ...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/4379
Hi @fhueske Thanks for the update. The PR. looks good to me. -:)
+ to merge.
---
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.
---