bkietz commented on a change in pull request #11910:
URL: https://github.com/apache/arrow/pull/11910#discussion_r765845229
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool)
{}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
BaseBinaryBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
- : BaseBinaryBuilder(pool) {}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(type) {}
Review comment:
Please make this constructor `protected`; this will make it clearer that
BaseBinaryBuilder is intended for use by subclasses only
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -396,52 +401,62 @@ class BaseBinaryBuilder : public ArrayBuilder {
/// \brief Builder class for variable-length binary data
class ARROW_EXPORT BinaryBuilder : public BaseBinaryBuilder<BinaryType> {
public:
- using BaseBinaryBuilder::BaseBinaryBuilder;
+ explicit BinaryBuilder(MemoryPool* pool = default_memory_pool())
+ : BaseBinaryBuilder(binary(), pool) {}
+
+ BinaryBuilder(const std::shared_ptr<DataType> &type, MemoryPool *pool)
+ : BaseBinaryBuilder(type, pool) {}
Review comment:
For this constructor and the other constructors below which accept an
explicit type, please do one of:
- delete them
- make them protected as well
- rewrite them to `BinaryBuilder(const std::shared_ptr<ExtensionType> &type,
MemoryPool *pool = default_memory_pool())`, since the only valid case I can see
for allowing public construction of a `BinaryBuilder` with an output type other
than `binary()` is building an extension array (which AFAICT is not used
anywhere nor is it tested so my preference would be deletion for now)
##########
File path: cpp/src/arrow/array/builder_binary.h
##########
@@ -51,10 +51,12 @@ class BaseBinaryBuilder : public ArrayBuilder {
using offset_type = typename TypeClass::offset_type;
explicit BaseBinaryBuilder(MemoryPool* pool = default_memory_pool())
- : ArrayBuilder(pool), offsets_builder_(pool), value_data_builder_(pool)
{}
+ : ArrayBuilder(pool), offsets_builder_(pool),
+ value_data_builder_(pool), type_(binary()) {}
Review comment:
Please delete this constructor; I think it's more understandable if all
subclasses of BaseBinaryBuilder are required to pass the resulting type
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_string.cc
##########
@@ -58,7 +58,7 @@ struct NumericToStringCastFunctor {
static Status Convert(KernelContext* ctx, const ArrayData& input, ArrayData*
output) {
FormatterType formatter(input.type);
- BuilderType builder(input.type, ctx->memory_pool());
+ BuilderType builder(utf8(), ctx->memory_pool());
Review comment:
IIUC, this will cause the output to be typed `utf8` even if we're
casting to `large_utf8`. I think it's better to rely on the default constructor
(which sets the correct output type without the need to be explicit):
```suggestion
BuilderType builder(ctx->memory_pool());
```
--
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]
