Numerical aggregate functions have an option to skip or include nans in calculation, skip by default#8457
Conversation
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress_rd[f64, (100000, 0.0)] |
843.8 µs | 1,021.7 µs | -17.41% |
| ❌ | Simulation | chunked_varbinview_opt_into_canonical[(1000, 10)] |
184.2 µs | 221 µs | -16.63% |
| ❌ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
223.7 µs | 258.3 µs | -13.4% |
| ❌ | Simulation | decompress_rd[f64, (100000, 0.01)] |
887 µs | 1,021 µs | -13.12% |
| ❌ | Simulation | chunked_varbinview_opt_canonical_into[(100, 100)] |
305.3 µs | 339.4 µs | -10.05% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
191.5 µs | 154.8 µs | +23.67% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
206.6 µs | 170.1 µs | +21.43% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.1)] |
979 µs | 842.8 µs | +16.16% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
215.3 ns | 186.1 ns | +15.67% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[1024] |
275.6 ns | 246.4 ns | +11.84% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rk/skipnans (dbd26f8) with develop (de60638)
|
Need to make sure that stats are only ever written with skipnan: true |
joseph-isaacs
left a comment
There was a problem hiding this comment.
Its good to have options, stats should ignore NaN but the default should be to propergate NaN
Can do this in the future
…in calculation, skip by default Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Robert Kruszewski <github@robertk.io>
Mean::finalize (the grouped array path) divided the sum and count arrays directly, so an all-null group produced 0/0 = NaN, while finalize_scalar returned s/c for the same input and also produced NaN. Nulls are skipped during accumulation, so a zero-count group is an empty mean and should be null, as in SQL. Mask out zero-count entries after the division in finalize, and add a zero-count arm to finalize_scalar, so both paths return null. NaN handling is unchanged: NumericalAggregateOpts::skip_nans (the default, from #8457) still controls whether NaN values poison the result. The new tests run the same groups (NaN/null mixtures, all-null, all-valid) through both the scalar-partial path and the grouped array path with identical assertions. Signed-off-by: Dimitar Dimitrov <dimitar@spiraldb.com>
Almost all of the time you want to skip nans but for rare cases when you don't
we need to be able to configure it