-
Notifications
You must be signed in to change notification settings - Fork 94
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
[NU-1921] Add standard deviation and variance aggregations #7307
base: staging
Are you sure you want to change the base?
[NU-1921] Add standard deviation and variance aggregations #7307
Conversation
created: #7322 |
0e84fe5
to
acf33c6
Compare
acf33c6
to
c665586
Compare
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
...est/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/TransformersTest.scala
Outdated
Show resolved
Hide resolved
...est/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/TransformersTest.scala
Outdated
Show resolved
Hide resolved
.../src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/aggregates.scala
Outdated
Show resolved
Hide resolved
utils/math-utils/src/main/scala/pl/touk/nussknacker/engine/util/MathUtils.scala
Show resolved
Hide resolved
Please add changelog entry. |
…tion-and-variance-aggregations
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Show resolved
Hide resolved
...est/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/TransformersTest.scala
Show resolved
Hide resolved
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
.../test/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/AggregatesSpec.scala
Outdated
Show resolved
Hide resolved
.../src/main/scala/pl/touk/nussknacker/engine/flink/util/transformer/aggregate/aggregates.scala
Outdated
Show resolved
Hide resolved
@TypeInfo(classOf[LargeFloatSumState.TypeInfoFactory]) | ||
// it would be natural to use type Number instead of this class | ||
// it is done this way so that it is serialized properly | ||
case class LargeFloatSumState( |
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.
For me it would be clearer to represent this class as 3 cases to avoid null handling:
- empty sum
- double sum
- big decimal sum
The problem is, I don't know if Flink can serialize such class hierarchy efficiently.
|
|
||
private def isForStandardDeviationInsteadOfBeingForVariance(): Boolean = { | ||
standardDeviationVarianceType match { | ||
case SampleStandardDeviation => true |
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.
We can remove ifs above and do sth like:
case PopulationVariance => populationVariance
case SampleVariance => sampleVariance(populationVariance)
case SampleStandardDeviation => MathUtils.largeFloatSqrt(populationVariance)
case PopulationStandardDeviation => MathUtils.largeFloatSqrt(sampleVariance(populationVariance))
?
Describe your changes
Checklist before merge