Add Support for Covariance Function #9236

SabrinaZhaozyf · 2022-08-17T22:55:45Z

This PR adds support for COVAR_POP and COVAR_SAMP function to Pinot. These functions only work on numerical SV columns.

Label: feature
Issue: #8493
Use Cases

covar_pop(x, y) returns the population covariance(formula: sum[(x_i - E(X))(y_i - E(Y))] / n, type: DOUBLE) of the input values.
covar_samp(x, y) returns the sample covariance(formula: sum[(x_i - E(X))(y_i - E(Y))] / (n - 1), type: DOUBLE) of the input values.

Note

Current tests check against Java's statistical functions
. If any of the input columns has type float, it will be read as double which can lead to discrepancy with Java's result due to loss of precision during type conversion between float and double.

Future

Implementation can be extended to calculate Pearson's coefficient by adding extra states to CovarianceTuple such as sum(x^2), sum(y^y), etc.
Implementation for calculating variance and standard deviation should also be similar.

…before testing invalid inputs

codecov-commenter · 2022-08-23T01:07:25Z

Codecov Report

Merging #9236 (49d5929) into master (e8a8684) will decrease coverage by 1.35%.
The diff coverage is 63.34%.

❗ Current head 49d5929 differs from pull request most recent head b67b6d6. Consider uploading reports for the commit b67b6d6 to get more accurate results

@@             Coverage Diff              @@
##             master    #9236      +/-   ##
============================================
- Coverage     69.99%   68.64%   -1.36%     
- Complexity     4682     5007     +325     
============================================
  Files          1848     1864      +16     
  Lines         98572    99469     +897     
  Branches      14967    15133     +166     
============================================
- Hits          68999    68279     -720     
- Misses        24723    26285    +1562     
- Partials       4850     4905      +55

Flag	Coverage Δ
integration1	`26.19% <23.06%> (-0.16%)`	⬇️
integration2	`?`
unittests1	`67.09% <64.45%> (+0.07%)`	⬆️
unittests2	`15.29% <17.92%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
...apache/pinot/broker/api/HttpRequesterIdentity.java	`28.57% <0.00%> (-57.15%)`	⬇️
...org/apache/pinot/broker/api/RequesterIdentity.java	`50.00% <0.00%> (-50.00%)`	⬇️
.../pinot/broker/api/resources/PinotBrokerLogger.java	`0.00% <0.00%> (ø)`
...mmon/assignment/InstanceAssignmentConfigUtils.java	`14.63% <0.00%> (-0.76%)`	⬇️
...he/pinot/common/assignment/InstancePartitions.java	`40.00% <0.00%> (-1.18%)`	⬇️
...ot/common/function/scalar/ArithmeticFunctions.java	`66.66% <0.00%> (-2.90%)`	⬇️
...t/common/response/broker/BrokerResponseNative.java	`96.44% <ø> (ø)`
...er/api/access/ZkBasicAuthAccessControlFactory.java	`0.00% <ø> (ø)`
...ache/pinot/controller/api/resources/Constants.java	`42.10% <ø> (ø)`
...er/api/resources/LLCSegmentCompletionHandlers.java	`62.37% <ø> (ø)`
... and 306 more

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

...segment-local/src/main/java/org/apache/pinot/segment/local/customobject/CovarianceTuple.java

siddharthteotia

Does this work on both SV and MV columns ?

siddharthteotia · 2022-08-23T16:27:13Z

Please update the PR description with a brief note on what the new function is about, what it does, how to use etc.

jasperjiaguo

Maybe also mention the precision issue with float and further extension to correlation?
Also probably paste a short description of population covariance vs sample covariance in the description (and future docs)(https://en.wikipedia.org/wiki/Bessel%27s_correction)

Minor but LGTM

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

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

amrishlal · 2022-08-23T21:04:04Z

Can you please clarify or add a link in the description to the formula you are using to calculate covariance? I don't see "average value" being used in your calculation? It will also be good to add support for "covariance of sample" (which is probably more commonly used) along with "covariance of population"

jasperjiaguo · 2022-08-23T21:35:01Z

Can you please clarify or add a link in the description to the formula you are using to calculate covariance? I don't see "average value" being used in your calculation? It will also be good to add support for "covariance of sample" (which is probably more commonly used) along with "covariance of population"

Yes. This should be simple if we add a flag for switching Bessel's correction in extract result. We probably want to add that @SabrinaZhaozyf ?

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java

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

siddharthteotia

LGTM

Will wait for @jasperjiaguo to take another look before merging.

jasperjiaguo

LGTM, just the previous minor comment on doc

walterddr

lgtm overall. minor comments

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java

...segment-local/src/main/java/org/apache/pinot/segment/local/customobject/CovarianceTuple.java

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java

SabrinaZhaozyf added 6 commits August 11, 2022 14:17

finished simple cov, before testing, before group by

2484144

added inner segment tests, before fixing precision issues with floats

d8b925e

declared precision var

bf4fed1

aligned tests type with aggfunc to cast to double before summing

75bbdd9

added inter seg tests

ac28576

finished aggregation group by, added tests for aggregation group by, …

5a60db9

…before testing invalid inputs

siddharthteotia added the feature label Aug 17, 2022

SabrinaZhaozyf added 8 commits August 18, 2022 10:10

added tear down

097fe8a

style

7c73fdd

stylish

bcdb6c5

added distinct inter seg tests

a337fc8

style fix

08f31c1

added comments for test clarity

d410281

fixed typo

4c603c5

added test with filter

f6ddf41

SabrinaZhaozyf changed the title ~~[WIP] Add Support for Covariance Function~~ Add Support for Covariance Function Aug 23, 2022

SabrinaZhaozyf marked this pull request as ready for review August 23, 2022 00:31

filter

3507ccd

siddharthteotia reviewed Aug 23, 2022

View reviewed changes

...segment-local/src/main/java/org/apache/pinot/segment/local/customobject/CovarianceTuple.java Show resolved Hide resolved

siddharthteotia reviewed Aug 23, 2022

View reviewed changes

jasperjiaguo reviewed Aug 23, 2022

View reviewed changes

pinot-core/src/test/java/org/apache/pinot/queries/BaseQueriesTest.java Show resolved Hide resolved

pinot-core/src/test/java/org/apache/pinot/queries/CovarianceQueriesTest.java Outdated Show resolved Hide resolved

walterddr reviewed Aug 23, 2022

View reviewed changes

pinot-core/src/test/java/org/apache/pinot/queries/CovarianceQueriesTest.java Outdated Show resolved Hide resolved

SabrinaZhaozyf added 3 commits August 23, 2022 15:09

added covar_samp

7ddf6fe

before fixing basequerytests

53edf11

cleaned distinct instance tests

eac3f42

jasperjiaguo reviewed Aug 24, 2022

View reviewed changes

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Outdated Show resolved Hide resolved

SabrinaZhaozyf added 2 commits August 23, 2022 17:21

add bessel corrector

f9def14

i love math

30da45f

siddharthteotia reviewed Aug 24, 2022

View reviewed changes

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Show resolved Hide resolved

amrishlal reviewed Aug 24, 2022

View reviewed changes

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Show resolved Hide resolved

amrishlal reviewed Aug 24, 2022

View reviewed changes

pinot-core/src/test/java/org/apache/pinot/queries/BaseQueriesTest.java Show resolved Hide resolved

SabrinaZhaozyf added 2 commits August 24, 2022 13:59

added javadoc on formula, addressed comments

535f302

got rid of duplicate comments

2ee0a14

jasperjiaguo reviewed Aug 24, 2022

View reviewed changes

pinot-core/src/test/java/org/apache/pinot/queries/BaseQueriesTest.java Show resolved Hide resolved

siddharthteotia approved these changes Aug 24, 2022

View reviewed changes

jasperjiaguo approved these changes Aug 24, 2022

View reviewed changes

walterddr reviewed Aug 24, 2022

View reviewed changes

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Outdated Show resolved Hide resolved

...segment-local/src/main/java/org/apache/pinot/segment/local/customobject/CovarianceTuple.java Show resolved Hide resolved

walterddr reviewed Aug 24, 2022

View reviewed changes

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Outdated Show resolved Hide resolved

...ain/java/org/apache/pinot/core/query/aggregation/function/CovarianceAggregationFunction.java Outdated Show resolved Hide resolved

updated javadoc for best test guidance

80f779a

SabrinaZhaozyf mentioned this pull request Aug 24, 2022

[Feature] Support for Correlation Function #9277

Open

SabrinaZhaozyf requested a review from walterddr August 24, 2022 22:16

SabrinaZhaozyf added 3 commits August 25, 2022 11:19

reduced division, handled 0 case

4eae910

trigger test

39249fa

trigger test

b67b6d6

siddharthteotia merged commit 88bb40c into apache:master Aug 25, 2022

siddharthteotia mentioned this pull request Oct 14, 2022

Support COVARIANCE functions #9595

Closed

walterddr mentioned this pull request Nov 28, 2022

support BOOL_AND and BOOL_OR aggregate functions #9848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Covariance Function #9236

Add Support for Covariance Function #9236

SabrinaZhaozyf commented Aug 17, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading

siddharthteotia left a comment

siddharthteotia commented Aug 23, 2022

jasperjiaguo left a comment •

edited

Loading

amrishlal commented Aug 23, 2022

jasperjiaguo commented Aug 23, 2022 •

edited

Loading

siddharthteotia left a comment

jasperjiaguo left a comment

walterddr left a comment

Add Support for Covariance Function #9236

Add Support for Covariance Function #9236

Conversation

SabrinaZhaozyf commented Aug 17, 2022 • edited Loading

codecov-commenter commented Aug 23, 2022 • edited Loading

Codecov Report

siddharthteotia left a comment

Choose a reason for hiding this comment

siddharthteotia commented Aug 23, 2022

jasperjiaguo left a comment • edited Loading

Choose a reason for hiding this comment

amrishlal commented Aug 23, 2022

jasperjiaguo commented Aug 23, 2022 • edited Loading

siddharthteotia left a comment

Choose a reason for hiding this comment

jasperjiaguo left a comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

SabrinaZhaozyf commented Aug 17, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading

jasperjiaguo left a comment •

edited

Loading

jasperjiaguo commented Aug 23, 2022 •

edited

Loading