[FLINK-7194] [table] Add methods for type hints to UDAGG interface. #4379
Conversation
There was a problem hiding this comment.
Hi @fhueske Thanks for the PR. I like the changes. I only left a couple of suggestions.
Thanks, Jincheng
| */ | ||
| def requiresOver: Boolean = false | ||
|
|
||
| /** | ||
| * Returns the TypeInformation for the result of the AggregateFunction. |
There was a problem hiding this comment.
TypeInformation for the result -> TypeInformation of the result
| def getResultType: TypeInformation[T] = null | ||
|
|
||
| /** | ||
| * Returns the TypeInformation for the accumulator of the AggregateFunction. |
There was a problem hiding this comment.
Returns the TypeInformation for -> Returns of TypeInformation for
| @@ -191,10 +191,10 @@ class DecimalSumWithRetractAggFunction | |||
| acc.f1 = 0L | |||
| } | |||
|
|
|||
| def getAccumulatorType(): TypeInformation[_] = { | |||
| override def getAccumulatorType(): TypeInformation[DecimalSumWithRetractAccumulator] = { | |||
There was a problem hiding this comment.
getAccumulatorType()->getAccumulatorType
| @@ -17,6 +17,8 @@ | |||
| */ | |||
| package org.apache.flink.table.functions | |||
|
|
|||
There was a problem hiding this comment.
Suggest to remove the useless java doc, something like:
- line 35
- getAccumulatorType. - line 102 to line 112
def getResultType: TypeInformation[_]
| @@ -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, | |||
There was a problem hiding this comment.
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?
| s"You can override AggregateFunction.getResultType() to specify the type.", | ||
| ite) | ||
| } | ||
| } |
There was a problem hiding this comment.
Just a suggestion(I am not sure the suggestion is better or not):
Extract common code for getResultTypeOfAggregateFunction and getAccumulatorTypeOfAggregateFunction. Something like following:
There was a problem hiding this comment.
I moved the TypeExtractor part into a separate method
|
Thanks for the review @sunjincheng121. |
|
Hi @fhueske Thanks for the update. The PR. looks good to me. -:) +1 to merge. |
|
Loos good to me. +1 to merge |
|
Merging |
General
Documentation
JavaDoc for public methods has been added
Tests & Build
mvn clean verifyhas been executed successfully locally or a Travis build has passed