Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
gatesn
force-pushed
the
ngates/aggregate-constant
branch
from
7d210b8 to
68eabe7
Compare
gatesn
changed the base branch from
ngates/aggregate-pushdown
to
graphite-base/6839
Signed-off-by: Nicholas Gates <nick@nickgates.com>
gatesn
force-pushed
the
graphite-base/6839
branch
from
6f1e7a3 to
0e50f08
Compare
gatesn
force-pushed
the
ngates/aggregate-constant
branch
from
331c227 to
f8f016d
Compare
gatesn
force-pushed
the
ngates/aggregate-constant
branch
from
f8f016d to
13d889e
Compare
|
The reason we have columnar for scalar fns is for constant folding/reduction, do we have a similar thing here? |
|
It just forces the aggregate function to also define its operation over scalars. Instead of requiring everyone to register an additional kernel for ConstantArray. Same as for ScalarFn, it just makes it harder to do the wrong thing. |
|
My preference is for another kernel |
| /// Accumulate a constant array into the sum state. | ||
| /// Computes `scalar * len` and adds to the accumulator. | ||
| /// Returns Ok(true) if saturated (overflow), Ok(false) if not. | ||
| fn accumulate_constant(inner: &mut SumState, c: &ConstantArray) -> VortexResult<bool> { |
There was a problem hiding this comment.
This really can just be a kernel
joseph-isaacs
left a comment
joseph-isaacs
left a comment
There was a problem hiding this comment.
I would do this as a kernel to reduce the complexity of implementing a new aggregation!
2 participants
Changes the AggregateFnVTable::accumulate to take columnar instead of canonical. Might as well make the function define the optimized path over constants the same way we do for scalar functions.