Add hyperLogLogPlus aggregation function for distinct count #11346

deemoliu · 2023-08-14T17:17:24Z

feature: add DistinctCountHLLPlus function.

Context:
currently pinot is using Clearspring implementation of HLL algorithm. https://github.com/addthis/stream-lib/blob/master/src/main/java/com/clearspring/analytics/stream/cardinality/HyperLogLog.java
reference: HLL paper: https://algo.inria.fr/flajolet/Publications/FlFuGaMe07.pdf

we have some customers observed ElasticSearch dataset to Pinot, Pinot is using HLL algorithm while ES is using HLL++, user reported that HLL++ has higher accuracy than HLL when cardinality of dimension is at 10k-100k.
reference: HLL++ paper https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/40671.pdf

We tried to solve the above issue by tuning the log2m parameters of HLL (distinctCountHLL) functions, then we observed the CPU usage increased and bring the cluster into unstable state.

This PR tried to bridge the gap between ES and Pinot by introducing HyperLogLogPlusPlus to Pinot. Clearspring already has support for the HLL++. https://github.com/addthis/stream-lib/blob/master/src/main/java/com/clearspring/analytics/stream/cardinality/HyperLogLogPlus.java

Approximation Benchmark:

Test Query

SELECT
  DISTINCTCOUNTHLLPLUS(some_id, 12)
FROM
  test_pinot_table
where
  time_ms >= 1694380000000
  time_ms <= 1694382000000
  and filter1 > 0
  AND filter2 = 0
  AND filter3 > 0
  AND filter4 != TRUE

Approximation Benchmarking result

Result

Query1 Query2 Query3

Distinct Count 144730 2797 204807

HLL (default log2m=8) 158486 3138 191046

HLL log2m=9 155441 2850 209481

HLL log2m=10 148871 2797 204871

HLL++ default p=14 143821 2787 204371

hLL++ p=13 145069 2774 206735

HLL++ p=12 145329 2770 209274

HLL++ p=11 145251 2720 210161

HLL++ p=10 147969 2679 214758

HLL++ p=9 147234 2767 201620

HLL++ p=8 141377 2653 184344

codecov-commenter · 2023-08-14T22:47:01Z

Codecov Report

Merging #11346 (f0cf7f2) into master (2306298) will increase coverage by 0.02%.
Report is 4 commits behind head on master.
The diff coverage is 52.09%.

@@             Coverage Diff              @@
##             master   #11346      +/-   ##
============================================
+ Coverage     63.07%   63.10%   +0.02%     
- Complexity     1110     1121      +11     
============================================
  Files          2326     2342      +16     
  Lines        124918   125686     +768     
  Branches      19145    19306     +161     
============================================
+ Hits          78792    79311     +519     
- Misses        40510    40714     +204     
- Partials       5616     5661      +45

Flag	Coverage Δ
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`63.06% <52.09%> (+0.01%)`	⬆️
java-17	`62.89% <52.09%> (-0.04%)`	⬇️
java-20	`62.89% <52.09%> (+12.96%)`	⬆️
temurin	`63.10% <52.09%> (+0.02%)`	⬆️
unittests	`63.09% <52.09%> (+0.02%)`	⬆️
unittests1	`67.20% <52.09%> (-0.09%)`	⬇️
unittests2	`14.46% <0.00%> (-0.03%)`	⬇️

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

