[GitHub] [arrow] wjones127 commented on a diff in pull request #14749: ARROW-18280: [C++][Python] Support slicing to end in list_slice kernel

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

wjones127 commented on code in PR #14749:
URL: https://github.com/apache/arrow/pull/14749#discussion_r1034147079


##########
cpp/src/arrow/compute/kernels/scalar_nested_test.cc:
##########
@@ -168,6 +173,20 @@ TEST(TestScalarNested, ListSliceFixedOutput) {
       expected = ArrayFromJSON(fixed_size_list(value_type, 2),
                                "[[3, null], [null, null], [null, null], null]");
       CheckScalarUnary("list_slice", input, expected, &args);
+
+      args.start = 1;
+      args.stop = std::nullopt;
+      expected = ArrayFromJSON(fixed_size_list(value_type, 2),
+                               "[[2, 3], [5, null], [null, null], null]");
+      if (input->type()->id() == Type::FIXED_SIZE_LIST) {
+        CheckScalarUnary("list_slice", input, expected, &args);
+      } else {
+        EXPECT_RAISES_WITH_MESSAGE_THAT(
+            NotImplemented,
+            ::testing::HasSubstr("Unable to produce FixedSizeListArray from "
+                                 "non-FixedSizeListArray without `stop` being set."),
+            CallFunction("list_slice", {input}, &args));
+      }

Review Comment:
   Is this a case that is possible?
   
   ```suggestion
         }
         
         args.start = 3;
         args.stop = std::nullopt;
         expected = ArrayFromJSON(fixed_size_list(value_type, 0),
                                  "[[], [], [], null]");
         if (input->type()->id() == Type::FIXED_SIZE_LIST) {
           CheckScalarUnary("list_slice", input, expected, &args);
         }
   ```



##########
cpp/src/arrow/compute/kernels/scalar_nested.cc:
##########
@@ -131,13 +124,26 @@ struct ListSlice {
         list_type->id() == arrow::Type::FIXED_SIZE_LIST);
     std::unique_ptr<ArrayBuilder> builder;
 
+    // should have been checked in resolver
+    // if stop not set, then cannot return fixed size list without input being fixed size
+    // list b/c we cannot determine the max list element in type resolving.
+    DCHECK(opts.stop.has_value() ||
+           (!opts.stop.has_value() && (!return_fixed_size_list ||
+                                       list_type->id() == arrow::Type::FIXED_SIZE_LIST)));
+
     // construct array values
     if (return_fixed_size_list) {
-      RETURN_NOT_OK(MakeBuilder(
-          ctx->memory_pool(),
-          fixed_size_list(value_type,
-                          static_cast<int32_t>(opts.stop.value() - opts.start)),
-          &builder));
+      const auto stop = [&]() {
+        if (opts.stop.has_value()) {
+          return static_cast<int32_t>(opts.stop.value());
+        } else {
+          DCHECK_EQ(list_type->id(), arrow::Type::FIXED_SIZE_LIST);
+          return checked_cast<const FixedSizeListType*>(list_type)->list_size();
+        }
+      }();
+      const auto size = std::max(stop - static_cast<int32_t>(opts.start), 0);

Review Comment:
   Isn't this guaranteed to be non-negative?



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org