GH-43797: [C++] Attach `arrow::ArrayStatistics` to `arrow::ArrayData` #43801

kou · 2024-08-23T06:47:18Z

Rationale for this change

If we can attach associated statistics to an array via ArrayData, we can use it in later processes such as query planning.

If ArrayData not Array has statistics, we can use statistics in computing kernels.

There was a concern that associated arrow::ArrayStatistics may be outdated if arrow::ArrayData is mutated after attaching arrow::ArrayStatistics. But arrow::ArrayData isn't mutable after the first population. So arrow::ArrayStatistics will not be outdated. We can require mutators to take responsibility for statistics.

What changes are included in this PR?

Add arrow::ArrayData::statistics
Add arrow::Array::statistics() to get statistics attached in arrow::ArrayData

This doesn't provide a new arrow::ArrayData constructor (arrow::ArrayData::Make()) that accepts arrow::ArrayStatistics. We can change arrow::ArrayData::statistics after we create arrow::ArrayData.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

arrow::Array::statistics() is a new public API.

GitHub Issue: [C++] Attach arrow::ArrayStatistics to arrow::ArrayData #43797

github-actions · 2024-08-23T06:47:47Z

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

kou · 2024-08-27T05:58:55Z

@pitrou @bkietz @felipecrv What do you think about this approach?
See also the discussion at #43705 (comment) and comments followed by it.

bkietz

I think making statistics a "lazy" member of ArrayData is the right approach. It should probably be wrapped in a pointer, though: this will ensure that new members can be added to ArrayStatistics without impacting the size of ArrayData

cpp/src/arrow/array/data.h

kou · 2024-08-28T00:46:53Z

OK. I've changed to a pointer instead of embedding ArrayStatistics to ArrayData.
But I used std::shared_ptr not std::unique_ptr because we want to export ArrayStatistics from arrow::Array. If we use std::unique_ptr, arrow::Array::statistics() also needs to return std::unique_ptr. It will be difficult to use. So I used std::shared_ptr.

mapleFU · 2024-08-29T11:20:19Z

What just raise my curiousity is that subslice for array should discard the statistics?

bkietz · 2024-08-29T20:24:55Z

Most statistics would be invalidated by slicing, such as the distinct and null counts. The minimum and maximum could be preserved, but would have to be demoted to inexact until recomputed.

…yData` If we can attach associated statistics to an array via `ArrayData`, we can use it in later processes such as query planning.

kou · 2024-08-30T04:55:31Z

Good catch! I forgot the Slice() case...
I've discarded statistics on Slice(). If an user wants to re-use (min/max) statistics of the original ArrayData, the user is responsible it.

I noticed that ArrayData::CopyTo() needs to copy statistics. I've done it too.

mapleFU

General LGTM but I'm not familiar with details in this

cpp/src/arrow/array/data.cc

kou · 2024-08-31T10:09:33Z

If nobody objects this, I'll merge this in the next week.

mapleFU

LGTM

…yData` (apache#43801) ### Rationale for this change If we can attach associated statistics to an array via `ArrayData`, we can use it in later processes such as query planning. If `ArrayData` not `Array` has statistics, we can use statistics in computing kernels. There was a concern that associated `arrow::ArrayStatistics` may be outdated if `arrow::ArrayData` is mutated after attaching `arrow::ArrayStatistics`. But `arrow::ArrayData` isn't mutable after the first population. So `arrow::ArrayStatistics` will not be outdated. We can require mutators to take responsibility for statistics. ### What changes are included in this PR? * Add `arrow::ArrayData::statistics` * Add `arrow::Array::statistics()` to get statistics attached in `arrow::ArrayData` This doesn't provide a new `arrow::ArrayData` constructor (`arrow::ArrayData::Make()`) that accepts `arrow::ArrayStatistics`. We can change `arrow::ArrayData::statistics` after we create `arrow::ArrayData`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. `arrow::Array::statistics()` is a new public API. * GitHub Issue: apache#43797 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

conbench-apache-arrow · 2024-09-03T17:09:42Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4ed5a14.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 29 possible false positives for unstable benchmarks that are known to sometimes produce them.

…yData` (apache#43801) ### Rationale for this change If we can attach associated statistics to an array via `ArrayData`, we can use it in later processes such as query planning. If `ArrayData` not `Array` has statistics, we can use statistics in computing kernels. There was a concern that associated `arrow::ArrayStatistics` may be outdated if `arrow::ArrayData` is mutated after attaching `arrow::ArrayStatistics`. But `arrow::ArrayData` isn't mutable after the first population. So `arrow::ArrayStatistics` will not be outdated. We can require mutators to take responsibility for statistics. ### What changes are included in this PR? * Add `arrow::ArrayData::statistics` * Add `arrow::Array::statistics()` to get statistics attached in `arrow::ArrayData` This doesn't provide a new `arrow::ArrayData` constructor (`arrow::ArrayData::Make()`) that accepts `arrow::ArrayStatistics`. We can change `arrow::ArrayData::statistics` after we create `arrow::ArrayData`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. `arrow::Array::statistics()` is a new public API. * GitHub Issue: apache#43797 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

felipecrv · 2024-10-30T23:06:11Z

cpp/src/arrow/array/array_base.h

+  /// object which backs this Array.
+  ///
+  /// \return const ArrayStatistics&
+  std::shared_ptr<ArrayStatistics> statistics() const { return data_->statistics; }


The return type should be std::shared_ptr<ArrayStatistics>& and we should probably add const ArrayStatistics to the shared_ptr so that callers can't mutate the statistics through the shared pointer.

I agree with you. I don't know why I missed const and & here...

Let's add them: GH-44590

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Aug 23, 2024

kou mentioned this pull request Aug 23, 2024

GH-43666: [C++] Attach arrow::ArrayStatistics to arrow::Array #43705

Closed

bkietz requested changes Aug 27, 2024

View reviewed changes

cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 27, 2024

kou force-pushed the cpp-array-statistics-attach-datum branch from 8ee9b69 to 4d5b234 Compare August 28, 2024 00:39

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 28, 2024

kou added 4 commits August 30, 2024 13:37

apacheGH-43797: [C++] Attach arrow::ArrayStatistics to `arrow::Arra…

0002140

…yData` If we can attach associated statistics to an array via `ArrayData`, we can use it in later processes such as query planning.

Use pointer instead of embedding

97c2c7d

Clear statistics on slice

ea16740

Add support for CopyTo

942f757

kou force-pushed the cpp-array-statistics-attach-datum branch from 4d5b234 to 942f757 Compare August 30, 2024 04:37

mapleFU reviewed Aug 30, 2024

View reviewed changes

cpp/src/arrow/array/data.cc Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 31, 2024

Add a comment for Slice() and statistics

72f3f2a

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 31, 2024

mapleFU reviewed Sep 1, 2024

View reviewed changes

kou merged commit 4ed5a14 into apache:main Sep 3, 2024
37 of 40 checks passed

kou deleted the cpp-array-statistics-attach-datum branch September 3, 2024 01:13

kou removed the awaiting changes Awaiting changes label Sep 3, 2024

kou mentioned this pull request Sep 3, 2024

[C++] Attach arrow::ArrayStatistics to arrow::ArrayData #43797

Closed

felipecrv reviewed Oct 30, 2024

View reviewed changes

github-actions bot added the awaiting changes Awaiting changes label Oct 30, 2024

kou mentioned this pull request Oct 31, 2024

[C++] Add const and & to arrow::Array::statistics() return type #44590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43797: [C++] Attach `arrow::ArrayStatistics` to `arrow::ArrayData` #43801

GH-43797: [C++] Attach `arrow::ArrayStatistics` to `arrow::ArrayData` #43801

kou commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024

kou commented Aug 27, 2024

bkietz left a comment

kou commented Aug 28, 2024

mapleFU commented Aug 29, 2024

bkietz commented Aug 29, 2024

kou commented Aug 30, 2024

mapleFU left a comment

kou commented Aug 31, 2024

mapleFU left a comment

conbench-apache-arrow bot commented Sep 3, 2024

felipecrv Oct 30, 2024

felipecrv Oct 30, 2024

kou Oct 31, 2024

GH-43797: [C++] Attach arrow::ArrayStatistics to arrow::ArrayData #43801

GH-43797: [C++] Attach arrow::ArrayStatistics to arrow::ArrayData #43801

Conversation

kou commented Aug 23, 2024 • edited Loading

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 Aug 23, 2024

kou commented Aug 27, 2024

bkietz left a comment

Choose a reason for hiding this comment

kou commented Aug 28, 2024

mapleFU commented Aug 29, 2024

bkietz commented Aug 29, 2024

kou commented Aug 30, 2024

mapleFU left a comment

Choose a reason for hiding this comment

kou commented Aug 31, 2024

mapleFU left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Sep 3, 2024

felipecrv Oct 30, 2024

Choose a reason for hiding this comment

felipecrv Oct 30, 2024

Choose a reason for hiding this comment

kou Oct 31, 2024

Choose a reason for hiding this comment

GH-43797: [C++] Attach `arrow::ArrayStatistics` to `arrow::ArrayData` #43801

GH-43797: [C++] Attach `arrow::ArrayStatistics` to `arrow::ArrayData` #43801

kou commented Aug 23, 2024 •

edited

Loading