New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices #7898
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. Here's a small number of comments and questions.
64f06da
to
bb91c38
Compare
@mrkn I don't think it is, feel free to add it :-) |
@pitrou OK, I'll do it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. Here are a couple more comments.
cd94c43
to
b12606c
Compare
@pitrou I've finished. Could you please have a look at this again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments and we'll be good.
cpp/src/arrow/array/builder_dict.h
Outdated
@@ -93,6 +93,10 @@ class ARROW_EXPORT DictionaryMemoTable { | |||
std::unique_ptr<DictionaryMemoTableImpl> impl_; | |||
}; | |||
|
|||
/// \brief Check array's value type by DCHECK | |||
ARROW_EXPORT void CheckArrayType(const std::shared_ptr<DataType>& expected_type, | |||
const Array& array, const std::string& message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this a protected method of DictionaryBuilderBase
or something? We don't want to expose it as a public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, take const char*
for the error message, to avoid constructing a std::string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make this a protected method of ArrayBuilder
.
@pitrou I've finished. Could you please have a look at this again? |
This check is available only on debug build.
Avoid crashing since we can return an error
…or deciding the starting bit width of the indices As [the discussion in arrow-dev ML](https://lists.apache.org/thread.html/r69b17b5043bc629507440e962fd50d087c8833cd70682651c6ebda3d%40%3Cdev.arrow.apache.org%3E), it may be better that when creating a dictionary builder by MakeBuilder function, we can specify the starting bit width of the builder's index by the dictionary index type specified in the argument of MakeBuilder. Closes apache#7898 from mrkn/ARROW-9642 Lead-authored-by: Kenta Murata <mrkn@mrkn.jp> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…or deciding the starting bit width of the indices As [the discussion in arrow-dev ML](https://lists.apache.org/thread.html/r69b17b5043bc629507440e962fd50d087c8833cd70682651c6ebda3d%40%3Cdev.arrow.apache.org%3E), it may be better that when creating a dictionary builder by MakeBuilder function, we can specify the starting bit width of the builder's index by the dictionary index type specified in the argument of MakeBuilder. Closes apache#7898 from mrkn/ARROW-9642 Lead-authored-by: Kenta Murata <mrkn@mrkn.jp> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
As the discussion in arrow-dev ML, it may be better that when creating a dictionary builder by MakeBuilder function, we can specify the starting bit width of the builder's index by the dictionary index type specified in the argument of MakeBuilder.