You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "praveenc7 (via GitHub)" <gi...@apache.org> on 2025/12/22 21:25:27 UTC
[PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
praveenc7 opened a new pull request, #17411:
URL: https://github.com/apache/pinot/pull/17411
## Summary
ISSUE=https://github.com/apache/pinot/issues/17336
For dictionary-encoded columns, DISTINCT_COUNT_SMART_HLL currently uses a RoaringBitmap to deduplicate dictionary IDs before feeding values into HLL. While efficient for low cardinality, this approach becomes CPU-intensive for high cardinality (hundreds of thousands to millions of distinct values), where RoaringBitmap insertions dominate query execution time and negate the benefits of HLL.
<img width="1560" height="785" alt="Screenshot 2025-12-16 at 3 59 43 PM" src="https://github.com/user-attachments/assets/2e4ea2b2-a594-4a3b-9b16-5af726b8d953" />
## Proposal
Introduce a cardinality-aware execution path for DISTINCT_COUNT_HLL:
- Low cardinality → continue using RoaringBitmap (exact deduplication, memory-efficient)
- High cardinality → bypass RoaringBitmap and update HLL directly
### Observed improvements
- Reduces server-side CPU time by ~4x - 10× for high-cardinality queries (observed improvements from ~8s → ~700ms in prod benchmarks).
## Testing Done
Added JMH benchmark covering:
This JMH benchmark isolates server-side aggregation cost for the DistinctCountHLLAggregationFunction under controlled parameters: Each variation was run for 10 minutes
recordCount: {100K, 500K, 1M, 5M, 10M, 25M}
cardinalityRatioPercent: {1, 10, 30, 50, 80, 100} → Creates a record with configured cardinality
useRoaringBitMap/HLL -> Controls on to run the test with useRoaringBitMap or HLL
DictIds are pre-generated so benchmark timing includes only aggregation, not data generation.
Sample plots :
<img width="628" height="391" alt="Screenshot 2025-12-16 at 3 58 39 PM" src="https://github.com/user-attachments/assets/fcb37396-1f21-4f71-8fef-fa12f0dad255" />
Flame graph after optimization : Aggregate doesn't dominate CPU
<img width="1537" height="836" alt="Screenshot 2025-12-16 at 4 01 42 PM" src="https://github.com/user-attachments/assets/8edcc39c-2539-479b-a180-f16b0e2b77f5" />
## Benchmark Results (Average Latency, ms/op)
### Record Count = 100,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 1,000 | 0.6 | 0.80 |
| 10,000 | 0.71 | 0.87 |
| 30,000 | 0.89 | 0.90 |
| 50,000 | 1.05 | 0.96 |
| 80,000 | 1.79 | 1.00 |
| 100,000 | 1.91 | 1.05 |
### Record Count = 500,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 5,000 | 1.45 | 2.85 |
| 50,000 | 2.36 | 2.92 |
| 150,000 | 5.53 | 3.16 |
| 250,000 | 7.26 | 3.18 |
| 400,000 | 9.59 | 3.18 |
| 500,000 | 10.69 | 3.17 |
### Record Count = 1,000,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 10,000 | 2.53 | 5.36 |
| 100,000 | 6.69 | 5.44 |
| 300,000 | 13.12 | 5.80 |
| 500,000 | 15.92 | 5.78 |
| 800,000 | 19.84 | 5.78 |
| 1,000,000 | 22.11 | 5.71 |
### Record Count = 5,000,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 50,000 | 11.51 | 25.12 |
| 500,000 | 53.62 | 25.29 |
| 1,500,000 | 75.60 | 26.13 |
| 2,500,000 | 92.91 | 25.53 |
| 4,000,000 | 113.21 | 25.24 |
| 5,000,000 | 129.34 | 25.79 |
### Record Count = 10,000,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 100,000 | 52.98 | 50.64 |
| 1,000,000 | 117.68 | 50.61 |
| 3,000,000 | 161.56 | 50.08 |
| 5,000,000 | 206.71 | 51.14 |
| 8,000,000 | 248.77 | 50.01 |
| 10,000,000 | 278.78 | 50.37 |
### Record Count = 25,000,000
| Cardinality | RoaringBitmap | Direct HLL |
|---:|---:|---:|
| 250,000 | 199.06 | 125.82 |
| 2,500,000 | 348.39 | 126.40 |
| 7,500,000 | 466.14 | 124.74 |
| 12,500,000 | 555.77 | 124.35 |
| 20,000,000 | 679.43 | 124.99 |
### Recommendation:
Based on the micro-benchmark results across record counts and cardinalities, 100K distinct values is a good default threshold to start with for switching away from the RoaringBitmap path. At this scale, RoaringBitmap remains efficient for low-cardinality cases, while higher cardinalities already show clear benefits from using direct HLL updates. This threshold provides a safe balance between preserving deduplication benefits for low cardinality and avoiding excessive bitmap maintenance cost for high-cardinality workloads
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3716407248
> also please add a release note for this enhancement
Sure will add one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17411:
URL: https://github.com/apache/pinot/pull/17411#discussion_r2663893110
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountSmartHLLAggregationFunction.java:
##########
@@ -89,22 +95,22 @@ public AggregationFunctionType getType() {
@Override
public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
- Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
Review Comment:
revert this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17411:
URL: https://github.com/apache/pinot/pull/17411#discussion_r2663884853
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseDistinctCountSmartSketchAggregationFunction.java:
##########
@@ -670,6 +690,62 @@ protected static Set convertToValueSet(DictIdsWrapper dictIdsWrapper) {
}
}
+ /**
+ * Aggregate a single dictionary ID for a group. If already a sketch, offer directly.
+ * Otherwise add to bitmap and track as modified.
+ */
+ private void aggregateDictIdForGroup(GroupByResultHolder groupByResultHolder, int groupKey, Dictionary dictionary,
+ int dictId, IntSet modifiedGroups) {
+ Object result = groupByResultHolder.getResult(groupKey);
+ if (!(result instanceof DictIdsWrapper)) {
+ // Already converted to sketch, offer directly
+ ((HyperLogLog) result).offer(dictionary.get(dictId));
Review Comment:
The code assumes any result that is not a `DictIdsWrapper` is a `HyperLogLog`, but `result` could be null on first access for a group. This will cause a NullPointerException. Add a null check and initialize the result if it's null before casting.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountSmartHLLAggregationFunctionTest.java:
##########
@@ -0,0 +1,179 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.aggregation.function;
+
+import com.clearspring.analytics.stream.cardinality.HyperLogLog;
+import java.util.List;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+
+public class DistinctCountSmartHLLAggregationFunctionTest {
+
+ @Test
+ public void testParameterParsing() {
+ // Test default values
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ // Test individual parameters
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "threshold=50000")));
+ assertEquals(function.getThreshold(), 50_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "log2m=8")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=50000")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 50_000);
+
+ // Test disabled dictThreshold (non-positive values)
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=-1")));
+ assertEquals(function.getDictIdCardinalityThreshold(), Integer.MAX_VALUE);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=0")));
+ assertEquals(function.getDictIdCardinalityThreshold(), Integer.MAX_VALUE);
+
+ // Test multiple parameters together
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "threshold=200000;log2m=10;dictThreshold=150000")));
+ assertEquals(function.getThreshold(), 200_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 150_000);
+
+ // Test parameter order independence
+ DistinctCountSmartHLLAggregationFunction function1 = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=50000;threshold=100000;log2m=8")));
+ DistinctCountSmartHLLAggregationFunction function2 = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "log2m=8;dictThreshold=50000;threshold=100000")));
+ assertEquals(function1.getThreshold(), function2.getThreshold());
+ assertEquals(function1.getDictIdCardinalityThreshold(), function2.getDictIdCardinalityThreshold());
+
+ // Test legacy parameter names
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "hllConversionThreshold=50000;hllLog2m=10")));
+ assertEquals(function.getThreshold(), 50_000);
+
+ // Test case-insensitive parameters
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "THRESHOLD=50000;LOG2M=8;DICTTHRESHOLD=100000")));
+ assertEquals(function.getThreshold(), 50_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+ }
+
+ @Test
+ public void testFunctionMetadata() {
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+
+ // Test function type
+ assertEquals(function.getType().getName(), "distinctCountSmartHLL");
+
+ // Test result types
+ assertEquals(function.getIntermediateResultColumnType(), DataSchema.ColumnDataType.OBJECT);
+ assertEquals(function.getFinalResultColumnType(), DataSchema.ColumnDataType.INT);
+
+ // Test result holder creation
+ assertNotNull(function.createAggregationResultHolder());
+ assertNotNull(function.createGroupByResultHolder(10, 100));
+ }
+
+ @Test
+ public void testHLLOperations() {
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+
+ // Test merge final results (should sum)
+ Integer finalResult = function.mergeFinalResult(100, 200);
+ assertEquals(finalResult.intValue(), 300);
+
+ // Test extract final result (HLL cardinality)
+ HyperLogLog hll = new HyperLogLog(12);
+ for (int i = 0; i < 1000; i++) {
+ hll.offer(i);
+ }
+ Long cardinality = Long.valueOf(function.extractFinalResult(hll));
Review Comment:
Replace `Long.valueOf()` with direct cast `(Long)` since `extractFinalResult()` already returns a Long object. Using `Long.valueOf()` on an object is unnecessary and could cause a NullPointerException if the result is null.
```suggestion
Long cardinality = (Long) function.extractFinalResult(hll);
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountSmartHLLAggregationFunction.java:
##########
@@ -51,13 +51,17 @@
* - threshold: Threshold of the number of distinct values to trigger the conversion, 100_000 by default. Non-positive
* value means never convert.
* - log2m: Log2m for the converted HyperLogLog, 12 by default.
- * Example of second argument: 'threshold=10;log2m=8'
+ * - dictThreshold: Threshold for dictionary-encoded columns to trigger early conversion from RoaringBitmap to HLL
+ * during aggregation. 100_000 by default. Set to Integer.MAX_VALUE to disable and convert only
+ * at finalization.
Review Comment:
The documentation should clarify that non-positive values (≤0) are also treated as disabled and converted to Integer.MAX_VALUE, consistent with the implementation in the Parameters class where values ≤0 are set to Integer.MAX_VALUE.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseDistinctCountSmartSketchAggregationFunction.java:
##########
@@ -670,6 +690,62 @@ protected static Set convertToValueSet(DictIdsWrapper dictIdsWrapper) {
}
}
+ /**
+ * Aggregate a single dictionary ID for a group. If already a sketch, offer directly.
+ * Otherwise add to bitmap and track as modified.
+ */
+ private void aggregateDictIdForGroup(GroupByResultHolder groupByResultHolder, int groupKey, Dictionary dictionary,
+ int dictId, IntSet modifiedGroups) {
+ Object result = groupByResultHolder.getResult(groupKey);
+ if (!(result instanceof DictIdsWrapper)) {
+ // Already converted to sketch, offer directly
+ ((HyperLogLog) result).offer(dictionary.get(dictId));
+ } else {
+ // Still using bitmap, add dict ID
+ getDictIdBitmap(groupByResultHolder, groupKey, dictionary).add(dictId);
+ modifiedGroups.add(groupKey);
+ }
+ }
+
+ /**
+ * Aggregate multiple dictionary IDs for a group. If already a sketch, offer directly.
+ * Otherwise add to bitmap and track as modified.
+ */
+ private void aggregateDictIdsForGroup(GroupByResultHolder groupByResultHolder, int groupKey, Dictionary dictionary,
+ int[] dictIds, IntSet modifiedGroups) {
+ Object result = groupByResultHolder.getResult(groupKey);
+ if (!(result instanceof DictIdsWrapper)) {
+ // Already converted to sketch, offer directly
+ for (int dictId : dictIds) {
+ ((HyperLogLog) result).offer(dictionary.get(dictId));
+ }
Review Comment:
The code assumes any result that is not a `DictIdsWrapper` is a `HyperLogLog`, but `result` could be null on first access for a group. This will cause a NullPointerException. Add a null check and initialize the result if it's null before casting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3713439713
also please add a release note for this enhancement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3717501228
> overall lgtm!
>
> One question, how was the default of 100_000 chosen as the threshold? Is that a desirable default for all queries going forward? Are there any scenarios where using the Integer.MAX_VALUE may be preferred? It'll be good to call this out in documentation as a guide for users
>
> Also, can you take care of updating OSS documentation to reflect these changes? Thanks!
Thanks for the review @somandal
On the 100,000 threshold?
I chose 100K based on a combination of (a) micro-benchmarking across multiple recordCount sizes and cardinality ratios, and (b) validating behavior on the perf cluster with representative query patterns. The crossover point varies a bit by record count/cardinality ratio, but ~100K is a consistent “safe default” where the direct HLL path stops regressing and starts outperforming the RoaringBitmap-heavy path for workloads. (I included representative benchmark tables in the Testing section)
When would Integer.MAX_VALUE be preferred?
With the adaptive threshold in place, I don’t see a strong need to use Integer.MAX_VALUE in normal scenarios. The intent of the change is precisely to handle both low- and high-cardinality cases automatically. Added it as a guardrail if something odd shows up in production
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal merged PR #17411:
URL: https://github.com/apache/pinot/pull/17411
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on code in PR #17411:
URL: https://github.com/apache/pinot/pull/17411#discussion_r2666286576
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseDistinctCountSmartSketchAggregationFunction.java:
##########
@@ -670,6 +690,62 @@ protected static Set convertToValueSet(DictIdsWrapper dictIdsWrapper) {
}
}
+ /**
+ * Aggregate a single dictionary ID for a group. If already a sketch, offer directly.
+ * Otherwise add to bitmap and track as modified.
+ */
+ private void aggregateDictIdForGroup(GroupByResultHolder groupByResultHolder, int groupKey, Dictionary dictionary,
+ int dictId, IntSet modifiedGroups) {
+ Object result = groupByResultHolder.getResult(groupKey);
+ if (!(result instanceof DictIdsWrapper)) {
+ // Already converted to sketch, offer directly
+ ((HyperLogLog) result).offer(dictionary.get(dictId));
Review Comment:
Good point, it can be null for invalid groupKey. Updating it
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountSmartHLLAggregationFunctionTest.java:
##########
@@ -0,0 +1,179 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.aggregation.function;
+
+import com.clearspring.analytics.stream.cardinality.HyperLogLog;
+import java.util.List;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+
+public class DistinctCountSmartHLLAggregationFunctionTest {
+
+ @Test
+ public void testParameterParsing() {
+ // Test default values
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ // Test individual parameters
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "threshold=50000")));
+ assertEquals(function.getThreshold(), 50_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "log2m=8")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=50000")));
+ assertEquals(function.getThreshold(), 100_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 50_000);
+
+ // Test disabled dictThreshold (non-positive values)
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=-1")));
+ assertEquals(function.getDictIdCardinalityThreshold(), Integer.MAX_VALUE);
+
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=0")));
+ assertEquals(function.getDictIdCardinalityThreshold(), Integer.MAX_VALUE);
+
+ // Test multiple parameters together
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "threshold=200000;log2m=10;dictThreshold=150000")));
+ assertEquals(function.getThreshold(), 200_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 150_000);
+
+ // Test parameter order independence
+ DistinctCountSmartHLLAggregationFunction function1 = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "dictThreshold=50000;threshold=100000;log2m=8")));
+ DistinctCountSmartHLLAggregationFunction function2 = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "log2m=8;dictThreshold=50000;threshold=100000")));
+ assertEquals(function1.getThreshold(), function2.getThreshold());
+ assertEquals(function1.getDictIdCardinalityThreshold(), function2.getDictIdCardinalityThreshold());
+
+ // Test legacy parameter names
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "hllConversionThreshold=50000;hllLog2m=10")));
+ assertEquals(function.getThreshold(), 50_000);
+
+ // Test case-insensitive parameters
+ function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col"),
+ ExpressionContext.forLiteral(FieldSpec.DataType.STRING, "THRESHOLD=50000;LOG2M=8;DICTTHRESHOLD=100000")));
+ assertEquals(function.getThreshold(), 50_000);
+ assertEquals(function.getDictIdCardinalityThreshold(), 100_000);
+ }
+
+ @Test
+ public void testFunctionMetadata() {
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+
+ // Test function type
+ assertEquals(function.getType().getName(), "distinctCountSmartHLL");
+
+ // Test result types
+ assertEquals(function.getIntermediateResultColumnType(), DataSchema.ColumnDataType.OBJECT);
+ assertEquals(function.getFinalResultColumnType(), DataSchema.ColumnDataType.INT);
+
+ // Test result holder creation
+ assertNotNull(function.createAggregationResultHolder());
+ assertNotNull(function.createGroupByResultHolder(10, 100));
+ }
+
+ @Test
+ public void testHLLOperations() {
+ DistinctCountSmartHLLAggregationFunction function = new DistinctCountSmartHLLAggregationFunction(
+ List.of(ExpressionContext.forIdentifier("col")));
+
+ // Test merge final results (should sum)
+ Integer finalResult = function.mergeFinalResult(100, 200);
+ assertEquals(finalResult.intValue(), 300);
+
+ // Test extract final result (HLL cardinality)
+ HyperLogLog hll = new HyperLogLog(12);
+ for (int i = 0; i < 1000; i++) {
+ hll.offer(i);
+ }
+ Long cardinality = Long.valueOf(function.extractFinalResult(hll));
Review Comment:
function.extractFinalResult(hll) returns Int actually hence Long.valueOf() is used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3717540619
Docs PR : https://github.com/pinot-contrib/pinot-docs/pull/459
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3684347704
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17411?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `19.59799%` with `320 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.49%. Comparing base ([`72be4e6`](https://app.codecov.io/gh/apache/pinot/commit/72be4e685d302199d006f105d3176bc52c9e8367?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`547f4b8`](https://app.codecov.io/gh/apache/pinot/commit/547f4b8a58d78397d55a2921ff5ee4a6ae5a0197?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17411?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tion/DistinctCountSmartHLLAggregationFunction.java](https://app.codecov.io/gh/apache/pinot/pull/17411?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Ffunction%2FDistinctCountSmartHLLAggregationFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50U21hcnRITExBZ2dyZWdhdGlvbkZ1bmN0aW9uLmphdmE=) | 19.59% | [305 Missing and 15 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (72be4e6) and HEAD (547f4b8). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (72be4e6) | HEAD (547f4b8) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17411 +/- ##
============================================
- Coverage 63.26% 55.49% -7.77%
+ Complexity 1474 702 -772
============================================
Files 3152 2464 -688
Lines 187881 139498 -48383
Branches 28765 22287 -6478
============================================
- Hits 118855 77410 -41445
+ Misses 59810 55544 -4266
+ Partials 9216 6544 -2672
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <19.59%> (-7.75%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <19.59%> (-7.77%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <19.59%> (-7.77%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <19.59%> (-0.18%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17411?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Query Engine] Improve DistinctCountSmartHLL for dictionary-encoded columns [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17411:
URL: https://github.com/apache/pinot/pull/17411#issuecomment-3713650268
Also please fix the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org