pitrou commented on code in PR #14225:
URL: https://github.com/apache/arrow/pull/14225#discussion_r1019270632
##########
cpp/src/arrow/array/builder_base.h:
##########
@@ -69,7 +69,8 @@ constexpr int64_t kListMaximumElements =
std::numeric_limits<int32_t>::max() - 1
/// For example, ArrayBuilder* pointing to BinaryBuilder should be downcast
before use.
class ARROW_EXPORT ArrayBuilder {
public:
- explicit ArrayBuilder(MemoryPool* pool) : pool_(pool),
null_bitmap_builder_(pool) {}
+ explicit ArrayBuilder(MemoryPool* pool, int64_t alignment =
kDefaultBufferAlignment)
+ : pool_(pool), alignment_(alignment), null_bitmap_builder_(pool) {}
Review Comment:
It seems we don't want to enforce alignment for the null bitmap builder?
##########
cpp/src/arrow/testing/random.h:
##########
@@ -25,6 +25,7 @@
#include <random>
#include <vector>
+#include "arrow/memory_pool.h"
Review Comment:
We shouldn't need this if `arrow/type_fwd.h` is included instead.
##########
cpp/src/arrow/testing/random.h:
##########
@@ -49,7 +50,9 @@ class ARROW_TESTING_EXPORT RandomArrayGenerator {
/// \param[in] null_probability the probability of a bit being zero
///
/// \return a generated Buffer
- std::shared_ptr<Buffer> NullBitmap(int64_t size, double null_probability =
0);
+ std::shared_ptr<Buffer> NullBitmap(int64_t size, double null_probability = 0,
+ int64_t alignment =
MemoryPool::kDefaultAlignment,
+ MemoryPool* memory_pool = NULLPTR);
Review Comment:
I see, thanks.
##########
r/src/memorypool.cpp:
##########
@@ -23,15 +23,23 @@ class GcMemoryPool : public arrow::MemoryPool {
public:
GcMemoryPool() : pool_(arrow::default_memory_pool()) {}
- arrow::Status Allocate(int64_t size, uint8_t** out) override {
+ using MemoryPool::Allocate;
+ using MemoryPool::Free;
+ using MemoryPool::Reallocate;
+
+ arrow::Status Allocate(int64_t size, int64_t alignment, uint8_t** out)
override {
return GcAndTryAgain([&] { return pool_->Allocate(size, out); });
Review Comment:
We should forward alignment here.
##########
cpp/src/arrow/array/builder_dict.h:
##########
@@ -146,8 +146,9 @@ class DictionaryBuilderBase : public ArrayBuilder {
!is_fixed_size_binary_type<T1>::value,
const std::shared_ptr<DataType>&>
value_type,
- MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool),
+ MemoryPool* pool = default_memory_pool(),
+ int64_t alignment = kDefaultBufferAlignment)
+ : ArrayBuilder(pool, alignment),
Review Comment:
Should `indices_builder_` below also pick up the alignment?
##########
cpp/src/arrow/buffer_builder.h:
##########
@@ -198,6 +203,7 @@ class ARROW_EXPORT BufferBuilder {
uint8_t* data_;
int64_t capacity_;
int64_t size_;
+ int64_t alignment_;
Review Comment:
Ah, ok, nevermind :-)
##########
cpp/src/arrow/testing/random.h:
##########
@@ -49,7 +50,9 @@ class ARROW_TESTING_EXPORT RandomArrayGenerator {
/// \param[in] null_probability the probability of a bit being zero
///
/// \return a generated Buffer
- std::shared_ptr<Buffer> NullBitmap(int64_t size, double null_probability =
0);
+ std::shared_ptr<Buffer> NullBitmap(int64_t size, double null_probability = 0,
+ int64_t alignment =
kDefaultBufferAlignment,
+ MemoryPool* memory_pool = NULLPTR);
Review Comment:
The common idiom accross the codebase is `MemoryPool* pool =
default_memory_pool()`, let's stick to this perhaps?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
