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

benibus · 2023-04-17T18:49:09Z

Rationale for this change

The current FieldPath::Get methods - when extracting nested child values - don't combine the child's null bitmap with higher-level parent bitmaps. While this is often preferable (it allows for zero-copy), there are cases where higher level "flattening" version is useful - e.g. when pre-processing sort keys for structs.

What changes are included in this PR?

Adds FieldPath::GetFlattened methods alongside the existing FieldPath::Get overloads
Adds GetAllFlattened, GetOneFlattened and GetOneOrNoneFlattened methods to FieldRef
Adds a couple internal helpers for dealing with both Get variants in templates
Overhauls the FieldPath tests in an effort to improve coverage and generalize across the supported input types

More significantly, this alters the FieldPathGetImpl internals to use a new NestedSelector class. The reason for this is that the prior method required presenting a vector of instantiated child values for each depth level prior to selection. With support for flattening (and recently, ChunkedArrays), this becomes a problem since we need to explicitly create all of those child values for each depth level despite the fact that we're only going to select one. So these changes allow any expensive instantiations to be deferred to selection time.

This also indirectly solves an issue that surfaced in the new tests, which is that FieldPath::Get would return incorrect nested values when sliced Arrays are involved. This is because the underlying child data's offset/length weren't being adjusted based on the parent.

Are these changes tested?

Yes (tests are included)

Are there any user-facing changes?

Yes, this adds methods to a public API

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

github-actions · 2023-04-17T18:49:35Z

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

github-actions · 2023-04-17T18:49:37Z

⚠️ GitHub issue #14946 has been automatically assigned in GitHub to PR creator.

benibus · 2023-04-18T20:59:06Z

Probably worth mentioning that this (indirectly) addresses most of #34830 as well - at least on the FieldPath side of things.

benibus · 2023-04-19T15:26:28Z

@zeroshade @felipecrv

cpp/src/arrow/type.cc

cpp/src/arrow/type.h

felipecrv · 2023-05-02T22:51:43Z

@benibus can you ping the people who understood the issue in this PR?

benibus · 2023-05-03T15:26:47Z

@westonpace I feel you're probably most qualified to look at this given your comments on the original issue.

benibus · 2023-05-10T14:30:24Z

cc @pitrou

pitrou

Thanks a lot for this work! This looks generally good, just a bunch of relatively minor suggestions.

cpp/src/arrow/type.h

cpp/src/arrow/type_test.cc

pitrou · 2023-05-17T13:25:06Z

cpp/src/arrow/type.h

Uh. Unlike GetAll, this can fail for various reasons such as failure to allocate enough memory. In that case, we'd probably want to return an error instead of dying out.

In that case, I'm a little confused as to why the non-flattened variants don't forward those errors either - since the standard FieldPath::Get methods can also fail (which was true prior to this PR).

I'm mostly referring to GetOne and GetOneAndNone here, as they already return a Result. Regardless, I'll propagate those errors for the new methods.

pitrou · 2023-05-17T13:25:42Z

cpp/src/arrow/type.h

Same here (and this method is already returning a Result!).

pitrou · 2023-05-17T13:25:59Z

cpp/src/arrow/type.h

pitrou · 2023-05-17T13:30:54Z

cpp/src/arrow/type_test.cc

I might have missed it somehow, but we probably want to call ValidateFull on the actual results here (and below).

pitrou · 2023-05-17T13:32:57Z

cpp/src/arrow/type_test.cc

Given that this is adding quite a lot of test code, and this file is already long, I think it may be time to move the Field{Ref,Path} tests to a separate test module.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou

+1, thanks for the update! Will merge if CI is green.

pitrou · 2023-05-22T14:53:05Z

CI failures are unrelated.

ursabot · 2023-05-30T21:25:04Z

