Conversation
Signed-off-by: blaginin <dima@spiraldb.com> Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: blaginin <dima@spiraldb.com>
| /// Return the count of non-null elements in an array. | ||
| /// | ||
| /// See [`Count`] for details. | ||
| pub fn count(array: &ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult<u64> { |
There was a problem hiding this comment.
If this is counting things in an array, should it return a usize?
| false | ||
| } | ||
|
|
||
| fn accumulate( |
There was a problem hiding this comment.
We should be able to register a generic aggregate kernel to reduce count-non-null to be Array.validity().sum(), then we avoid decompressing all the data.
There was a problem hiding this comment.
added generic kernels first, but then it's hard to express "I never want Count::accumulate to be called" - so switched to try_accumulate instead
| } | ||
|
|
||
| fn partial_struct_dtype() -> DType { | ||
| DType::Struct( |
There was a problem hiding this comment.
Can you static LazyLock this entire dtype inside this function?
| pub struct MeanPartial { | ||
| sum: f64, | ||
| count: u64, | ||
| } |
There was a problem hiding this comment.
This will compute with a pretty large error?
There was a problem hiding this comment.
There was a problem hiding this comment.
you need only hold the partial mean and count
There was a problem hiding this comment.
You can incrementally push the next value into this partial, but you cannot combine two partials afaik?
There was a problem hiding this comment.
For combining you do.
There was a problem hiding this comment.
DF sum is T and ours is f64
There was a problem hiding this comment.
Error comes from numbers of different scales being combined, not from number of OPS.
Since we are storing these on disk we cannot "just change it later".
There was a problem hiding this comment.
DF only supports AVG for floats, decimals, and durations. And coerces all floats to f64.
There was a problem hiding this comment.
| Input Type | DuckDB | DataFusion | Velox |
|--------------|--------|------------|------------|
| Integer types| DOUBLE | Float64 | DOUBLE |
| FLOAT/REAL | DOUBLE | Float64 | REAL |
| DOUBLE | DOUBLE | Float64 | DOUBLE |
| DECIMAL(p,s) | DOUBLE | Float64 | DECIMAL(p,s)|
There was a problem hiding this comment.
thought about this more - i think it should decimal for decimals? - happy to change
Signed-off-by: blaginin <github@blaginin.me>
Signed-off-by: blaginin <github@blaginin.me>
Merging this PR will improve performance by 27.66%
Performance Changes
Comparing 
Footnotes
|
Signed-off-by: blaginin <github@blaginin.me>
Signed-off-by: blaginin <github@blaginin.me>
# Conflicts: # vortex-array/public-api.lock # vortex-array/src/aggregate_fn/fns/mod.rs
There was a problem hiding this comment.
we can factor this out of this PR, we could at a later date add the back to on disk version will be the same?
There was a problem hiding this comment.
disk version will be the same
i blocked serialization for now?
we can factor this out of this PR
why?
| /// - Decimals are kept as decimals with widened precision and scale | ||
| /// (`+4` each, capped at [`MAX_PRECISION`] / [`MAX_SCALE`]), matching | ||
| /// DataFusion's `coerce_avg_type`. |
There was a problem hiding this comment.
I think you definitely want higher for the sum, that is only 10k max sized values for an overflow
There was a problem hiding this comment.
yeah, but Sum.partial_dtype() also adds +10
3 participants
No description provided.