yashmayya commented on PR #13791: URL: https://github.com/apache/pinot/pull/13791#issuecomment-2314745025
@gortiz thanks for the feedback, I've added a query level benchmark for `SUM` largely borrowed from your unmerged PR - https://github.com/apache/pinot/pull/12354. I was getting much worse results in this new benchmark when run against this new implementation of `SUM` versus the old existing implementation of `SUM` when null handling was disabled. I attached the async profiler to the benchmark and discovered that the reason was not due to the aggregation function itself, but due to this part - https://github.com/apache/pinot/blob/6323ee9683563a17a070d26da37384898feff898/pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/ProjectionBlockValSet.java#L63-L83 Old: <img width="2555" alt="Screenshot 2024-08-28 at 2 15 08 PM" src="https://github.com/user-attachments/assets/a1805e3f-0257-4c60-a90e-047e0afc3e4e";> New: <img width="2560" alt="Screenshot 2024-08-28 at 2 15 21 PM" src="https://github.com/user-attachments/assets/5f374b04-1855-404a-8214-e15bf6e13cbd";> I've made a slight optimization to `NullableSingleInputAggregationFunction::foldNotNull` so that we don't call `BlockValSet::getNullBitmap` if null handling is disabled and now the results are more along the lines of what we were expecting. However, while the inefficiency in various implementations of `BlockValSet::getNullBitmap` is not a blocker for this PR, we'll definitely need to take a closer look before we can enable null handling by default for the multi-stage query engine's leaf stages (https://github.com/apache/pinot/pull/13570). <hr> Results after the optimization: ### Old ``` Benchmark (_nullHandling) (_nullPeriod) Mode Cnt Score Error Units BenchmarkSumQuery.test false 1 avgt 10 224.108 ± 34.354 us/op BenchmarkSumQuery.test false 2 avgt 10 233.009 ± 19.583 us/op BenchmarkSumQuery.test false 4 avgt 10 234.786 ± 24.552 us/op BenchmarkSumQuery.test false 8 avgt 10 237.363 ± 22.383 us/op BenchmarkSumQuery.test false 16 avgt 10 239.232 ± 22.889 us/op BenchmarkSumQuery.test false 32 avgt 10 243.880 ± 22.810 us/op BenchmarkSumQuery.test false 64 avgt 10 247.721 ± 28.414 us/op BenchmarkSumQuery.test false 128 avgt 10 245.275 ± 22.303 us/op BenchmarkSumQuery.test true 1 avgt 10 558.580 ± 26.856 us/op BenchmarkSumQuery.test true 2 avgt 10 504.837 ± 26.026 us/op BenchmarkSumQuery.test true 4 avgt 10 1818.300 ± 20.919 us/op BenchmarkSumQuery.test true 8 avgt 10 1399.164 ± 14.531 us/op BenchmarkSumQuery.test true 16 avgt 10 1194.566 ± 30.630 us/op BenchmarkSumQuery.test true 32 avgt 10 1022.591 ± 31.863 us/op BenchmarkSumQuery.test true 64 avgt 10 1012.052 ± 32.646 us/op BenchmarkSumQuery.test true 128 avgt 10 916.071 ± 11.453 us/op ``` ### New ``` Benchmark (_nullHandling) (_nullPeriod) Mode Cnt Score Error Units BenchmarkSumQuery.test false 1 avgt 10 215.207 ± 19.276 us/op BenchmarkSumQuery.test false 2 avgt 10 231.648 ± 28.188 us/op BenchmarkSumQuery.test false 4 avgt 10 229.831 ± 20.311 us/op BenchmarkSumQuery.test false 8 avgt 10 239.099 ± 22.620 us/op BenchmarkSumQuery.test false 16 avgt 10 237.133 ± 21.507 us/op BenchmarkSumQuery.test false 32 avgt 10 236.974 ± 19.205 us/op BenchmarkSumQuery.test false 64 avgt 10 245.063 ± 21.753 us/op BenchmarkSumQuery.test false 128 avgt 10 246.219 ± 33.566 us/op BenchmarkSumQuery.test true 1 avgt 10 536.200 ± 21.115 us/op BenchmarkSumQuery.test true 2 avgt 10 600.010 ± 44.327 us/op BenchmarkSumQuery.test true 4 avgt 10 1219.326 ± 40.290 us/op BenchmarkSumQuery.test true 8 avgt 10 930.877 ± 26.545 us/op BenchmarkSumQuery.test true 16 avgt 10 766.682 ± 28.898 us/op BenchmarkSumQuery.test true 32 avgt 10 768.964 ± 22.742 us/op BenchmarkSumQuery.test true 64 avgt 10 639.605 ± 26.990 us/op BenchmarkSumQuery.test true 128 avgt 10 594.882 ± 32.955 us/op ``` -- 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
