zanmato1984 commented on code in PR #43292:
URL: https://github.com/apache/arrow/pull/43292#discussion_r1689062596
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1678,16 +1952,28 @@ TEST_F(TestTakeKernelWithStruct, TakeStruct) {
{"a": 2, "b": "hello"}
])");
- this->TestNoValidityBitmapButUnknownNullCount(
- struct_type, R"([{"a": 1}, {"a": 2, "b": "hello"}])", "[0, 1, 0]");
+ this->TestNoValidityBitmapButUnknownNullCount(R"([{"a": 1}, {"a": 2, "b":
"hello"}])",
+ "[0, 1, 0]");
}
-class TestTakeKernelWithUnion : public TestTakeKernelTyped<UnionType> {};
-
-TEST_F(TestTakeKernelWithUnion, TakeUnion) {
- for (const auto& union_type :
- {dense_union({field("a", int32()), field("b", utf8())}, {2, 5}),
- sparse_union({field("a", int32()), field("b", utf8())}, {2, 5})}) {
+template <typename ArrowUnionType>
+class TestTakeKernelWithUnion : public TestTakeKernelTyped<ArrowUnionType> {
+ protected:
+ std::shared_ptr<DataType> value_type() const override {
+ return std::make_shared<ArrowUnionType>(
+ FieldVector{
+ field("a", int32()),
+ field("b", utf8()),
+ },
+ std::vector<int8_t>{
+ 2,
+ 5,
+ });
+ }
+};
+TYPED_TEST_SUITE(TestTakeKernelWithUnion, UnionArrowTypes);
+TYPED_TEST(TestTakeKernelWithUnion, TakeUnion) {
Review Comment:
```suggestion
TYPED_TEST(TestTakeKernelWithUnion, TakeUnion) {
```
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1363,177 +1626,184 @@ TEST_F(TestTakeKernel, Interval) {
this->TestNumericBasics(month_interval());
auto type = day_time_interval();
- CheckTake(type, "[[1, -600], [2, 3000], null]", "[0, null, 2, 1]",
- "[[1, -600], null, null, [2, 3000]]");
+ CheckTakeXA(type, "[[1, -600], [2, 3000], null]", "[0, null, 2, 1]",
+ "[[1, -600], null, null, [2, 3000]]");
type = month_day_nano_interval();
- CheckTake(type, "[[1, -2, 34567890123456789], [2, 3, -34567890123456789],
null]",
- "[0, null, 2, 1]",
- "[[1, -2, 34567890123456789], null, null, [2, 3,
-34567890123456789]]");
+ CheckTakeXA(type, "[[1, -2, 34567890123456789], [2, 3, -34567890123456789],
null]",
+ "[0, null, 2, 1]",
+ "[[1, -2, 34567890123456789], null, null, [2, 3,
-34567890123456789]]");
}
template <typename ArrowType>
-class TestTakeKernelWithNumeric : public TestTakeKernelTyped<ArrowType> {
- protected:
- void AssertTake(const std::string& values, const std::string& indices,
- const std::string& expected) {
- CheckTake(type_singleton(), values, indices, expected);
- }
-
- std::shared_ptr<DataType> type_singleton() {
- return TypeTraits<ArrowType>::type_singleton();
- }
-};
+class TestTakeKernelWithNumeric : public TestTakeKernelTyped<ArrowType> {};
TYPED_TEST_SUITE(TestTakeKernelWithNumeric, NumericArrowTypes);
TYPED_TEST(TestTakeKernelWithNumeric, TakeNumeric) {
- this->TestNumericBasics(this->type_singleton());
+ this->TestNumericBasics(this->value_type());
}
template <typename TypeClass>
class TestTakeKernelWithString : public TestTakeKernelTyped<TypeClass> {
public:
- std::shared_ptr<DataType> value_type() {
- return TypeTraits<TypeClass>::type_singleton();
- }
-
- void AssertTake(const std::string& values, const std::string& indices,
- const std::string& expected) {
- CheckTake(value_type(), values, indices, expected);
- }
-
- void AssertTakeDictionary(const std::string& dictionary_values,
- const std::string& dictionary_indices,
- const std::string& indices,
- const std::string& expected_indices) {
- auto dict = ArrayFromJSON(value_type(), dictionary_values);
- auto type = dictionary(int8(), value_type());
- ASSERT_OK_AND_ASSIGN(auto values,
- DictionaryArray::FromArrays(
- type, ArrayFromJSON(int8(), dictionary_indices),
dict));
- ASSERT_OK_AND_ASSIGN(
- auto expected,
- DictionaryArray::FromArrays(type, ArrayFromJSON(int8(),
expected_indices), dict));
- auto take_indices = ArrayFromJSON(int8(), indices);
- AssertTakeArrays(values, take_indices, expected);
+ void AssertTakeXADictionary(const std::string& dictionary_values,
+ const std::string& dictionary_indices,
+ const std::string& indices,
+ const std::string& expected_indices) {
+ return CheckTakeXADictionary(this->value_type(), dictionary_values,
+ dictionary_indices, indices,
expected_indices);
}
};
TYPED_TEST_SUITE(TestTakeKernelWithString, BaseBinaryArrowTypes);
TYPED_TEST(TestTakeKernelWithString, TakeString) {
- this->AssertTake(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["a", "b", "a"])");
- this->AssertTake(R"([null, "b", "c"])", "[0, 1, 0]", "[null, \"b\", null]");
- this->AssertTake(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b",
"a"])");
+ this->CheckTakeXA(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["a", "b", "a"])");
+ this->CheckTakeXA(R"([null, "b", "c"])", "[0, 1, 0]", "[null, \"b\", null]");
+ this->CheckTakeXA(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b",
"a"])");
- this->TestNoValidityBitmapButUnknownNullCount(this->value_type(), R"(["a",
"b", "c"])",
- "[0, 1, 0]");
+ this->TestNoValidityBitmapButUnknownNullCount(R"(["a", "b", "c"])", "[0, 1,
0]");
std::shared_ptr<DataType> type = this->value_type();
+ const std::string kABC = R"(["a", "b", "c"])";
Review Comment:
Maybe lift it to the top of the function so line 1660 and line 1662 can use
it?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1678,16 +1952,28 @@ TEST_F(TestTakeKernelWithStruct, TakeStruct) {
{"a": 2, "b": "hello"}
])");
- this->TestNoValidityBitmapButUnknownNullCount(
- struct_type, R"([{"a": 1}, {"a": 2, "b": "hello"}])", "[0, 1, 0]");
+ this->TestNoValidityBitmapButUnknownNullCount(R"([{"a": 1}, {"a": 2, "b":
"hello"}])",
+ "[0, 1, 0]");
}
-class TestTakeKernelWithUnion : public TestTakeKernelTyped<UnionType> {};
-
-TEST_F(TestTakeKernelWithUnion, TakeUnion) {
- for (const auto& union_type :
- {dense_union({field("a", int32()), field("b", utf8())}, {2, 5}),
- sparse_union({field("a", int32()), field("b", utf8())}, {2, 5})}) {
+template <typename ArrowUnionType>
+class TestTakeKernelWithUnion : public TestTakeKernelTyped<ArrowUnionType> {
+ protected:
+ std::shared_ptr<DataType> value_type() const override {
+ return std::make_shared<ArrowUnionType>(
+ FieldVector{
+ field("a", int32()),
+ field("b", utf8()),
+ },
+ std::vector<int8_t>{
+ 2,
+ 5,
+ });
+ }
+};
+TYPED_TEST_SUITE(TestTakeKernelWithUnion, UnionArrowTypes);
Review Comment:
```suggestion
TYPED_TEST_SUITE(TestTakeKernelWithUnion, UnionArrowTypes);
```
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1363,177 +1626,184 @@ TEST_F(TestTakeKernel, Interval) {
this->TestNumericBasics(month_interval());
auto type = day_time_interval();
- CheckTake(type, "[[1, -600], [2, 3000], null]", "[0, null, 2, 1]",
- "[[1, -600], null, null, [2, 3000]]");
+ CheckTakeXA(type, "[[1, -600], [2, 3000], null]", "[0, null, 2, 1]",
+ "[[1, -600], null, null, [2, 3000]]");
type = month_day_nano_interval();
- CheckTake(type, "[[1, -2, 34567890123456789], [2, 3, -34567890123456789],
null]",
- "[0, null, 2, 1]",
- "[[1, -2, 34567890123456789], null, null, [2, 3,
-34567890123456789]]");
+ CheckTakeXA(type, "[[1, -2, 34567890123456789], [2, 3, -34567890123456789],
null]",
+ "[0, null, 2, 1]",
+ "[[1, -2, 34567890123456789], null, null, [2, 3,
-34567890123456789]]");
}
template <typename ArrowType>
-class TestTakeKernelWithNumeric : public TestTakeKernelTyped<ArrowType> {
- protected:
- void AssertTake(const std::string& values, const std::string& indices,
- const std::string& expected) {
- CheckTake(type_singleton(), values, indices, expected);
- }
-
- std::shared_ptr<DataType> type_singleton() {
- return TypeTraits<ArrowType>::type_singleton();
- }
-};
+class TestTakeKernelWithNumeric : public TestTakeKernelTyped<ArrowType> {};
TYPED_TEST_SUITE(TestTakeKernelWithNumeric, NumericArrowTypes);
TYPED_TEST(TestTakeKernelWithNumeric, TakeNumeric) {
- this->TestNumericBasics(this->type_singleton());
+ this->TestNumericBasics(this->value_type());
}
template <typename TypeClass>
class TestTakeKernelWithString : public TestTakeKernelTyped<TypeClass> {
public:
- std::shared_ptr<DataType> value_type() {
- return TypeTraits<TypeClass>::type_singleton();
- }
-
- void AssertTake(const std::string& values, const std::string& indices,
- const std::string& expected) {
- CheckTake(value_type(), values, indices, expected);
- }
-
- void AssertTakeDictionary(const std::string& dictionary_values,
- const std::string& dictionary_indices,
- const std::string& indices,
- const std::string& expected_indices) {
- auto dict = ArrayFromJSON(value_type(), dictionary_values);
- auto type = dictionary(int8(), value_type());
- ASSERT_OK_AND_ASSIGN(auto values,
- DictionaryArray::FromArrays(
- type, ArrayFromJSON(int8(), dictionary_indices),
dict));
- ASSERT_OK_AND_ASSIGN(
- auto expected,
- DictionaryArray::FromArrays(type, ArrayFromJSON(int8(),
expected_indices), dict));
- auto take_indices = ArrayFromJSON(int8(), indices);
- AssertTakeArrays(values, take_indices, expected);
+ void AssertTakeXADictionary(const std::string& dictionary_values,
+ const std::string& dictionary_indices,
+ const std::string& indices,
+ const std::string& expected_indices) {
+ return CheckTakeXADictionary(this->value_type(), dictionary_values,
+ dictionary_indices, indices,
expected_indices);
}
};
TYPED_TEST_SUITE(TestTakeKernelWithString, BaseBinaryArrowTypes);
TYPED_TEST(TestTakeKernelWithString, TakeString) {
- this->AssertTake(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["a", "b", "a"])");
- this->AssertTake(R"([null, "b", "c"])", "[0, 1, 0]", "[null, \"b\", null]");
- this->AssertTake(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b",
"a"])");
+ this->CheckTakeXA(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["a", "b", "a"])");
+ this->CheckTakeXA(R"([null, "b", "c"])", "[0, 1, 0]", "[null, \"b\", null]");
+ this->CheckTakeXA(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b",
"a"])");
- this->TestNoValidityBitmapButUnknownNullCount(this->value_type(), R"(["a",
"b", "c"])",
- "[0, 1, 0]");
+ this->TestNoValidityBitmapButUnknownNullCount(R"(["a", "b", "c"])", "[0, 1,
0]");
std::shared_ptr<DataType> type = this->value_type();
+ const std::string kABC = R"(["a", "b", "c"])";
std::shared_ptr<Array> arr;
- ASSERT_RAISES(IndexError,
- TakeJSON(type, R"(["a", "b", "c"])", int8(), "[0, 9, 0]",
&arr));
- ASSERT_RAISES(IndexError, TakeJSON(type, R"(["a", "b", null, "ddd", "ee"])",
int64(),
- "[2, 5]", &arr));
+ ASSERT_RAISES(IndexError, TakeAAA(type, kABC, "[0, 9, 0]").Value(&arr));
+ ASSERT_RAISES(IndexError, TakeAAA(type, kABC, "[2, 5]").Value(&arr));
+ Datum chunked_arr;
+ ASSERT_RAISES(IndexError, TakeCAC(type, {kABC, kABC}, "[0, 9,
0]").Value(&chunked_arr));
+ ASSERT_RAISES(IndexError, TakeCAC(type, {kABC, kABC}, "[4,
10]").Value(&chunked_arr));
}
TYPED_TEST(TestTakeKernelWithString, TakeDictionary) {
auto dict = R"(["a", "b", "c", "d", "e"])";
- this->AssertTakeDictionary(dict, "[3, 4, 2]", "[0, 1, 0]", "[3, 4, 3]");
- this->AssertTakeDictionary(dict, "[null, 4, 2]", "[0, 1, 0]", "[null, 4,
null]");
- this->AssertTakeDictionary(dict, "[3, 4, 2]", "[null, 1, 0]", "[null, 4,
3]");
+ this->AssertTakeXADictionary(dict, "[3, 4, 2]", "[0, 1, 0]", "[3, 4, 3]");
+ this->AssertTakeXADictionary(dict, "[null, 4, 2]", "[0, 1, 0]", "[null, 4,
null]");
+ this->AssertTakeXADictionary(dict, "[3, 4, 2]", "[null, 1, 0]", "[null, 4,
3]");
}
class TestTakeKernelFSB : public TestTakeKernelTyped<FixedSizeBinaryType> {
public:
- std::shared_ptr<DataType> value_type() { return fixed_size_binary(3); }
-
- void AssertTake(const std::string& values, const std::string& indices,
- const std::string& expected) {
- CheckTake(value_type(), values, indices, expected);
- }
+ std::shared_ptr<DataType> value_type() const override { return
fixed_size_binary(3); }
};
TEST_F(TestTakeKernelFSB, TakeFixedSizeBinary) {
- this->AssertTake(R"(["aaa", "bbb", "ccc"])", "[0, 1, 0]", R"(["aaa", "bbb",
"aaa"])");
- this->AssertTake(R"([null, "bbb", "ccc"])", "[0, 1, 0]", "[null, \"bbb\",
null]");
- this->AssertTake(R"(["aaa", "bbb", "ccc"])", "[null, 1, 0]", R"([null,
"bbb", "aaa"])");
+ const std::string kABC = R"(["aaa", "bbb", "ccc"])";
+ this->CheckTakeXA(kABC, "[0, 1, 0]", R"(["aaa", "bbb", "aaa"])");
+ this->CheckTakeXA(R"([null, "bbb", "ccc"])", "[0, 1, 0]", "[null, \"bbb\",
null]");
+ this->CheckTakeXA(kABC, "[null, 1, 0]", R"([null, "bbb", "aaa"])");
- this->TestNoValidityBitmapButUnknownNullCount(this->value_type(),
- R"(["aaa", "bbb", "ccc"])",
"[0, 1, 0]");
+ this->TestNoValidityBitmapButUnknownNullCount(kABC, "[0, 1, 0]");
std::shared_ptr<DataType> type = this->value_type();
+ const std::string kABNullDE = R"(["aaa", "bbb", "ccc", null, "eee"])";
Review Comment:
```suggestion
const std::string kABNullDE = R"(["aaa", "bbb", null, "ddd", "eee"])";
```
--
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]
