You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/22 14:05:00 UTC

[GitHub] [arrow] milesgranger commented on a diff in pull request #14697: ARROW-18265: [C++][Python] Support FieldRef to work with ListElement

milesgranger commented on code in PR #14697:
URL: https://github.com/apache/arrow/pull/14697#discussion_r1029371327


##########
cpp/src/arrow/type.cc:
##########
@@ -1066,12 +1066,32 @@ struct FieldPathGetImpl {
     }
 
     int depth = 0;
-    const T* out;
+    const T* out = nullptr;
     for (int index : path->indices()) {
       if (children == nullptr) {
         return Status::NotImplemented("Get child data of non-struct array");
       }
 
+      if constexpr (std::is_same_v<T, std::shared_ptr<arrow::Field>>) {
+        // For lists, we don't care about the index, jump right into the list value type.
+        // The index here is used in the kernel to grab the specific list element.
+        // Children then are fields from list type, and out will be the children from
+        // that field, thus index 0.
+        if (out != nullptr && (*out)->type()->id() == Type::LIST) {
+          children = get_children(*out);
+          index = 0;
+        } else if (out == nullptr && children->size() == 1 && index >= 0) {
+          // WIP: Perhaps a dangerous assumption?
+          // For list types when previous was not a FieldPath, but
+          // a string name referencing a list type. Then the first iteration
+          // with index (referencing the list item), might look like it's
+          // out of bounds when actually we don't care about this index for
+          // type checking, only the kernel does; we need the list value type
+          // which is now the only child in the children vector.

Review Comment:
   Sure, this path happens with the `".a[1].c"` case, where the 'a' goes through string name, and the second '[1]' is through this FieldPath, but is also out of bounds from children (index 1) whereas `".a[0].c"` works b/c 0 is the index we want anyway from the list type. 
   
   I'll work on adding the FieldPath::Get tests in C++. Thanks!



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