[GitHub] [arrow] pitrou commented on a change in pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r467947160



##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -409,6 +409,10 @@ class DictionaryBuilder : public internal::DictionaryBuilderBase<AdaptiveIntBuil
   using BASE = internal::DictionaryBuilderBase<AdaptiveIntBuilder, T>;
   using BASE::BASE;
 
+  Status ExpandIndexByteWidth(uint8_t new_index_byte_width) {
+    return BASE::indices_builder_.ExpandIntSize(new_index_byte_width);

Review comment:
       Wouldn't it be better to add a new constructor signature, such as:
   ```c++
   DictionaryBuilderBase(const std::shared_ptr<DataType>& start_index_type,
                         const std::shared_ptr<DataType>& value_type,
                         MemoryPool* pool = default_memory_pool())
   ```
   ?

##########
File path: cpp/src/arrow/array/builder_adaptive.h
##########
@@ -122,9 +122,10 @@ class ARROW_EXPORT AdaptiveUIntBuilder : public internal::AdaptiveIntBuilderBase
 
   std::shared_ptr<DataType> type() const override;
 
+  Status ExpandIntSize(uint8_t new_int_size);

Review comment:
       Hmm... I don't think it is required to make this public. Is it?

##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -22,6 +22,8 @@
 #include <string>
 #include <vector>
 
+#include <iostream>

Review comment:
       Is this necessary? If so, please put this include with the standard library includes above.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org