yashmayya commented on code in PR #13758:
URL: https://github.com/apache/pinot/pull/13758#discussion_r1716280555
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/NullableSingleInputAggregationFunction.java:
##########
@@ -81,7 +81,9 @@ public void forEachNotNull(int length, BlockValSet
blockValSet, BatchConsumer co
return;
}
- forEachNotNull(length, roaringBitmap.getIntIterator(), consumer);
+ if (roaringBitmap.getCardinality() < length) {
Review Comment:
Thanks, that's a good call out, I hadn't looked at the `contains` API for
this use case and I can confirm that it does indeed perform slightly better
than `getCardinality` from this PR in the `_nullPeriod` = 1 case (and with
similar performance in other cases). Results with `contains`:
```
Benchmark (_nullHandlingEnabledPerCent) (_nullPeriod)
Mode Cnt Score Error Units
BenchmarkModeAggregation.test 100 1
thrpt 50 174354.600 ± 1313.033 ops/ms
BenchmarkModeAggregation.test 100 2
thrpt 50 36.165 ± 0.248 ops/ms
BenchmarkModeAggregation.test 100 4
thrpt 50 32.483 ± 0.106 ops/ms
BenchmarkModeAggregation.test 100 8
thrpt 50 31.178 ± 0.124 ops/ms
BenchmarkModeAggregation.test 100 16
thrpt 50 31.567 ± 0.446 ops/ms
BenchmarkModeAggregation.test 100 32
thrpt 50 31.649 ± 0.447 ops/ms
BenchmarkModeAggregation.test 100 64
thrpt 50 32.200 ± 0.134 ops/ms
BenchmarkModeAggregation.test 100 128
thrpt 50 32.311 ± 0.177 ops/ms
BenchmarkModeAggregation.test 50 1
thrpt 50 73644.879 ± 41534.000 ops/ms
BenchmarkModeAggregation.test 50 2
thrpt 50 33.695 ± 1.105 ops/ms
BenchmarkModeAggregation.test 50 4
thrpt 50 32.220 ± 0.207 ops/ms
BenchmarkModeAggregation.test 50 8
thrpt 50 31.126 ± 0.447 ops/ms
BenchmarkModeAggregation.test 50 16
thrpt 50 31.884 ± 0.296 ops/ms
BenchmarkModeAggregation.test 50 32
thrpt 50 31.326 ± 0.591 ops/ms
BenchmarkModeAggregation.test 50 64
thrpt 50 32.018 ± 0.286 ops/ms
BenchmarkModeAggregation.test 50 128
thrpt 50 32.320 ± 0.190 ops/ms
BenchmarkModeAggregation.test 0 1
thrpt 50 27.950 ± 0.199 ops/ms
BenchmarkModeAggregation.test 0 2
thrpt 50 27.595 ± 0.145 ops/ms
BenchmarkModeAggregation.test 0 4
thrpt 50 27.524 ± 0.160 ops/ms
BenchmarkModeAggregation.test 0 8
thrpt 50 28.522 ± 0.266 ops/ms
BenchmarkModeAggregation.test 0 16
thrpt 50 27.813 ± 0.167 ops/ms
BenchmarkModeAggregation.test 0 32
thrpt 50 28.266 ± 0.424 ops/ms
BenchmarkModeAggregation.test 0 64
thrpt 50 27.646 ± 0.295 ops/ms
BenchmarkModeAggregation.test 0 128
thrpt 50 27.856 ± 0.173 ops/ms
```
(results from `getCardinality` are in [this
comment](https://github.com/apache/pinot/pull/13758#issuecomment-2276020558))
> Do you want to merge this one first, then open a separate PR to fix all of
them?
Yes, I'd be happy to file a follow-up PR to update it in all the places 👍
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
