search

facebookincubator / velox

A C++ vectorized database acceleration library aimed to optimizing query engines and data processing systems.
https://velox-lib.io/
Apache License 2.0
3.42k stars 1.12k forks source link

Aggregate::accumulatorAlignmentSize() is not working as expected #4214

Open Yuhta opened 1 year ago

Yuhta commented 1 year ago

Bug description

As @majetideepak found out in #4129, Aggregate::accumulatorAlignmentSize() only aligns the offset and fails to align the actual address of the accumulator. This might not be easy to do. We should fix it or remove this method.

System information

Not relavant

Relevant logs

No response

mbasmanova commented 1 year ago

@Yuhta @majetideepak It is not very clear what the issue is. Would you provide some more details?

majetideepak commented 1 year ago

@mbasmanova We added the Aggregate::accumulatorAlignmentSize() API to align the offset_ to 16 bytes for int128_t pointers. However, we noticed in #4129 that this is insufficient. We have other allocations like groups below whose addresses cannot easily be aligned to 16 bytes and can result in unaligned int128_t pointer values causing a segfault. https://github.com/facebookincubator/velox/blob/71037877729aa300207e94d402f436213b232ced/velox/functions/prestosql/aggregates/DecimalAggregate.h#L83-L90