Files Changed	Coverage Δ
...rg/apache/pinot/core/plan/AggregationPlanNode.java	`72.46% <ø> (ø)`
...ggregator/DistinctCountHLLPlusValueAggregator.java	`0.00% <0.00%> (ø)`
...gment/local/aggregator/ValueAggregatorFactory.java	`81.25% <0.00%> (-5.42%)`	⬇️
...inot/segment/local/utils/HyperLogLogPlusUtils.java	`0.00% <0.00%> (ø)`
...va/org/apache/pinot/spi/utils/CommonConstants.java	`28.00% <ø> (ø)`
...he/pinot/segment/local/utils/CustomSerDeUtils.java	`37.97% <20.00%> (-2.61%)`	⬇️
...ion/DistinctCountHLLPlusMVAggregationFunction.java	`38.80% <38.80%> (ø)`
...ction/DistinctCountHLLPlusAggregationFunction.java	`47.64% <47.64%> (ø)`
...perator/query/NonScanBasedAggregationOperator.java	`77.87% <57.89%> (-4.04%)`	⬇️
...nction/FrequentLongsSketchAggregationFunction.java	`60.97% <60.97%> (ø)`
... and 9 more

... and 42 files with indirect coverage changes

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

deemoliu · 2023-09-11T22:45:26Z

@Jackie-Jiang @chenboat @cbalci can you please review this PR?

deemoliu · 2023-09-12T20:51:14Z

Test Query

SELECT
  DISTINCTCOUNTHLLPLUS(some_id, 12)
FROM
  test_pinot_table
where
  time_ms >= 1694380000000
  time_ms <= 1694382000000
  and filter1 > 0
  AND filter2 = 0
  AND filter3 > 0
  AND filter4 != TRUE

Approximation Benchmarking result

Result

Query1 Query2 Query3

Distinct Count 144730 2797 204807

HLL (default log2m=8) 158486 3138 191046

HLL log2m=9 155441 2850 209481

HLL log2m=10 148871 2797 204871

HLL++ default p=14 143821 2787 204371

hLL++ p=13 145069 2774 206735

HLL++ p=12 145329 2770 209274

HLL++ p=11 145251 2720 210161

HLL++ p=10 147969 2679 214758

HLL++ p=9 147234 2767 201620

HLL++ p=8 141377 2653 184344

chenboat · 2023-09-14T01:05:41Z

...rg/apache/pinot/core/query/aggregation/function/DistinctCountHLLPlusAggregationFunction.java

+  public DistinctCountHLLPlusAggregationFunction(List<ExpressionContext> arguments) {
+    super(arguments.get(0));
+    int numExpressions = arguments.size();
+    // This function expects 1 or 2 arguments.


update javadoc, should we expect 2 or 3 instead based on the code?

thank @chenboat for review. this function epxects 1 or 2 or 3 arguments.

siddharthteotia · 2023-09-14T20:29:18Z

I'd like to spend some time reviewing this today / tomorrow.

deemoliu · 2023-09-14T20:39:56Z

I'd like to spend some time reviewing this today / tomorrow.

@siddharthteotia thanks for reviewing. I will fix this conflict in this hour.

jackjlli

Thanks for working on this! Would you mind adding more info to this PR, like what functionalities are added, what's the difference between the raw one and the non-raw one, what the purpose of your benchmarking test is?

jackjlli · 2023-09-14T21:03:04Z

...core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java

+        }
+        return hllplus;
+      } catch (Exception e) {
+        throw new RuntimeException("Caught exception while merging HyperLogLogs", e);


nit: Caught exception while merging HyperLogLogsPlus

jackjlli · 2023-09-14T21:09:57Z

...main/java/org/apache/pinot/segment/local/aggregator/DistinctCountHLLPlusValueAggregator.java

+  public int getMaxAggregatedValueByteSize() {
+    // NOTE: For aggregated metrics, initial aggregated value might have not been generated. Returns the byte size
+    //       based on log2m.
+    return _maxByteSize > 0 ? _maxByteSize : 100000000;


This line doesn't seem to align with the comment above?

thanks @jackjlli for pointing out. let me uncomment the line below and remove this line.

Jackie-Jiang

LGTM with minor comments

Jackie-Jiang · 2023-09-12T21:19:24Z

...core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java

+        }
+        return hllplus;
+      } catch (Exception e) {
+        throw new RuntimeException("Caught exception while merging HyperLogLogs", e);


(minor) Revise the message

thanks @Jackie-Jiang i fixed the error message.

Jackie-Jiang · 2023-09-12T21:19:31Z

...core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java

+  private static HyperLogLogPlus getDistinctCountHLLPlusResult(Dictionary dictionary,
+      DistinctCountHLLPlusAggregationFunction function) {
+    if (dictionary.getValueType() == FieldSpec.DataType.BYTES) {
+      // Treat BYTES value as serialized HyperLogLog


(minor) Revise the comment

Jackie-Jiang · 2023-09-17T20:53:30Z

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

+
+    @Override
+    public HyperLogLogPlus deserialize(ByteBuffer byteBuffer) {
+      // NOTE: The passed in byte buffer is always BIG ENDIAN


(minor) Remove this comment as it doesn't apply

Jackie-Jiang · 2023-09-17T20:56:28Z

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/CustomSerDeUtils.java

+      try {
+        return HyperLogLogPlus.Builder.build(bytes);
+      } catch (IOException e) {
+        throw new RuntimeException("Caught exception while de-serializing HyperLogLogPlus", e);
+      }


Suggested change

try {

return HyperLogLogPlus.Builder.build(bytes);

} catch (IOException e) {

throw new RuntimeException("Caught exception while de-serializing HyperLogLogPlus", e);

}

return deserialize(bytes);

deemoliu · 2023-09-18T17:07:24Z

thanks @Jackie-Jiang @jackjlli @siddharthteotia and @chenboat for review. I will address comment today.

jackjlli

Minor but LGTM. Thanks for adding the aggregation function!

jackjlli · 2023-09-19T17:42:34Z

...rg/apache/pinot/core/query/aggregation/function/DistinctCountHLLPlusAggregationFunction.java

+      try {
+        HyperLogLogPlus hyperLogLogPlus = aggregationResultHolder.getResult();
+        if (hyperLogLogPlus != null) {
+          for (int i = 0; i < length; i++) {


nit: these two for loop can be merged. We just need to check if (hyperLogLogPlus == null). If yes, assign the first one to this reference. The following logic should be the same.

thanks for code review! addressed the code comment, please take a look

jackjlli · 2023-09-19T23:51:06Z

...rg/apache/pinot/core/query/aggregation/function/DistinctCountHLLPlusAggregationFunction.java

-            hyperLogLogPlus.addAll(ObjectSerDeUtils.HYPER_LOG_LOG_PLUS_SER_DE.deserialize(bytesValues[i]));
-          }
+        }
+        for (int i = 0; i < length; i++) {


nit: int i = 1;

jackjlli · 2023-09-20T00:45:59Z

...rg/apache/pinot/core/query/aggregation/function/DistinctCountHLLPlusAggregationFunction.java

+        if (hyperLogLogPlus == null) {
+          hyperLogLogPlus = ObjectSerDeUtils.HYPER_LOG_LOG_PLUS_SER_DE.deserialize(bytesValues[0]);
+          aggregationResultHolder.setValue(hyperLogLogPlus);
+        }


We need the else block if the check is not qualified:

} else { hyperLogLogPlus.addAll(ObjectSerDeUtils.HYPER_LOG_LOG_PLUS_SER_DE.deserialize(bytesValues[0])); }

got your point, updated, thanks!

deemoliu force-pushed the wip-hll-fix-test-failure branch from 32a26a0 to 5918003 Compare August 14, 2023 21:37

deemoliu mentioned this pull request Aug 14, 2023

Alternative HyperLogLog implementation with higher accuracy #4248

Closed

deemoliu force-pushed the wip-hll-fix-test-failure branch 3 times, most recently from c8948ae to 7cbea4a Compare September 8, 2023 00:48

deemoliu changed the title ~~[WIP] Add hyperLogLogPlus aggregation function for distinct count~~ Add hyperLogLogPlus aggregation function for distinct count Sep 11, 2023

chenboat reviewed Sep 14, 2023

View reviewed changes

jackjlli reviewed Sep 14, 2023

View reviewed changes

deemoliu force-pushed the wip-hll-fix-test-failure branch 2 times, most recently from 2a995c4 to 9308869 Compare September 14, 2023 22:25

Jackie-Jiang added feature documentation release-notes Referenced by PRs that need attention when compiling the next release notes labels Sep 17, 2023

Jackie-Jiang approved these changes Sep 17, 2023

View reviewed changes

jackjlli approved these changes Sep 19, 2023

View reviewed changes

jackjlli reviewed Sep 19, 2023

View reviewed changes

deemoliu added 2 commits September 19, 2023 17:03

Add hyperLogLogPlus aggregation function for distinct count

32ada40

address code comments

51b5cae

deemoliu force-pushed the wip-hll-fix-test-failure branch from 93ecb3b to 75da176 Compare September 20, 2023 00:30

jackjlli reviewed Sep 20, 2023

View reviewed changes

address code comments

f0cf7f2

deemoliu force-pushed the wip-hll-fix-test-failure branch from 75da176 to f0cf7f2 Compare September 20, 2023 20:37

chenboat merged commit 3501b86 into apache:master Sep 21, 2023
20 of 21 checks passed

eaugene mentioned this pull request Sep 24, 2023

Adding query function override for Aggregate functions of multi valued column #11307

Merged

deemoliu mentioned this pull request Dec 27, 2023

Add pinot docs for HyperLogLogPlus pinot-contrib/pinot-docs#270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hyperLogLogPlus aggregation function for distinct count #11346

Add hyperLogLogPlus aggregation function for distinct count #11346

deemoliu commented Aug 14, 2023 •

edited

Loading

codecov-commenter commented Aug 14, 2023 •

edited

Loading

deemoliu commented Sep 11, 2023

deemoliu commented Sep 12, 2023

chenboat Sep 14, 2023

deemoliu Sep 14, 2023

siddharthteotia commented Sep 14, 2023

deemoliu commented Sep 14, 2023

jackjlli left a comment

jackjlli Sep 14, 2023

jackjlli Sep 14, 2023

deemoliu Sep 14, 2023

Jackie-Jiang left a comment

Jackie-Jiang Sep 12, 2023

deemoliu Sep 19, 2023

Jackie-Jiang Sep 12, 2023

Jackie-Jiang Sep 17, 2023

Jackie-Jiang Sep 17, 2023

deemoliu commented Sep 18, 2023

jackjlli left a comment

jackjlli Sep 19, 2023

deemoliu Sep 19, 2023

jackjlli Sep 19, 2023

jackjlli Sep 20, 2023

deemoliu Sep 20, 2023

	Query1	Query2	Query3
Distinct Count	144730	2797	204807
HLL (default log2m=8)	158486	3138	191046
HLL log2m=9	155441	2850	209481
HLL log2m=10	148871	2797	204871
HLL++ default p=14	143821	2787	204371
hLL++ p=13	145069	2774	206735
HLL++ p=12	145329	2770	209274
HLL++ p=11	145251	2720	210161
HLL++ p=10	147969	2679	214758
HLL++ p=9	147234	2767	201620
HLL++ p=8	141377	2653	184344

Add hyperLogLogPlus aggregation function for distinct count #11346

Add hyperLogLogPlus aggregation function for distinct count #11346

Conversation

deemoliu commented Aug 14, 2023 • edited Loading

codecov-commenter commented Aug 14, 2023 • edited Loading

Codecov Report

deemoliu commented Sep 11, 2023

deemoliu commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthteotia commented Sep 14, 2023

deemoliu commented Sep 14, 2023

jackjlli left a 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

Jackie-Jiang left a 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

Choose a reason for hiding this comment

deemoliu commented Sep 18, 2023

jackjlli left a 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

Choose a reason for hiding this comment

deemoliu commented Aug 14, 2023 •

edited

Loading

codecov-commenter commented Aug 14, 2023 •

edited

Loading