GH-14939: [C++] Support Table lookups in FieldRef and FieldPath #34537

dinimar · 2023-03-11T11:01:48Z

Rationale for this change

Described in the issue

What changes are included in this PR?

added implementations for FieldPath::Get(const Table& table) and FindAll(const Table& table)
added unit tests for functions mentioned above both for Table and RecordBatch classes

Are these changes tested?

Yes, by unit tests

Are there any user-facing changes?

Most probably, no

Closes: [C++] Support Table lookups in FieldRef and FieldPath #14939

- add overloaded implementations of FieldRef::FindAll and FieldPath::Get for Table type - add unit tests for FieldRef::FindAll and FieldPath::Get both for RecordBatch and Table types

- fix if-statement formatting

github-actions · 2023-03-11T11:02:07Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2023-03-11T11:03:08Z

Closes: [C++] Support Table lookups in FieldRef and FieldPath #14939

dinimar · 2023-03-11T12:07:04Z

@benibus please review

dinimar · 2023-03-12T06:59:56Z

cc @rok @pitrou @benibus

benibus

Thanks @dinimar! I just did a first-pass and left some comments.

cpp/src/arrow/type.cc

cpp/src/arrow/type_test.cc

benibus · 2023-03-23T15:07:46Z

@dinimar I pushed a commit that reverts some of the formatting. I'm not entirely sure why the auto-formatter is still picking up arrow/compute/kernels/scalar_string_test.cc - probably best to leave it for now.

I'm going to add some more commits addressing the nested-field tests soon. Then I'll do another round of review :-)

benibus · 2023-03-23T21:35:03Z

@zeroshade @westonpace Looking at this a little deeper... do you know if FieldPath::Get is meant to be zero-copy? I'm not entirely sure if my suggestion to use ChunkedArray::Flatten here was appropriate since I believe it collapses top-level nulls. The Get implementations for Array and RecordBatch both use their underlying ArrayData exclusively without modifying/copying anything (regardless of nesting level).

Achieving the same thing for ChunkedArrays would be more advanced, of course - but I can help with the implementation.

dinimar · 2023-03-24T09:33:19Z

@dinimar I pushed a commit that reverts some of the formatting. I'm not entirely sure why the auto-formatter is still picking up arrow/compute/kernels/scalar_string_test.cc - probably best to leave it for now.

I'm going to add some more commits addressing the nested-field tests soon. Then I'll do another round of review :-)

About formatting it's possible to fix it by rebasing on the last successful commit (of course after all work is done). For example, yours 4487be. It passed all workflows including formatting check

westonpace · 2023-03-24T16:06:41Z

@zeroshade @westonpace Looking at this a little deeper... do you know if FieldPath::Get is meant to be zero-copy? I'm not entirely sure if my suggestion to use ChunkedArray::Flatten here was appropriate since I believe it collapses top-level nulls. The Get implementations for Array and RecordBatch both use their underlying ArrayData exclusively without modifying/copying anything (regardless of nesting level).

Yes, Get is meant to be zero-copy. This came up in discussion a little while back. See #14946 I believe the consensus was that FieldRef::Get has been around long enough that changing the definition now (to mean a flattened get) would be a breaking change and a better approach would be to introduce a GetFlattened alongside Get.

benibus

I've pushed commits that address the tests and zero-copy requirement (the latter required some implementation changes, but it's similar in principle). I also added overloads for ChunkedArrays for good measure.

Hopefully the tests better-clarify what I was getting at regarding nested fields, but let me know if you have any questions!

cpp/src/arrow/type_test.cc

westonpace

A few minor suggestions. Marking Request Changes because it isn't clear to me that a follow-up issue for creating generic tests has yet been created. Once we do that I think we can move forward with this.

westonpace · 2023-03-28T00:08:29Z

cpp/src/arrow/type.cc

