ARROW-18265: [C++][Python] Support FieldRef to work with ListElement #14697

milesgranger · 2022-11-22T12:29:59Z

Support list types other than Type::LIST
Add C++ tests

Example

In [1]: arr = pa.array([{'a': [{'b': 1}, {'c': 'hi'}]}])

In [2]: pc.struct_field(arr, '.a[1].c')
Out[2]:
<pyarrow.lib.StringArray object at 0x7efbc741f040>
[
  "hi"
]

In [3]: pc.struct_field(arr, '.a[0].b')
Out[3]:
<pyarrow.lib.Int64Array object at 0x7efc77398fa0>
[
  1
]

In [4]:

github-actions · 2022-11-22T12:30:16Z

https://issues.apache.org/jira/browse/ARROW-18265

cpp/src/arrow/type.cc

jorisvandenbossche

(just a quick look at the tests)

cpp/src/arrow/type_test.cc

parent was not passed in FieldPath first iteration

jorisvandenbossche · 2022-12-06T15:51:36Z

cpp/src/arrow/compute/kernels/scalar_nested.cc

+  static bool IsBaseListType(const DataType& type) {
+    return dynamic_cast<const BaseListType*>(&type) != nullptr;
+  }


We have is_list_like helper defined (although that also includes Map type, not sure if the list_element kernel supports that)

Also, if we keep this, using a combination of a few type.id() == .. checks (like ValidParentType just below) might be more readable to know which types are included)

jorisvandenbossche · 2022-12-06T16:01:02Z

