pitrou commented on a change in pull request #11694:
URL: https://github.com/apache/arrow/pull/11694#discussion_r756169072
##########
File path: cpp/src/arrow/array/array_binary_test.cc
##########
@@ -106,6 +106,13 @@ class TestStringArray : public ::testing::Test {
AssertZeroPadded(*strings_);
}
+ void TestArrayIndexOperator() {
+ auto& arr = *strings_;
+ for (int64_t i = 0; i < arr.length(); ++i) {
+ ASSERT_EQ(expected_[i], (valid_bytes_[i] ? arr[i].value() : ""));
Review comment:
You should test nulls as well. For example:
```c++
if (valid_bytes_[i]) {
ASSERT_TRUE(arr[i].has_value());
ASSERT_EQ(arr[i].value(), expected_[i]);
} else {
ASSERT_FALSE(arr[i].has_value());
}
```
##########
File path: cpp/src/arrow/array/array_binary_test.cc
##########
@@ -106,6 +106,13 @@ class TestStringArray : public ::testing::Test {
AssertZeroPadded(*strings_);
}
+ void TestArrayIndexOperator() {
+ auto& arr = *strings_;
Review comment:
Should use `const auto&`, but really there doesn't seem to be a need for
this, is there?
##########
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:
Please use `checked_pointer_cast` from `arrow/util/checked_cast.h`.
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -3348,4 +3358,54 @@ 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) {
+ using ArrayBuilderPointer = std::shared_ptr<arrow::ArrayBuilder>;
+ using CurrentBuilderType = typename TypeParam::BuilderType;
+ using CurrentArrayType = typename TypeParam::ArrayType;
+ using T = typename TypeParam::T;
+
+ using CType = typename std::conditional<std::is_integral<T>::value ||
+ std::is_floating_point<T>::value,
+ T, typename
TypeParam::ConversionType>::type;
+
+ const std::vector<bool> valids = {true, true, true, false, true, true};
+ const std::vector<CType> values(
+ 6); // default/garbage values, not important for this test
Review comment:
Hmm, no, we should never use undefined values in tests. Also, in this
case you would get zeros, which isn't very interesting.
There could be several ways to achieve this, for example you can try using
`RandomArrayGenerator::ArrayOf`.
--
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]