+Result<std::shared_ptr<ChunkedArray>> FieldPath::Get(
+    const ChunkedArray& chunked_array) const {
+  if (chunked_array.num_chunks() < 1) {
+    return Status::Invalid("Chunked array must have at least one chunk");


I was under the impression that it would be considered invalid since ChunkedArray::MakeEmpty always allocates one chunk. https://github.com/dinimar/arrow/blob/ec6caeff8477af08679de7a7f12b90d4ba302457/cpp/src/arrow/chunked_array.h#L95-L96

Whether that's reasonable or not, I'm open to just returning an empty ChunkedArray here (based on its type or storage_type) if that makes more sense.

Ah, ok. I very rarely work with chunked arrays and so you may be right. It is possible to create chunks arrays with 0 chunks though:

>>> pa.chunked_array([], pa.int8()).num_chunks 0

My comment was mostly for understanding. I thought maybe a chunk was needed for some kind of inference.

I think I would slightly prefer returning an empty chunked array (with 1 chunk for consistency with ChunkedArray::MakeEmpty).

@benibus please resolve this converstion. At the moment, it's the last thing to be done before second review from Weston Pace

cpp/src/arrow/type_test.cc

benibus · 2023-03-31T19:41:40Z

I've created an issue for the tests: #34830

- review remarks

dinimar · 2023-04-08T09:35:20Z

@westonpace please review

westonpace

Thank you both for working on this.

ursabot · 2023-04-13T06:14:10Z

Benchmark runs are scheduled for baseline = 1bda003 and contender = 1c06854. 1c06854 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.82% ⬆️1.05%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 1c06854f ec2-t3-xlarge-us-east-2
[Failed] 1c06854f test-mac-arm
[Finished] 1c06854f ursa-i9-9960x
[Finished] 1c06854f ursa-thinkcentre-m75q
[Finished] 1bda003b ec2-t3-xlarge-us-east-2
[Failed] 1bda003b test-mac-arm
[Finished] 1bda003b ursa-i9-9960x
[Finished] 1bda003b ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…apache#34537) ### Rationale for this change Described in the issue ### What changes are included in this PR? - added implementations for `FieldPath::Get(const Table& table)` and `FindAll(const Table& table)` - added unit tests for functions mentioned above both for `Table` and `RecordBatch` classes ### Are these changes tested? Yes, by unit tests ### Are there any user-facing changes? Most probably, no * Closes: apache#14939 Lead-authored-by: Dinir Imameev <dinir.imameev@gmail.com> Co-authored-by: benibus <bpharks@gmx.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

dinimar added 2 commits March 11, 2023 13:14

apacheGH-14939: [C++] Support Table lookups in FieldRef and FieldPath

e4ccb4c

- add overloaded implementations of FieldRef::FindAll and FieldPath::Get for Table type - add unit tests for FieldRef::FindAll and FieldPath::Get both for RecordBatch and Table types

apacheGH-14939: [C++] Support Table lookups in FieldRef and FieldPath

ff031c4

- fix if-statement formatting

github-actions bot added Component: C++ awaiting review Awaiting review labels Mar 11, 2023

dinimar changed the title ~~GH014939: [C++] Support Table lookups in FieldRef and FieldPath~~ GH-14939: [C++] Support Table lookups in FieldRef and FieldPath Mar 11, 2023

dinimar marked this pull request as ready for review March 11, 2023 11:47

benibus reviewed Mar 17, 2023

View reviewed changes

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

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

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

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

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

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 17, 2023

dinimar requested review from assignUser, kou, raulcd, paleolimbot, thisisnic, AlenkaF, domoritz, trxcllnt, lidavidm, zeroshade, westonpace and wjones127 as code owners March 19, 2023 12:45

github-actions bot added Component: C++ - Gandiva Component: Documentation Component: GLib Component: Go Component: Java labels Mar 19, 2023

benibus added 4 commits March 26, 2023 20:20

Make FieldPath::Get zero-copy for tables

6d99628

Add FieldPath::Get overload for ChunkedArray

0633937

Tests for Table and ChunkedArray

87f5247

Add FieldRef::FindAll overload for ChunkedArray

ec6caef

benibus reviewed Mar 27, 2023

View reviewed changes

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

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

dinimar requested a review from benibus March 28, 2023 07:17

benibus requested a review from westonpace March 29, 2023 19:34

westonpace requested changes Mar 31, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 31, 2023

apacheGH-14939: [C++] Support Table lookups in FieldRef and FieldPath

bcb7299

- review remarks

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 2, 2023

Better handling of cases with zero chunks

cfa3fae

benibus requested a review from westonpace April 3, 2023 20:35

westonpace approved these changes Apr 11, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 11, 2023

westonpace merged commit 1c06854 into apache:main Apr 11, 2023
27 of 33 checks passed

benibus mentioned this pull request Apr 24, 2023

GH-14946: [C++] Add flattening FieldPath/FieldRef::Get methods #35197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-14939: [C++] Support Table lookups in FieldRef and FieldPath #34537

GH-14939: [C++] Support Table lookups in FieldRef and FieldPath #34537

dinimar commented Mar 11, 2023 •

edited by github-actions bot

github-actions bot commented Mar 11, 2023

github-actions bot commented Mar 11, 2023

dinimar commented Mar 11, 2023

dinimar commented Mar 12, 2023

benibus left a comment

benibus commented Mar 23, 2023

benibus commented Mar 23, 2023

dinimar commented Mar 24, 2023

westonpace commented Mar 24, 2023

benibus left a comment

westonpace left a comment

westonpace Mar 28, 2023

benibus Mar 31, 2023

westonpace Mar 31, 2023

dinimar Apr 3, 2023

benibus commented Mar 31, 2023

dinimar commented Apr 8, 2023

westonpace left a comment

ursabot commented Apr 13, 2023

GH-14939: [C++] Support Table lookups in FieldRef and FieldPath #34537

GH-14939: [C++] Support Table lookups in FieldRef and FieldPath #34537

Conversation

dinimar commented Mar 11, 2023 • edited by github-actions bot

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Mar 11, 2023

github-actions bot commented Mar 11, 2023

dinimar commented Mar 11, 2023

dinimar commented Mar 12, 2023

benibus left a comment

Choose a reason for hiding this comment

benibus commented Mar 23, 2023

benibus commented Mar 23, 2023

dinimar commented Mar 24, 2023

westonpace commented Mar 24, 2023

benibus left a comment

Choose a reason for hiding this comment

westonpace left a comment

Choose a reason for hiding this comment

westonpace Mar 28, 2023

Choose a reason for hiding this comment

benibus Mar 31, 2023

Choose a reason for hiding this comment

westonpace Mar 31, 2023

Choose a reason for hiding this comment

dinimar Apr 3, 2023

Choose a reason for hiding this comment

benibus commented Mar 31, 2023

dinimar commented Apr 8, 2023

westonpace left a comment

Choose a reason for hiding this comment

ursabot commented Apr 13, 2023

dinimar commented Mar 11, 2023 •

edited by github-actions bot