[GitHub] [arrow] pitrou commented on a change in pull request #7382: ARROW-5760: [C++] New compute::Take implementation for better performance, faster dispatch, smaller code size / faster compilation

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #7382:
URL: https://github.com/apache/arrow/pull/7382#discussion_r438759834



##########
File path: cpp/src/arrow/util/bit_block_counter.h
##########
@@ -17,14 +17,21 @@
 
 #pragma once
 
+#include <algorithm>
 #include <cstdint>
+#include <limits>
+#include <memory>
 
+#include "arrow/array/array_base.h"

Review comment:
       Is this useful?

##########
File path: cpp/src/arrow/compute/kernels/vector_take_test.cc
##########
@@ -211,13 +306,63 @@ TYPED_TEST(TestTakeKernelWithString, TakeString) {
                                      "[2, 5]", &arr));
 }
 
+TEST(TestTakeKernelString, Random) {
+  DoRandomTakeTests<StringType>(
+      [](int64_t length, double null_probability, random::RandomArrayGenerator* rng) {
+        return rng->String(length, 0, 32, null_probability);
+      });
+  DoRandomTakeTests<LargeStringType>(
+      [](int64_t length, double null_probability, random::RandomArrayGenerator* rng) {
+        return rng->LargeString(length, 0, 32, null_probability);
+      });

Review comment:
       Do you think you can add tests for sliced arrays with a non-0 offset? Both for primitive types and string-like types.

##########
File path: cpp/src/arrow/compute/kernels/vector_take_test.cc
##########
@@ -72,6 +73,120 @@ void AssertTakeBoolean(const std::string& values, const std::string& indices,
   CheckTake(boolean(), values, indices, expected);
 }
 
+template <typename ValuesType, typename IndexType>
+void ValidateTakeImpl(const std::shared_ptr<Array>& values,
+                      const std::shared_ptr<Array>& indices,
+                      const std::shared_ptr<Array>& result) {
+  using ValuesArrayType = typename TypeTraits<ValuesType>::ArrayType;
+  using IndexArrayType = typename TypeTraits<IndexType>::ArrayType;
+  auto typed_values = checked_pointer_cast<ValuesArrayType>(values);
+  auto typed_result = checked_pointer_cast<ValuesArrayType>(result);
+  auto typed_indices = checked_pointer_cast<IndexArrayType>(indices);
+  for (int64_t i = 0; i < indices->length(); ++i) {
+    if (typed_indices->IsNull(i) || typed_values->IsNull(typed_indices->Value(i))) {
+      ASSERT_TRUE(result->IsNull(i));
+      continue;
+    }
+    ASSERT_EQ(typed_result->GetView(i), typed_values->GetView(typed_indices->Value(i)))

Review comment:
       Also add `ASSERT_FALSE(result->IsNull(i))`?




----------------------------------------------------------------
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