Benchmark runs are scheduled for baseline = 2216a0a and contender = f3500f6. f3500f6 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
[Finished ⬇️0.45% ⬆️0.06%] test-mac-arm
[Finished ⬇️2.94% ⬆️1.63%] ursa-i9-9960x
[Finished ⬇️1.82% ⬆️0.73%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f3500f65 ec2-t3-xlarge-us-east-2
[Finished] f3500f65 test-mac-arm
[Finished] f3500f65 ursa-i9-9960x
[Finished] f3500f65 ursa-thinkcentre-m75q
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 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

ursabot · 2023-05-31T02:55:32Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

…eys (#35727) ### Rationale for this change We don't currently support sorting `StructArray`s despite already having the high-level facilities to do so. For instance, we allow passing multiple sort keys (based on `FieldRef`s) to sort record batches and tables - but the current implementations are fairly limited since nested refs aren't allowed (due to the burden of null flattening). Since #35197, we now have an easier way to do this. ### What changes are included in this PR? - Adds support for `StructArray` in `sort_indices` - Adds support for nested sort keys in `sort_indices` for `RecordBatch`, `ChunkedArray`, and `Table` ### Are these changes tested? Yes (tests are included) ### Are there any user-facing changes? Yes * Closes: #33206 Authored-by: benibus <bpharks@gmx.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

### Rationale for this change #35197 appears to have introduced significant performance regressions in `FieldPath::Get` - indicated [here](https://conbench.ursa.dev/compare/runs/9cf73ac83f0a44179e6538b2c1c7babd...3d76cb5ffb8849bf8c3ea9b32d08b3b7/), in a benchmark that uses a wide (10K column) dataframe. ### What changes are included in this PR? - Adds basic benchmarks for `FieldPath::Get` across various input types, as they didn't previously exist - Addresses several performance issues. These came in the form of extremely high upfront costs for the `RecordBatch` and `ArrayData` overloads specifically - Some minor refactoring of `NestedSelector` ### Are these changes tested? Yes (covered by existing tests) ### Are there any user-facing changes? No * Closes: #36892 Lead-authored-by: benibus <bpharks@gmx.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

…apache#37032) ### Rationale for this change apache#35197 appears to have introduced significant performance regressions in `FieldPath::Get` - indicated [here](https://conbench.ursa.dev/compare/runs/9cf73ac83f0a44179e6538b2c1c7babd...3d76cb5ffb8849bf8c3ea9b32d08b3b7/), in a benchmark that uses a wide (10K column) dataframe. ### What changes are included in this PR? - Adds basic benchmarks for `FieldPath::Get` across various input types, as they didn't previously exist - Addresses several performance issues. These came in the form of extremely high upfront costs for the `RecordBatch` and `ArrayData` overloads specifically - Some minor refactoring of `NestedSelector` ### Are these changes tested? Yes (covered by existing tests) ### Are there any user-facing changes? No * Closes: apache#36892 Lead-authored-by: benibus <bpharks@gmx.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions bot added Component: C++ awaiting review Awaiting review labels Apr 17, 2023

benibus force-pushed the GH-14946-field-ref-flattened-get branch 2 times, most recently from 564e319 to 5e0920d Compare April 18, 2023 16:27

benibus marked this pull request as ready for review April 18, 2023 20:58

felipecrv reviewed Apr 24, 2023

View reviewed changes

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

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 24, 2023

felipecrv reviewed Apr 24, 2023

View reviewed changes

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

felipecrv reviewed Apr 24, 2023

View reviewed changes

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

benibus force-pushed the GH-14946-field-ref-flattened-get branch 2 times, most recently from 8c83fd6 to a9fddef Compare May 1, 2023 19:21

pitrou requested changes May 15, 2023

View reviewed changes

benibus force-pushed the GH-14946-field-ref-flattened-get branch 2 times, most recently from 15bb2ee to 96cebfe Compare May 16, 2023 21:42

benibus requested a review from pitrou May 17, 2023 12:50

pitrou reviewed May 17, 2023

View reviewed changes

cpp/src/arrow/type.h Outdated

Copy link

Member

pitrou May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (and this method is already returning a Result!).

pitrou reviewed May 17, 2023

View reviewed changes

cpp/src/arrow/type.h Outdated

Copy link

Member

pitrou May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

pitrou reviewed May 17, 2023

View reviewed changes

benibus force-pushed the GH-14946-field-ref-flattened-get branch from 96cebfe to 17a6591 Compare May 17, 2023 18:10

benibus requested a review from pitrou May 18, 2023 01:36

benibus and others added 17 commits May 22, 2023 15:18

Implement flattened methods for FieldRef

db2c0d3

Use internal helpers in FieldPath tests

971ce4e

Remove redundant FieldPath test

8b6aea4

Implement NestedSelector for FieldPathGetImpl

78ca0ba

Support passing MemoryPool to GetFlattenedField

918b083

Minor naming fix

0168b70

Apply comment suggestion

44bf768

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Remove FieldRef::Get indirections

015c2fd

Use if constexpr in FieldPathTestCase

08ce6bd

Explain some weirdness in AssertOutputsEqual

c9be785

Test/fix IndexError message

98d43ee

Add tests for FieldRef methods

fb1d694

Test cleanup

8ca8399

Return FieldPath::GetFlattened errors in FieldRef

759523c

Additional validation checks

e09516e

Relocate FieldPath/Ref tests to new module

163ee85

Add docstrings for new methods

a1662aa

pitrou force-pushed the GH-14946-field-ref-flattened-get branch from 17a6591 to a1662aa Compare May 22, 2023 13:33

pitrou approved these changes May 22, 2023

View reviewed changes

pitrou merged commit f3500f6 into apache:main May 22, 2023

benibus mentioned this pull request May 23, 2023

GH-33206: [C++] Add support for StructArray sorting and nested sort keys #35727

Merged

pitrou mentioned this pull request Jul 26, 2023

[C++] Potential regression on FieldRef/FieldPath non-flattening Get methods #36892

Closed

benibus mentioned this pull request Aug 6, 2023

GH-36892: [C++] Fix performance regressions in FieldPath::Get #37032

Merged

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

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

Uh oh!

Conversation

benibus commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Apr 17, 2023

Uh oh!

github-actions bot commented Apr 17, 2023

Uh oh!

benibus commented Apr 18, 2023

Uh oh!

benibus commented Apr 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felipecrv commented May 2, 2023

Uh oh!

benibus commented May 3, 2023

Uh oh!

benibus commented May 10, 2023

Uh oh!

pitrou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou May 17, 2023

Choose a reason for hiding this comment

Uh oh!

benibus May 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou May 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou May 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou May 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou May 17, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented May 22, 2023

Uh oh!

ursabot commented May 30, 2023

Uh oh!

ursabot commented May 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benibus commented Apr 17, 2023 •

edited

Loading

pitrou left a comment •

edited

Loading