Add PercentileKLL aggregation function by cbalci · Pull Request #10643 · apache/pinot

cbalci · 2023-04-19T05:30:54Z

Introducing a new approximate percentile calculation function PercentileKLL and its variations (MV & Raw), using Apache Datasketches libraries 'KLL'.

This is part of a proposal to improve Apache Datasketches support in Pinot:
(Google Docs Link) [Proposal] Improved Apache DataSketches Support in Pinot

Some advantages listed and discussed in the linked document:

Well defined error bound (comparison to t-Digest)
Faster updates, serialization/deserialization
Binary compatibility with external systems, hence the ability to use Pinot as sketch store
Ability to compute ‘Rank’ and ‘Histogram’ besides ‘Percentile’
Feature parity with Druid

Please leave design related comments on the linked document and code related comments in this PR.

Testing

Added unit tests to cover basic use cases that call PercentileKLL, PercentileKLLMV, PercentileRawKLL, PercentileRawKLLMV
Added tests to cover group by scenarios
Manually tested ingesting raw (externally generated) data sketches

feature performance

codecov-commenter · 2023-04-19T06:29:20Z

Codecov Report

Merging #10643 (396e2c1) into master (579082c) will decrease coverage by 50.75%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master   #10643       +/-   ##
=============================================
- Coverage     64.40%   13.66%   -50.75%     
+ Complexity     6446      439     -6007     
=============================================
  Files          2098     2103        +5     
  Lines        113315   113521      +206     
  Branches      17204    17255       +51     
=============================================
- Hits          72984    15511    -57473     
- Misses        35082    96740    +61658     
+ Partials       5249     1270     -3979

Flag	Coverage Δ
unittests1	`?`
unittests2	`13.66% <0.00%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
...org/apache/pinot/core/common/ObjectSerDeUtils.java	`0.00% <0.00%> (-90.68%)`	⬇️
...gregation/function/AggregationFunctionFactory.java	`0.00% <0.00%> (-83.34%)`	⬇️
...ion/function/PercentileKLLAggregationFunction.java	`0.00% <0.00%> (ø)`
...n/function/PercentileKLLMVAggregationFunction.java	`0.00% <0.00%> (ø)`
.../function/PercentileRawKLLAggregationFunction.java	`0.00% <0.00%> (ø)`
...unction/PercentileRawKLLMVAggregationFunction.java	`0.00% <0.00%> (ø)`
...inot/segment/local/customobject/SerializedKLL.java	`0.00% <0.00%> (ø)`
...che/pinot/segment/spi/AggregationFunctionType.java	`0.00% <0.00%> (-91.59%)`	⬇️

... and 1444 files with indirect coverage changes

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

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

.../java/org/apache/pinot/core/query/aggregation/function/PercentileKLLAggregationFunction.java

chiquelo · 2023-04-26T20:36:12Z

.../java/org/apache/pinot/core/query/aggregation/function/PercentileKLLAggregationFunction.java

+
+    _percentile = arguments.get(1).getLiteral().getDoubleValue();
+    if (numArguments == 3) {
+      _kValue = arguments.get(2).getLiteral().getIntValue();


A similar check should be done on the K value. There's could be some memory implications from setting the K to be too high: https://datasketches.apache.org/docs/KLL/KLLAccuracyAndSize.html

The library already returns a good exception (SketchesArgumentException: K must be >= 8 and <= 65535), so I didn't see much value in doing another check here.

For the memory implication, I think it should be up to the user to decide if the size of the sketch is going to be a problem.

Makes sense. Did not know about SketchesArgumentException. That should be good enough if bubbled up properly

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

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java

.../java/org/apache/pinot/core/query/aggregation/function/PercentileKLLAggregationFunction.java

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

chiquelo

lgtm

chiquelo · 2023-05-02T19:19:49Z

.../java/org/apache/pinot/core/query/aggregation/function/PercentileKLLAggregationFunction.java

+
+    _percentile = arguments.get(1).getLiteral().getDoubleValue();
+    if (numArguments == 3) {
+      _kValue = arguments.get(2).getLiteral().getIntValue();


Makes sense. Did not know about SketchesArgumentException. That should be good enough if bubbled up properly

nklijia2011 · 2023-05-04T18:02:27Z

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

+ * Compares two sketches for a specific percentile value.
+ */
+public class SerializedKLL implements Comparable<SerializedKLL> {
+  private final double _quantile;


why do we need _quantile inside this SerializedKLL?
if we want to return the KllDoublesSketch as byte[], the toString() is for that purpose?

Good question. That is used when we want to sort (ORDER BY) sketches.

navina · 2023-05-17T21:09:20Z

@cbalci / @chenboat : has this feature been documented ? is it available in the OSS docs?

cbalci · 2023-05-17T21:58:32Z

@navina, not yet. I'll add it to OSS documentation soon.

cbalci added 3 commits April 17, 2023 15:41

Add KllAggregationFunction for efficient percentile calculation

235d8f1

Merge branch 'apache:master' into kll-aggregation-function

8b34ee7

Fix test

9ff268c

mayankshriv requested a review from Jackie-Jiang April 19, 2023 20:54

chiquelo suggested changes Apr 26, 2023

View reviewed changes

chenboat reviewed Apr 27, 2023

View reviewed changes

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java Show resolved Hide resolved

chenboat reviewed Apr 27, 2023

View reviewed changes

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java Show resolved Hide resolved

chenboat reviewed Apr 27, 2023

View reviewed changes

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

chenboat reviewed Apr 27, 2023

View reviewed changes

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

Address review comments

3888866

cbalci requested review from chenboat and chiquelo May 1, 2023 21:47

cbalci added 2 commits May 1, 2023 14:49

Merge branch 'master' into kll-aggregation-function

39fa2f2

Fix merge issue

a18ab96

chiquelo approved these changes May 2, 2023

View reviewed changes

nklijia2011 reviewed May 4, 2023

View reviewed changes

chenboat approved these changes May 12, 2023

View reviewed changes

Merge branch 'master' into kll-aggregation-function

396e2c1

chenboat merged commit 41fbb32 into apache:master May 16, 2023

cbalci mentioned this pull request Jul 5, 2023

Add documentation for PercentileKLL and its variants pinot-contrib/pinot-docs#206

Merged

Conversation

cbalci commented Apr 19, 2023

Uh oh!

codecov-commenter commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

chiquelo Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

cbalci Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiquelo May 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chiquelo left a comment

Choose a reason for hiding this comment

Uh oh!

chiquelo May 2, 2023

Choose a reason for hiding this comment

Uh oh!

nklijia2011 May 4, 2023

Choose a reason for hiding this comment

Uh oh!

cbalci May 4, 2023

Choose a reason for hiding this comment

Uh oh!

navina commented May 17, 2023

Uh oh!

cbalci commented May 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Apr 19, 2023 •

edited

Loading

cbalci Apr 30, 2023 •

edited

Loading