pitrou commented on a change in pull request #11694:
URL: https://github.com/apache/arrow/pull/11694#discussion_r756837865
##########
File path: cpp/src/arrow/array/array_primitive.h
##########
@@ -141,6 +148,14 @@ class ARROW_EXPORT DayTimeIntervalArray : public
PrimitiveArray {
// For compatibility with Take kernel.
TypeClass::DayMilliseconds GetView(int64_t i) const { return GetValue(i); }
+ IteratorType begin() const { return IteratorType(*this); }
+
+ IteratorType end() const { return IteratorType(*this, length()); }
Review comment:
Thanks for adding this. Is it being exercised in the tests?
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -3348,4 +3358,38 @@ TEST(TestSwapEndianArrayData, InvalidLength) {
}
}
+template <typename T>
+class TestPrimitiveArray : public ::testing::Test {
+ public:
+ void SetUp() { pool_ = default_memory_pool(); }
+
+ protected:
+ MemoryPool* pool_;
+};
+
+TYPED_TEST_SUITE(TestPrimitiveArray, Primitives);
+
+TYPED_TEST(TestPrimitiveArray, IndexOperator) {
+ random::RandomArrayGenerator rng(0x76878);
+
+ auto input_array = rng.ArrayOf(TypeParam::type(), 6, 0.2);
+ auto con_array = checked_pointer_cast<typename
TypeParam::ArrayType>(input_array);
+ const auto& carr = *con_array;
Review comment:
Simpler:
```c++
const auto& carr = checked_cast<const typename
TypeParam::ArrayType&>(*input_array);
```
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -3348,4 +3358,38 @@ TEST(TestSwapEndianArrayData, InvalidLength) {
}
}
+template <typename T>
+class TestPrimitiveArray : public ::testing::Test {
+ public:
+ void SetUp() { pool_ = default_memory_pool(); }
+
+ protected:
+ MemoryPool* pool_;
+};
+
+TYPED_TEST_SUITE(TestPrimitiveArray, Primitives);
+
+TYPED_TEST(TestPrimitiveArray, IndexOperator) {
+ random::RandomArrayGenerator rng(0x76878);
+
+ auto input_array = rng.ArrayOf(TypeParam::type(), 6, 0.2);
+ auto con_array = checked_pointer_cast<typename
TypeParam::ArrayType>(input_array);
+ const auto& carr = *con_array;
+
+ std::vector<util::optional<typename TypeParam::T>> values;
+ for (auto v : carr) {
+ values.push_back(v);
+ }
+
+ for (int64_t i = 0; i < carr.length(); ++i) {
+ auto res = carr[i];
+ if (res) {
+ ASSERT_EQ(values[i].value(), res.value());
Review comment:
Ok, but this is just testing `operator[]` against the iterator, which
are both using exactly the same implementation.
Can you check against the actual array values instead?
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -2162,6 +2162,16 @@ TEST_F(TestFWBinaryArray, ArrayDataVisitorSliced) {
ARROW_UNUSED(visitor); // Workaround weird MSVC warning
}
+TEST_F(TestFWBinaryArray, ArrayIndexOperator) {
+ auto type = fixed_size_binary(3);
+ auto arr = ArrayFromJSON(type, R"(["abc", null, "def"])");
+ auto fsba = std::static_pointer_cast<FixedSizeBinaryArray>(arr);
Review comment:
Did you forget to do 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
