pitrou commented on code in PR #50164:
URL: https://github.com/apache/arrow/pull/50164#discussion_r4411039302
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -71,6 +78,42 @@ Result<std::shared_ptr<Array>> FilterFromJSON(
}
}
+std::shared_ptr<Array> MakeViewArrayWithMultiplePayloadBuffers(
+ const std::shared_ptr<DataType>& type) {
+ auto payload0 = Buffer::FromString("prefix-first-long-value-suffix");
+ auto payload1 = Buffer::FromString("header-second-long-value-tail");
+ constexpr int32_t first_offset = 7;
+ constexpr int32_t second_offset = 7;
+ constexpr int32_t first_size = sizeof("first-long-value") - 1;
Review Comment:
Why not `sizeof("prefix-")` and `sizeof("header-")` respectively?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -16,15 +16,20 @@
// under the License.
#include <algorithm>
+#include <cstring>
#include <iostream>
#include <limits>
#include <memory>
#include <string>
#include <utility>
#include <vector>
+#include "arrow/array/array_binary.h"
#include "arrow/array/builder_nested.h"
#include "arrow/array/concatenate.h"
+#include "arrow/buffer.h"
+#include "arrow/c/abi.h"
+#include "arrow/c/bridge.h"
Review Comment:
Why these inclusions?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1675,6 +1778,139 @@ TYPED_TEST(TestTakeKernelWithString, TakeString) {
ASSERT_RAISES(IndexError, TakeCAC(type, {kABC, kABC}, "[4,
10]").Value(&chunked_arr));
}
+TEST_F(TestTakeKernel, TakeViewMultiplePayloadBuffers) {
+ for (const auto& value_type : BinaryViewTypes()) {
+ ARROW_SCOPED_TRACE(value_type->ToString());
+ auto values = MakeViewArrayWithMultiplePayloadBuffers(value_type);
+ auto indices = ArrayFromJSON(int32(), "[2, 4, 0, 5]");
+ auto expected = ArrayFromJSON(
+ value_type, R"(["first-long-value", "second-long-value", "", "z"])");
+
+ DoCheckTakeAAA(values, indices, expected);
+
+ ASSERT_OK_AND_ASSIGN(Datum actual_datum,
+ CallFunction("array_take", {values, indices}));
+ auto actual = actual_datum.make_array();
+ ValidateOutput(*actual);
+ AssertArraysEqual(*expected, *actual, /*verbose=*/true);
+ ASSERT_EQ(actual->data()->buffers.size(), values->data()->buffers.size());
+ ASSERT_EQ(values->data()->buffers[2].get(),
actual->data()->buffers[2].get());
+ ASSERT_EQ(values->data()->buffers[3].get(),
actual->data()->buffers[3].get());
+
+ auto sliced_values = values->Slice(1, 4);
+ auto sliced_indices = ArrayFromJSON(int32(), "[1, 3, 0]");
+ auto sliced_expected =
+ ArrayFromJSON(value_type, R"(["first-long-value", "second-long-value",
"tiny"])");
+ DoAssertTakeAAA(sliced_values, sliced_indices, sliced_expected);
+
+ auto indices_with_offset = ArrayFromJSON(int32(), "[0, 4, null, 2,
5]")->Slice(1, 3);
+ auto expected_with_offset =
+ ArrayFromJSON(value_type, R"(["second-long-value", null,
"first-long-value"])");
+ DoAssertTakeAAA(values, indices_with_offset, expected_with_offset);
+ }
+}
+
+TEST_F(TestTakeKernel, TakeViewDropsUnusedPayloadBuffers) {
Review Comment:
It looks like this test is more or less a superset of the previous one, can
we merge them?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -665,12 +708,65 @@ class TestFilterKernelWithString : public
TestFilterKernel {
}
};
-TYPED_TEST_SUITE(TestFilterKernelWithString, BaseBinaryArrowTypes);
+TYPED_TEST_SUITE(TestFilterKernelWithString,
BaseBinaryOrBinaryViewLikeArrowTypes);
TYPED_TEST(TestFilterKernelWithString, FilterString) {
this->AssertFilter(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["b"])");
this->AssertFilter(R"([null, "b", "c"])", "[0, 1, 0]", R"(["b"])");
this->AssertFilter(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b"])");
+ this->AssertFilter(R"(["a", "long-value-over-inline-limit", null, "z"])",
+ "[1, 0, null, 1]", R"(["a", null, "z"])");
Review Comment:
Also test a case where the long-value is selected?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -665,12 +708,65 @@ class TestFilterKernelWithString : public
TestFilterKernel {
}
};
-TYPED_TEST_SUITE(TestFilterKernelWithString, BaseBinaryArrowTypes);
+TYPED_TEST_SUITE(TestFilterKernelWithString,
BaseBinaryOrBinaryViewLikeArrowTypes);
TYPED_TEST(TestFilterKernelWithString, FilterString) {
this->AssertFilter(R"(["a", "b", "c"])", "[0, 1, 0]", R"(["b"])");
this->AssertFilter(R"([null, "b", "c"])", "[0, 1, 0]", R"(["b"])");
this->AssertFilter(R"(["a", "b", "c"])", "[null, 1, 0]", R"([null, "b"])");
+ this->AssertFilter(R"(["a", "long-value-over-inline-limit", null, "z"])",
+ "[1, 0, null, 1]", R"(["a", null, "z"])");
+ this->AssertFilter(R"(["a", null, "z"])", "[0, 1, 1]", R"([null, "z"])");
+}
+
+TEST_F(TestFilterKernel, FilterBinaryViewMultiplePayloadBuffers) {
+ for (const auto& value_type : BinaryViewTypes()) {
+ ARROW_SCOPED_TRACE(value_type->ToString());
+ auto values = MakeViewArrayWithMultiplePayloadBuffers(value_type);
+ auto filter = ArrayFromJSON(boolean(), "[false, false, true, true, true,
false]");
+ auto expected =
+ ArrayFromJSON(value_type, R"(["first-long-value", null,
"second-long-value"])");
+ this->AssertFilter(values, filter, expected);
+
+ ASSERT_OK_AND_ASSIGN(Datum actual_datum,
+ CallFunction("array_filter", {values, filter},
&this->drop_));
+ auto actual = actual_datum.make_array();
+ ValidateOutput(*actual);
+ AssertArraysEqual(*expected, *actual, /*verbose=*/true);
+
+ ASSERT_OK_AND_ASSIGN(auto ree_filter, REEncode(filter));
Review Comment:
Why do we test REE-encoding here?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -4368,6 +4368,53 @@ TEST(Cast, FromDictionary) {
}
}
+TEST(Cast, DictionaryDecodeFromViewDictionary) {
Review Comment:
This seems very long-winded, why not simply expand `FromDictionary` above?
##########
cpp/src/arrow/compute/kernels/vector_swizzle_test.cc:
##########
@@ -754,4 +755,44 @@ TEST(Scatter, Binary) {
}
}
+TEST(Scatter, BinaryView) {
+ for (const auto& value_type : BinaryViewTypes()) {
+ ARROW_SCOPED_TRACE(value_type->ToString());
+ {
+ auto values = R"(["a", "b", "long-value-over-inline-limit", "d"])";
+ auto indices = "[2, 0, 3, 1]";
+ int64_t max_index = 3;
+ auto expected = R"(["b", "d", "a", "long-value-over-inline-limit"])";
+ TestScatter(value_type, values, indices, max_index, expected);
+ }
+ {
+ auto values = R"(["a", "b", "c"])";
+ auto indices = "[2, null, 0]";
+ int64_t max_index = 4;
+ auto expected = R"(["c", null, "a", null, null])";
+ TestScatter(value_type, values, indices, max_index, expected);
+ }
+ {
+ auto values = R"(["first", "second"])";
+ auto indices = "[0, 0]";
+ int64_t max_index = 0;
+ auto expected = R"(["second"])";
+ TestScatter(value_type, values, indices, max_index, expected);
+ }
+ {
+ auto dict_values = ArrayFromJSON(
Review Comment:
Why test dictionaries here?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -2637,6 +2873,23 @@ TEST_F(TestDropNullKernelWithRecordBatch,
DropNullRecordBatch) {
this->AssertDropNull(schm, R"([])", R"([])");
}
+TEST_F(TestDropNullKernelWithRecordBatch, DropNullRecordBatchWithView) {
Review Comment:
Any reason we add this test?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -1675,6 +1778,139 @@ TYPED_TEST(TestTakeKernelWithString, TakeString) {
ASSERT_RAISES(IndexError, TakeCAC(type, {kABC, kABC}, "[4,
10]").Value(&chunked_arr));
}
+TEST_F(TestTakeKernel, TakeViewMultiplePayloadBuffers) {
+ for (const auto& value_type : BinaryViewTypes()) {
+ ARROW_SCOPED_TRACE(value_type->ToString());
+ auto values = MakeViewArrayWithMultiplePayloadBuffers(value_type);
+ auto indices = ArrayFromJSON(int32(), "[2, 4, 0, 5]");
+ auto expected = ArrayFromJSON(
+ value_type, R"(["first-long-value", "second-long-value", "", "z"])");
+
+ DoCheckTakeAAA(values, indices, expected);
+
+ ASSERT_OK_AND_ASSIGN(Datum actual_datum,
+ CallFunction("array_take", {values, indices}));
+ auto actual = actual_datum.make_array();
+ ValidateOutput(*actual);
+ AssertArraysEqual(*expected, *actual, /*verbose=*/true);
+ ASSERT_EQ(actual->data()->buffers.size(), values->data()->buffers.size());
+ ASSERT_EQ(values->data()->buffers[2].get(),
actual->data()->buffers[2].get());
+ ASSERT_EQ(values->data()->buffers[3].get(),
actual->data()->buffers[3].get());
+
+ auto sliced_values = values->Slice(1, 4);
+ auto sliced_indices = ArrayFromJSON(int32(), "[1, 3, 0]");
+ auto sliced_expected =
+ ArrayFromJSON(value_type, R"(["first-long-value", "second-long-value",
"tiny"])");
+ DoAssertTakeAAA(sliced_values, sliced_indices, sliced_expected);
+
+ auto indices_with_offset = ArrayFromJSON(int32(), "[0, 4, null, 2,
5]")->Slice(1, 3);
+ auto expected_with_offset =
+ ArrayFromJSON(value_type, R"(["second-long-value", null,
"first-long-value"])");
+ DoAssertTakeAAA(values, indices_with_offset, expected_with_offset);
+ }
+}
+
+TEST_F(TestTakeKernel, TakeViewDropsUnusedPayloadBuffers) {
+ struct TestCase {
+ const char* indices;
+ const char* expected;
+ bool uses_first;
+ bool uses_second;
+ };
+ const TestCase cases[] = {
+ {"[4, 3, 4, null, 1]",
+ R"(["second-long-value", null, "second-long-value", null, "tiny"])",
false, true},
+ {"[2, 1, 2]", R"(["first-long-value", "tiny", "first-long-value"])",
true, false},
+ {"[4, 2]", R"(["second-long-value", "first-long-value"])", true, true},
+ {"[0, 1, 5, null, 3]", R"(["", "tiny", "z", null, null])", false, false},
+ {"[3, null, 3]", "[null, null, null]", false, false},
+ {"[]", "[]", false, false},
+ };
+ for (const auto& value_type : BinaryViewTypes()) {
+ for (const auto& test : cases) {
+ ARROW_SCOPED_TRACE(value_type->ToString(), " indices = ", test.indices);
+ auto values = MakeViewArrayWithMultiplePayloadBuffers(value_type);
+ auto indices = ArrayFromJSON(int32(), test.indices);
+ auto expected = ArrayFromJSON(value_type, test.expected);
+ std::weak_ptr<Buffer> first = values->data()->buffers[2];
+ std::weak_ptr<Buffer> second = values->data()->buffers[3];
+
+ ASSERT_OK_AND_ASSIGN(auto actual, TakeAAA(*values, *indices));
+ ValidateOutput(*actual);
+ AssertArraysEqual(*expected, *actual);
+ const auto& buffers = actual->data()->buffers;
+ ASSERT_EQ(buffers.size(), test.uses_second ? 4 : (test.uses_first ? 3 :
2));
+ if (test.uses_first) {
+ ASSERT_EQ(buffers[2], first.lock());
+ } else if (test.uses_second) {
+ ASSERT_NE(buffers[2], nullptr);
+ ASSERT_EQ(buffers[2]->size(), 0);
+ ASSERT_EQ(buffers[2]->parent(), nullptr);
+ }
+ if (test.uses_second) {
+ ASSERT_EQ(buffers[3], second.lock());
+ }
+ values.reset();
+ ASSERT_EQ(first.expired(), !test.uses_first);
+ ASSERT_EQ(second.expired(), !test.uses_second);
+
+ // Empty placeholders must also be safe to export and import.
+ ArrowArray c_array{};
+ ASSERT_OK(ExportArray(*actual, &c_array));
+ ASSERT_OK_AND_ASSIGN(auto imported, ImportArray(&c_array, value_type));
+ ValidateOutput(*imported);
+ AssertArraysEqual(*expected, *imported);
+ }
+ }
+}
+
+TEST_F(TestTakeKernel, TakeViewValidityFollowsIndices) {
Review Comment:
Hmm, is this testing something specific?
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -2812,6 +3073,26 @@ TEST_F(TestDropNullKernelWithTable, DropNullTable) {
AssertSchemaEqual(schm, actual->schema());
ASSERT_EQ(actual->num_rows(), 0);
}
+
+ for (const auto& value_type : BinaryViewTypes()) {
Review Comment:
Same question.
##########
cpp/src/arrow/compute/kernels/vector_selection_test.cc:
##########
@@ -2692,6 +2945,14 @@ TEST_F(TestDropNullKernelWithChunkedArray,
DropNullChunkedArray) {
this->AssertDropNull(int8(), {"[null]", "[null, null]"}, {"[]"});
this->AssertDropNull(int8(), {"[7]", "[8, 9]"}, {"[7]", "[8, 9]"});
this->AssertDropNull(int8(), {"[]", "[]"}, {"[]", "[]"});
+
+ for (const auto& value_type : BinaryViewTypes()) {
+ ARROW_SCOPED_TRACE(value_type->ToString());
+ this->AssertDropNull(
+ value_type,
+ {R"(["a", null])", R"([null, "long-value-over-inline-limit"])",
R"([""])"},
+ {R"(["a"])", R"(["long-value-over-inline-limit"])", R"([""])"});
+ }
Review Comment:
Same question here: this function is not trying to exercise all data types.
--
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]
