-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-7371] [table] Add support for constant parameters in OVER aggregate #4736
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @twalthr.
I made a few suggestions.
Best, Fabian
|
||
val field = | ||
s""" | ||
|transient $boxedType $fieldTerm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why transient
? Couldn't this be final
?
.window( | ||
Over partitionBy 'c orderBy 'proctime preceding UNBOUNDED_ROW as 'w) | ||
.select('c, weightAvgFun('a, 42, 'b, "2") over 'w as 'wAvg) | ||
.select('c, 'wAvg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed
@@ -86,6 +86,13 @@ public Long getValue(WeightedAvgAccum accumulator) { | |||
} | |||
|
|||
// overloaded accumulate method | |||
// dummy to test constants | |||
public void accumulate(WeightedAvgAccum accumulator, long iValue, int iWeight, int x, String string) { | |||
accumulator.sum += iWeight + Integer.parseInt(string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the method to
accumulator.sum += (iValue + Integer.parseInt(string)) * iWeight;
accumulator.count += iWeight;
to have some influence of the value of string
in the result?
Thanks @fhueske. I addressed you feedback. Will merge this now... |
What is the purpose of the change
This PR allows to pass constants to OVER window aggregates. E.g.
.select('c, weightAvgFun('a, 42, 'b, "2") over 'w as 'wAvg)
.Brief change log
Until now the constants where simply ignored. I added code generation for the literals in
AggregationCodeGenerator
.Verifying this change
I add a ITCase for it. I might add more tests if I have time. In general, we need to rework the logic there a little bit, because I think we also do not support DATE, TIME etc. right now.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation