You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/04 07:25:13 UTC
[GitHub] [arrow] mrkn opened a new pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
mrkn opened a new pull request #7898:
URL: https://github.com/apache/arrow/pull/7898
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r478484727
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
Yes, this approach is fine, but please always define `CheckArrayType` (so that the ABI isn't different in debug builds).
As for `FixedSizedBinaryType`, let's leave it like that for now.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r468656539
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -22,6 +22,8 @@
#include <string>
#include <vector>
+#include <iostream>
Review comment:
This should be removed before committing but I forgot.
Thanks for catching this!
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r469945583
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -105,18 +105,46 @@ class DictionaryBuilderBase : public ArrayBuilder {
// WARNING: the type given below is the value type, not the DictionaryType.
// The DictionaryType is instantiated on the Finish() call.
- template <typename T1 = T>
- DictionaryBuilderBase(enable_if_t<!std::is_base_of<FixedSizeBinaryType, T1>::value,
+ template <typename B = BuilderType, typename T1 = T>
+ DictionaryBuilderBase(uint8_t start_int_size,
+ enable_if_t<std::is_base_of<AdaptiveIntBuilderBase, B>::value &&
+ !is_fixed_size_binary_type<T1>::value,
const std::shared_ptr<DataType>&>
value_type,
MemoryPool* pool = default_memory_pool())
+ : ArrayBuilder(pool),
+ memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
+ delta_offset_(0),
+ byte_width_(-1),
+ indices_builder_(start_int_size, pool),
+ value_type_(value_type) {}
+
+ template <typename T1 = T>
+ DictionaryBuilderBase(
Review comment:
This constructor is for the cases that BuilderType is not AdaptiveIntBuilder. The constructor for the case of AdaptiveIntBuilder cannot be reused by this constructor because the constructor of a non-adaptive IntBuilder (i.e. `indices_builder_`) doesn't take the `start_int_size` argument.
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-668440282
https://issues.apache.org/jira/browse/ARROW-9642
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-672733878
@pitrou OK, I'll do it.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r469954523
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
I think this is practically meaningless. It is the same as `AppendNulls` as its implementation. But, as `AppendArray` function in `DictionaryBuilder` of `NullType` actually accepts an Array of any type, I believe we should test the behavior.
We probably need to check the value type of the given `Array` in `AppendArray`.
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r471486448
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
Checking the array type in each `AppendArray` call may result in performance degradation.
However, we should certainly _not_ assert that passing the wrong type passes silently.
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r480167400
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -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);
Review comment:
Can you make this a protected method of `DictionaryBuilderBase` or something? We don't want to expose it as a public API.
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -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);
Review comment:
Also, take `const char*` for the error message, to avoid constructing a `std::string`.
##########
File path: cpp/src/arrow/builder.cc
##########
@@ -129,8 +134,10 @@ Status MakeBuilder(MemoryPool* pool, const std::shared_ptr<DataType>& type,
case Type::DICTIONARY: {
const auto& dict_type = static_cast<const DictionaryType&>(*type);
- DictionaryBuilderCase visitor = {pool, dict_type.value_type(), nullptr, out};
- return visitor.Make();
+ DictionaryBuilderCase visitor = {pool, dict_type.index_type(),
+ dict_type.value_type(), nullptr, out};
+ auto status = visitor.Make();
+ return status;
Review comment:
`return visitor.Make()`, simply?
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2306,6 +2319,19 @@ TEST_F(TestAdaptiveUIntBuilder, TestAppendNulls) {
}
}
+TEST(TestAdaptiveUIntBuilderWithStartIntSize, TestReset) {
+ auto builder = std::make_shared<AdaptiveUIntBuilder>(
+ static_cast<uint8_t>(sizeof(uint16_t)), default_memory_pool());
+ ASSERT_TRUE(uint16()->Equals(*builder->type()));
Review comment:
`AssertTypeEqual`
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r469387808
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2029,6 +2029,19 @@ TEST_F(TestAdaptiveIntBuilder, TestAppendNulls) {
}
}
+TEST(TestAdaptiveIntBuilderWithStartIntSize, TestReset) {
+ auto builder = std::make_shared<AdaptiveIntBuilder>(
+ static_cast<uint8_t>(sizeof(int16_t)), default_memory_pool());
+ ASSERT_TRUE(int16()->Equals(*builder->type()));
+
+ ASSERT_OK(
+ builder->Append(static_cast<int64_t>(std::numeric_limits<int16_t>::max()) + 1));
+ ASSERT_TRUE(int32()->Equals(*builder->type()));
Review comment:
Same here.
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2029,6 +2029,19 @@ TEST_F(TestAdaptiveIntBuilder, TestAppendNulls) {
}
}
+TEST(TestAdaptiveIntBuilderWithStartIntSize, TestReset) {
+ auto builder = std::make_shared<AdaptiveIntBuilder>(
+ static_cast<uint8_t>(sizeof(int16_t)), default_memory_pool());
+ ASSERT_TRUE(int16()->Equals(*builder->type()));
Review comment:
`AssertTypeEqual`?
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2029,6 +2029,19 @@ TEST_F(TestAdaptiveIntBuilder, TestAppendNulls) {
}
}
+TEST(TestAdaptiveIntBuilderWithStartIntSize, TestReset) {
+ auto builder = std::make_shared<AdaptiveIntBuilder>(
+ static_cast<uint8_t>(sizeof(int16_t)), default_memory_pool());
+ ASSERT_TRUE(int16()->Equals(*builder->type()));
+
+ ASSERT_OK(
+ builder->Append(static_cast<int64_t>(std::numeric_limits<int16_t>::max()) + 1));
+ ASSERT_TRUE(int32()->Equals(*builder->type()));
+
+ builder->Reset();
+ ASSERT_TRUE(int16()->Equals(*builder->type()));
Review comment:
Same here.
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -105,18 +105,46 @@ class DictionaryBuilderBase : public ArrayBuilder {
// WARNING: the type given below is the value type, not the DictionaryType.
// The DictionaryType is instantiated on the Finish() call.
- template <typename T1 = T>
- DictionaryBuilderBase(enable_if_t<!std::is_base_of<FixedSizeBinaryType, T1>::value,
+ template <typename B = BuilderType, typename T1 = T>
+ DictionaryBuilderBase(uint8_t start_int_size,
+ enable_if_t<std::is_base_of<AdaptiveIntBuilderBase, B>::value &&
+ !is_fixed_size_binary_type<T1>::value,
const std::shared_ptr<DataType>&>
value_type,
MemoryPool* pool = default_memory_pool())
+ : ArrayBuilder(pool),
+ memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
+ delta_offset_(0),
+ byte_width_(-1),
+ indices_builder_(start_int_size, pool),
+ value_type_(value_type) {}
+
+ template <typename T1 = T>
+ DictionaryBuilderBase(
Review comment:
Would the following work instead:
```c++
DictionaryBuilderBase(const std::shared_ptr<DataType>& value_type, MemoryPool* pool = default_memory_pool())
: DictionaryBuilderBase(sizeof(int8_t), value_type, pool) {}
```
?
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
+ ASSERT_EQ(11, builder.length());
+ ASSERT_EQ(11, builder.null_count());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ ASSERT_TRUE(dict_type->Equals(*result->type()));
Review comment:
`AssertTypeEquals`
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
So one can append an array of ints to a null dictionary builder? What does it mean?
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r476407343
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
@mrkn Sorry for the delay. That sounds fine to me.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r469298987
##########
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:
This function needn't be public anymore.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r474160038
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
@pitrou I replaced the `int_array` by `null_array`.
How do you think inserting `DCHECK` in `AppendArray` to check the array type only in the debug build?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r480201923
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -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);
Review comment:
I'll make this a protected method of `ArrayBuilder`.
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r480170645
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +905,79 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto null_array = ArrayFromJSON(null(), "[null, null, null, null]");
+ ASSERT_OK(builder.AppendArray(*null_array));
+ ASSERT_EQ(11, builder.length());
+ ASSERT_EQ(11, builder.null_count());
+
+ std::shared_ptr<Array> result;
+ ASSERT_OK(builder.Finish(&result));
+ AssertTypeEqual(*dict_type, *result->type());
+ ASSERT_EQ(11, result->length());
+ ASSERT_EQ(11, result->null_count());
+}
+
+TEST(TestNullDictionaryBuilder, AppendArrayTypeMismatch) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ auto int8_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_DEBUG_DEATH({ (void)builder.AppendArray(*int8_array); },
+ "Wrong value type of array to be appended");
+}
+
+// ----------------------------------------------------------------------
+// Index byte width tests
+
+template <typename IndexType, typename ValueType>
+void AssertIndexByteWidth(const std::shared_ptr<DataType>& value_type =
+ TypeTraits<ValueType>::type_singleton()) {
+ auto index_type = TypeTraits<IndexType>::type_singleton();
+ auto dict_type =
+ checked_pointer_cast<DictionaryType>(dictionary(index_type, value_type));
+ std::unique_ptr<ArrayBuilder> builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &builder));
+ auto builder_dict_type = checked_pointer_cast<DictionaryType>(builder->type());
+ ASSERT_TRUE(dict_type->index_type()->Equals(builder_dict_type->index_type()))
Review comment:
`AssertTypeEqual`
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r478143508
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
@pitrou I tried to directly insert `DCHECK` in `AppendArray`, but I found it is difficult because `AppendArray` is defined in `builder_dict.h`. That is a public header file` so we cannot use `DCHECK` there.
Instead of directly inserting `DCHECK`, the candidate patch is below.
```diff
diff --git a/cpp/src/arrow/array/builder_dict.cc b/cpp/src/arrow/array/builder_dict.cc
index 54fd94856..dc239f268 100644
--- a/cpp/src/arrow/array/builder_dict.cc
+++ b/cpp/src/arrow/array/builder_dict.cc
@@ -189,5 +189,11 @@ Status DictionaryMemoTable::InsertValues(const Array& array) {
int32_t DictionaryMemoTable::size() const { return impl_->size(); }
+#ifndef NDEBUG
+void CheckArrayType(const Type::type type_id, const Array& array, const std::string& message) {
+ DCHECK_EQ(type_id, array.type_id()) << message;
+}
+#endif
+
} // namespace internal
} // namespace arrow
diff --git a/cpp/src/arrow/array/builder_dict.h b/cpp/src/arrow/array/builder_dict.h
index f8c77a5b0..a0d02fd1b 100644
--- a/cpp/src/arrow/array/builder_dict.h
+++ b/cpp/src/arrow/array/builder_dict.h
@@ -93,6 +93,10 @@ class ARROW_EXPORT DictionaryMemoTable {
std::unique_ptr<DictionaryMemoTableImpl> impl_;
};
+#ifndef NDEBUG
+void CheckArrayType(const Type::type type_id, const Array& array, const std::string& message);
+#endif
+
/// \brief Array builder for created encoded DictionaryArray from
/// dense array
///
@@ -248,6 +252,10 @@ class DictionaryBuilderBase : public ArrayBuilder {
const Array& array) {
using ArrayType = typename TypeTraits<T>::ArrayType;
+#ifndef NDEBUG
+ CheckArrayType(T::type_id, array, "Wrong value type of array to be appended");
+#endif
+
const auto& concrete_array = static_cast<const ArrayType&>(array);
for (int64_t i = 0; i < array.length(); i++) {
if (array.IsNull(i)) {
@@ -406,6 +414,9 @@ class DictionaryBuilderBase<BuilderType, NullType> : public ArrayBuilder {
/// \brief Append a whole dense array to the builder
Status AppendArray(const Array& array) {
+#ifndef NDEBUG
+ CheckArrayType(NullType::type_id, array, "Wrong value type of array to be appended");
+#endif
for (int64_t i = 0; i < array.length(); i++) {
ARROW_RETURN_NOT_OK(AppendNull());
}
};
} // namespace internal
```
I don't think this approach is good. Can I follow this way?
By the way, I found that `AppendArray` for `DictionaryBuilderBase` of `FixedSizedBinaryType` checks the array type at the beginning of the function and return `Status::Invalid` when mismatching type types. I guess if this kind of check is accepted for `FixedSizedBinaryType`, the same kind of check can be put in the other `AppendArray` functions.
----------------------------------------------------------------
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
[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
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r470001711
##########
File path: cpp/src/arrow/array/builder_dict.h
##########
@@ -105,18 +105,46 @@ class DictionaryBuilderBase : public ArrayBuilder {
// WARNING: the type given below is the value type, not the DictionaryType.
// The DictionaryType is instantiated on the Finish() call.
- template <typename T1 = T>
- DictionaryBuilderBase(enable_if_t<!std::is_base_of<FixedSizeBinaryType, T1>::value,
+ template <typename B = BuilderType, typename T1 = T>
+ DictionaryBuilderBase(uint8_t start_int_size,
+ enable_if_t<std::is_base_of<AdaptiveIntBuilderBase, B>::value &&
+ !is_fixed_size_binary_type<T1>::value,
const std::shared_ptr<DataType>&>
value_type,
MemoryPool* pool = default_memory_pool())
+ : ArrayBuilder(pool),
+ memo_table_(new internal::DictionaryMemoTable(pool, value_type)),
+ delta_offset_(0),
+ byte_width_(-1),
+ indices_builder_(start_int_size, pool),
+ value_type_(value_type) {}
+
+ template <typename T1 = T>
+ DictionaryBuilderBase(
Review comment:
Ah, I see, sorry.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r469298490
##########
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:
@pitrou I've done to do this. Could you please review this again?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r468657312
##########
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:
I see. I try to rewrite this way.
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r470633675
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
@pitrou How about this?
----------------------------------------------------------------
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
[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
Posted by 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
[GitHub] [arrow] mrkn commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-684105045
@pitrou I've finished. Could you please have a look at this again?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-683613062
@pitrou I've finished. Could you please have a look at this again?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-672392348
@pitrou @wesm I found that DictionaryBuilderCase rejects that the value type is NullType though there is the specialization of DictionaryBuilderBase with T=NullType. Is this intentional?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn edited a comment on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
mrkn edited a comment on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-672392348
@pitrou @wesm I found that `DictionaryBuilderCase` rejects that the value type is `NullType` though there is the specialization of `DictionaryBuilderBase` with `T=NullType`. Is this intentional?
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r480195899
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2306,6 +2319,19 @@ TEST_F(TestAdaptiveUIntBuilder, TestAppendNulls) {
}
}
+TEST(TestAdaptiveUIntBuilderWithStartIntSize, TestReset) {
+ auto builder = std::make_shared<AdaptiveUIntBuilder>(
+ static_cast<uint8_t>(sizeof(uint16_t)), default_memory_pool());
+ ASSERT_TRUE(uint16()->Equals(*builder->type()));
Review comment:
I'm sorry for missing this frequently.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#issuecomment-672720419
@mrkn I don't think it is, feel free to add it :-)
----------------------------------------------------------------
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
[GitHub] [arrow] mrkn 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
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7898:
URL: https://github.com/apache/arrow/pull/7898#discussion_r478785713
##########
File path: cpp/src/arrow/array/array_dict_test.cc
##########
@@ -904,6 +904,67 @@ TEST(TestDecimalDictionaryBuilder, DoubleTableSize) {
ASSERT_TRUE(expected.Equals(result));
}
+TEST(TestNullDictionaryBuilder, Basic) {
+ // MakeBuilder
+ auto dict_type = dictionary(int8(), null());
+ std::unique_ptr<ArrayBuilder> boxed_builder;
+ ASSERT_OK(MakeBuilder(default_memory_pool(), dict_type, &boxed_builder));
+ auto& builder = checked_cast<DictionaryBuilder<NullType>&>(*boxed_builder);
+
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_EQ(3, builder.length());
+ ASSERT_EQ(3, builder.null_count());
+
+ ASSERT_OK(builder.AppendNulls(4));
+ ASSERT_EQ(7, builder.length());
+ ASSERT_EQ(7, builder.null_count());
+
+ auto int_array = ArrayFromJSON(int8(), "[0, 1, 0, null]");
+ ASSERT_OK(builder.AppendArray(*int_array));
Review comment:
@pitrou I see, will do it.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou closed pull request #7898: ARROW-9642: [C++] Let MakeBuilder refer DictionaryType's index_type for deciding the starting bit width of the indices
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7898:
URL: https://github.com/apache/arrow/pull/7898
----------------------------------------------------------------
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