cpp/src/arrow/compute/kernels/scalar_nested.cc

    if (!ValidParentType(type)) {
      return Status::TypeError("struct_field: cannot subscript field of type ", type);
-    } else if (index < 0 || index >= type.num_fields()) {
+    } else if (!IsBaseListType(type) && (index < 0 || index >= type.num_fields())) {


In theory, we could check the index for fixed sized list arrays as well (not for variable size list arrays, though, so not sure that is worth it)

jorisvandenbossche · 2022-12-06T16:33:26Z

cpp/src/arrow/type.cc

+        // Children then are fields from list type, and out will be the children from
+        // that field, thus index 0.
+        if (out != nullptr && IsBaseListType(*(*out)->type())) {
+          children = get_children(*out);


I don't fully understand why we need the get_children here (because then we call get_children twice in a single iteration? since there is another call a bit below)
Can you add a clarifying comment about that?

(or to put it differently: why isn't this initial get_children needed when parent is a struct type? What's the difference between both?)

jorisvandenbossche · 2022-12-06T16:42:14Z

cpp/src/arrow/type_test.cc

+    // FindAll, only used by kernels for which list element to take. When FindAll
+    // encounters a list, it assumes the list value type. Not b/c they are numeric, but
+    // they are the position referencing a specific list element.
+    EXPECT_THAT(FieldRef("c", 0, "d").FindAll(*type), ElementsAre(FieldPath{0, 0, 0}));


Can you also add variants of those tests that pass a schema instead of type to FindAll?

pitrou · 2022-12-12T14:47:04Z

I find it a bit uneasy how this PR is conflating a purely metadata-based field lookup (the current semantics of FieldRef) and a dynamic index lookup into a list. Is this actually important to support?

amol- · 2022-12-12T15:15:33Z

I find it a bit uneasy how this PR is conflating a purely metadata-based field lookup (the current semantics of FieldRef) and a dynamic index lookup into a list. Is this actually important to support?

Not sure if FieldRef is the right answer to the problem, but in general I think that what we want to achieve is support for indexing specific subfields in entries field.subfield and field.arraysubfield.0 and so on. This seems a fairly standard feature and not far from what MongoDB provides for example ( https://www.mongodb.com/docs/manual/core/document/#dot-notation )

jorisvandenbossche · 2022-12-13T13:21:49Z

a purely metadata-based field lookup (the current semantics of FieldRef)

I am not sure that this is still entirely correct nowadays, since we already do use FieldRef in other places in the compute engine to refer to fields (eg to specify the sort key, in projections (although there wrapped in an expression), ...).
And it is certainly true that right now we only support struct/union field lookups, but also those are not purely metadata-based, as the actual lookup of values (not just the type/field from a schema) also needs to potentially calculate validity bitmaps. While for list types, the lookup of the type is also metadata-only. It's only when looking up actual values that you need a nontrivial computation.

Is this actually important to support?

If you want to work with list types, this is a pretty basic operation. Now, the actual operation is already supported (list_element kernel), here it is only about being able to express this in a field reference. So this is somewhat in the bucket of "user convenience", as you can already achieve something similar with the kernel.
Now, for example also Substrait has this concept (https://github.com/substrait-io/substrait/blob/7f272f13f22cd5f5842baea42bcf7961e6251881/proto/substrait/algebra.proto#L932-L938)

Another example of database that supports this in queries: https://cloud.google.com/bigquery/docs/reference/standard-sql/arrays#accessing_array_elements

jorisvandenbossche · 2022-12-13T13:24:40Z

And to be clear, that's also basically the question that I asked on the JIRA: "do we want to use FieldRef/FieldPath for this?" (because it does deviate from how it is currently implemented) It could also be a different (new to add) data structure (that would be used throughout the compute code instead of the current FieldRef/FieldPath)

pitrou · 2022-12-13T14:16:49Z

Hmm... The convenience argument is convincing, I admit.

Still, I wonder if we shouldn't make it optional in FieldRef, perhaps by adding a bool allow_list_lookup = false argument to FieldRef lookup APIs?

amol- · 2023-03-30T17:08:57Z

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

github-actions bot added Component: C++ Component: Python labels Nov 22, 2022

jorisvandenbossche reviewed Nov 22, 2022

View reviewed changes

cpp/src/arrow/type.cc Outdated Show resolved Hide resolved

github-actions bot added Component: FlightRPC and removed Component: FlightRPC labels Nov 22, 2022

milesgranger added 4 commits December 2, 2022 11:45

[skip ci] Support FieldRef to work with ListElement

ae9e603

[skip ci] Add C++ test for FieldRef/FieldPath

b06f715

[skip ci] Pass parent DataType when available to FieldPathGetImpl

5a082ae

Support other list types

a12ecb1

milesgranger marked this pull request as ready for review December 2, 2022 10:45

jorisvandenbossche reviewed Dec 2, 2022

View reviewed changes

cpp/src/arrow/type_test.cc Show resolved Hide resolved

Expand C++ tests and fix for leading list type

99f829c

parent was not passed in FieldPath first iteration

jorisvandenbossche reviewed Dec 6, 2022

View reviewed changes

This was referenced Dec 14, 2022

[C++] Should FieldRef::Get* for a StructArray return the "raw" child array or the "flattened" field array? #14946

Closed

[C++] Rename "struct_field" kernel, or add new kernel for generic FieldRef selection in projections? #14971

Open

asfimport mentioned this pull request Dec 13, 2022

[C++] Allow FieldPath to work with ListElement #20489

Closed

amol- closed this Mar 30, 2023

milesgranger deleted the ARROW-18265_fieldpath-work-with-list-element branch March 31, 2023 06:45

ARROW-18265: [C++][Python] Support FieldRef to work with ListElement #14697

ARROW-18265: [C++][Python] Support FieldRef to work with ListElement #14697

Uh oh!

Conversation

milesgranger commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 22, 2022

Uh oh!

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

pitrou commented Dec 12, 2022

Uh oh!

amol- commented Dec 12, 2022

Uh oh!

jorisvandenbossche commented Dec 13, 2022

Uh oh!

jorisvandenbossche commented Dec 13, 2022

Uh oh!

pitrou commented Dec 13, 2022

Uh oh!

amol- commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

milesgranger commented Nov 22, 2022 •

edited

Loading