Add Variance and Standard Deviation Aggregation Functions #9910

snleee · 2022-12-05T21:15:00Z

This PR adds VAR_POP(), VAR_SAMP(), STDDEV_POP(), STDDEV_SAMP() aggregation functions.

codecov-commenter · 2022-12-06T08:51:35Z

Codecov Report

Merging #9910 (38c8821) into master (ecf41be) will increase coverage by 0.03%.
The diff coverage is 74.62%.

@@             Coverage Diff              @@
##             master    #9910      +/-   ##
============================================
+ Coverage     64.30%   64.33%   +0.03%     
- Complexity     5034     5531     +497     
============================================
  Files          1928     1931       +3     
  Lines        103874   104243     +369     
  Branches      15823    15882      +59     
============================================
+ Hits          66793    67067     +274     
- Misses        32254    32333      +79     
- Partials       4827     4843      +16

Flag	Coverage Δ
unittests1	`68.01% <74.62%> (+0.08%)`	⬆️
unittests2	`15.82% <0.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ion/utils/StatisticalAggregationFunctionUtils.java	`50.00% <50.00%> (ø)`
...inot/segment/local/customobject/VarianceTuple.java	`65.00% <65.00%> (ø)`
...gation/function/CovarianceAggregationFunction.java	`66.66% <66.66%> (+1.23%)`	⬆️
...org/apache/pinot/core/common/ObjectSerDeUtils.java	`91.43% <75.00%> (+0.70%)`	⬆️
...regation/function/VarianceAggregationFunction.java	`79.68% <79.68%> (ø)`
.../pinot/common/function/scalar/StringFunctions.java	`67.54% <100.00%> (+11.29%)`	⬆️
...gregation/function/AggregationFunctionFactory.java	`81.56% <100.00%> (+0.53%)`	⬆️
...che/pinot/segment/spi/AggregationFunctionType.java	`90.42% <100.00%> (+0.42%)`	⬆️
.../pinot/segment/spi/store/ColumnIndexDirectory.java	`28.57% <0.00%> (-11.43%)`	⬇️
...ache/pinot/segment/spi/store/SegmentDirectory.java	`45.45% <0.00%> (-10.11%)`	⬇️
... and 74 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jasperjiaguo

LGTM over all.

pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java

jasperjiaguo · 2022-12-07T00:18:07Z

pinot-core/src/test/java/org/apache/pinot/queries/VarianceQueriesTest.java

+    checkWithPrecisionForStandardDeviation((VarianceTuple) aggregationResult.get(7), NUM_RECORDS, expectedDoubleSum,
+        expectedStdDevs[7].getResult(), true);
+
+    // Update the expected result by 3 more times (broker query will compute 4 identical segments)


The variance on 4 identical segments is the same as variance on one of each segment. Should probably to consider using getDistinctInstances in BaseQueriesTest

@jasperjiaguo Can you elaborate on your suggestion?

I referred covariance test and it also uses getBrokerResponse() to get the result. According to the documentation, getBrokerResponse() seems to compute the result from 4 identical segments. FYI,getBrokerResponse() internally calls getDistinctInstances().

Yes, in CovarianceQueriesTest, @SabrinaZhaozyf has added the getDistinctInstances() to use 4 distinct segments for these statistical functions, because Covar on 4 identical segments is the same as Covar on one of each segment, as kind of defeats the purpose of testing merge logic over multiple segments. The getDistinctInstances in base class is still returning 4 identical segments though for backward compat. We can probably inherit CovarianceQueriesTest in VarianceQueriesTest.

@jasperjiaguo Thanks for pointing out. I actually merged Covariance & Variance tests. This actually helped me to identify a bug when we merging empty VarianceTuple with non empty VarianceTuple!

Can you go over the pr once more?

LGTM, thanks!

pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java

walterddr · 2022-12-07T00:58:24Z

.../main/java/org/apache/pinot/core/query/aggregation/function/VarianceAggregationFunction.java

+ * Sample Variances" by Chan et al. Please refer to the "Parallel Algorithm" section from the following wiki:
+ * - https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Parallel_algorithm
+ */
+public class VarianceAggregationFunction extends BaseSingleInputAggregationFunction<VarianceTuple, Double> {


majority of this code is shared with CoVariance agg. can we abstract out some comment utilities?

Interestingly, they look very similar but the implementations of all functions are slightly different due to the algorithm difference. Also, we use a different intermediate class VarianceTuple instead of CovarianceTuple.

I took out a common function that can be shared and created a util class. Please take a look

This PR adds `VAR_POP()`, `VAR_SAMP()`, `STDDEV_POP()`, `STDDEV_SAMP()` aggregation functions.

walterddr

lgtm overall. will let @Jackie-Jiang take another pass.

refactoring of taking out common utils can be done separately IMO.

walterddr · 2022-12-09T00:07:04Z

pinot-core/src/test/java/org/apache/pinot/queries/StatisticalQueriesTest.java

+  }
+
+  @Test
+  public void testStandardDeviationAggregationOnly() {


saw some instability on https://github.com/apache/pinot/actions/runs/3652424011/jobs/6170799903.

Should be fixed with #9948

snleee force-pushed the add-variance-function branch 3 times, most recently from b5fa523 to 30e7324 Compare December 6, 2022 08:04

snleee changed the title ~~[WIP] Add Variance and Standard Deviation Aggregation Functions~~ Add Variance and Standard Deviation Aggregation Functions Dec 6, 2022

snleee requested review from Jackie-Jiang, walterddr and siddharthteotia December 6, 2022 08:05

snleee force-pushed the add-variance-function branch from 30e7324 to de8dcc4 Compare December 6, 2022 08:12

snleee force-pushed the add-variance-function branch from de8dcc4 to 11cb6e2 Compare December 6, 2022 16:37

jasperjiaguo approved these changes Dec 7, 2022

View reviewed changes

walterddr reviewed Dec 7, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java Show resolved Hide resolved

walterddr reviewed Dec 7, 2022

View reviewed changes

snleee force-pushed the add-variance-function branch from 11cb6e2 to dfc4304 Compare December 7, 2022 06:54

Add Variance and Standard Deviation Aggregation Functions

ec1ff2b

This PR adds `VAR_POP()`, `VAR_SAMP()`, `STDDEV_POP()`, `STDDEV_SAMP()` aggregation functions.

snleee force-pushed the add-variance-function branch 2 times, most recently from bf2e529 to 01c4be6 Compare December 7, 2022 07:03

walterddr approved these changes Dec 7, 2022

View reviewed changes

jasperjiaguo mentioned this pull request Dec 7, 2022

Refactor covariance function to numerical stable version #9936

Open

Addressing comments

b70d7dd

snleee force-pushed the add-variance-function branch from 01c4be6 to b70d7dd Compare December 7, 2022 19:54

Merging unit tests

38c8821

snleee force-pushed the add-variance-function branch from 847996c to 38c8821 Compare December 8, 2022 01:23

snleee merged commit 852477b into master Dec 8, 2022

snleee deleted the add-variance-function branch December 8, 2022 20:51

snleee restored the add-variance-function branch December 8, 2022 22:31

snleee deleted the add-variance-function branch December 8, 2022 23:15

walterddr reviewed Dec 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Variance and Standard Deviation Aggregation Functions #9910

Add Variance and Standard Deviation Aggregation Functions #9910

snleee commented Dec 5, 2022

codecov-commenter commented Dec 6, 2022 •

edited

jasperjiaguo left a comment

jasperjiaguo Dec 7, 2022 •

edited

snleee Dec 7, 2022

jasperjiaguo Dec 7, 2022 •

edited

snleee Dec 8, 2022

jasperjiaguo Dec 8, 2022

walterddr Dec 7, 2022

snleee Dec 7, 2022

snleee Dec 7, 2022

walterddr left a comment

walterddr Dec 9, 2022

Jackie-Jiang Dec 9, 2022

Add Variance and Standard Deviation Aggregation Functions #9910

Add Variance and Standard Deviation Aggregation Functions #9910

Conversation

snleee commented Dec 5, 2022

codecov-commenter commented Dec 6, 2022 • edited

Codecov Report

jasperjiaguo left a comment

Choose a reason for hiding this comment

jasperjiaguo Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasperjiaguo Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 6, 2022 •

edited

jasperjiaguo Dec 7, 2022 •

edited

jasperjiaguo Dec 7, 2022 •

edited