GH-44010: [C++] Add `arrow::RecordBatch::MakeStatisticsArray()` #44252

kou · 2024-09-30T08:08:18Z

Rationale for this change

Statistics schema for Arrow C data interface (GH-43553) is complex because it uses nested types (struct, map and union). So reusable implementation to make statistics array is useful.

What changes are included in this PR?

arrow::RecordBatch::MakeStatisticsArray() is a convenient function that converts arrow::ArrayStatistics in a arrow::RecordBatch to arrow::Array for the Arrow C data interface.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

GitHub Issue: [C++] Add a convenient function that converts arrow::ArrayStatistics to arrow::Array for the Arrow C data interface #44010

github-actions · 2024-09-30T08:08:51Z

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

cpp/src/arrow/compare.cc

cpp/src/arrow/record_batch.cc

kou · 2024-10-02T05:48:21Z

@pitrou @ianmcook What do you think about this?

Statistics schema https://github.com/apache/arrow/pull/43553/files#diff-f3758fb6986ea8d24bb2e13c2feb625b68bbd6b93b3fbafd3e2a03dcdc7ba263R86-R95 is compact but it may be complex to build. Because it uses many nested types.

mapleFU · 2024-10-30T06:40:17Z

cpp/src/arrow/array/statistics.h

+      const std::shared_ptr<DataType>& operator()(const int64_t&) { return int64(); }
+      const std::shared_ptr<DataType>& operator()(const uint64_t&) { return uint64(); }
+      const std::shared_ptr<DataType>& operator()(const double&) { return float64(); }
+      const std::shared_ptr<DataType>& operator()(const std::string&) { return utf8(); }


I may forgot a bit but we don't distinct "bytes" and "utf8" in stats?

Ah, we didn't discuss it...
Let's discuss it in #44579.

We can assume "utf8" here for now.

We can add a // TODO(GH-44579) here?

Yes. I should have added it...
I've added it.

mapleFU · 2024-10-30T06:41:37Z

cpp/src/arrow/c/abi.h

@@ -80,6 +80,24 @@ struct ArrowArray {
  void* private_data;
 };

+#  define ARROW_STATISTICS_KEY_AVERAGE_BYTE_WIDTH_EXACT "ARROW:average_byte_width:exact"


Don't know constexpr std::string_view is better or this is better

We can't use constexpr because this header may be used by C programs.

mapleFU · 2024-10-30T06:45:05Z

cpp/src/arrow/record_batch.cc

+  statistics.nth_statistics = 0;
+  statistics.start_new_column = true;
+  statistics.nth_column = std::nullopt;
+  statistics.key = ARROW_STATISTICS_KEY_ROW_COUNT_EXACT;


So RowCount is also handled as a stats 🤔?

Statistics array will be passed to consumer before consumer receives a record batch.
So this may be useful for consumer.

But DuckDB doesn't have row count in its BaseStatistics...: https://github.com/duckdb/duckdb/blob/670cd341249e266de384e0341f200f4864b41b27/src/include/duckdb/storage/statistics/base_statistics.hpp#L38-L146
This may not be useful...

Let's keep this for now to demonstrate table/record batch level statistics.

cpp/src/arrow/record_batch.cc

It's a convenient function that converts `arrow::ArrayStatistics` in a `arrow::RecordBatch` to `arrow::Array` for the Arrow C data interface.

mapleFU

Will take a careful pass tonight

mapleFU · 2024-11-04T06:13:18Z

cpp/src/arrow/array/statistics.h

+      const std::shared_ptr<DataType>& operator()(const int64_t&) { return int64(); }
+      const std::shared_ptr<DataType>& operator()(const uint64_t&) { return uint64(); }
+      const std::shared_ptr<DataType>& operator()(const double&) { return float64(); }
+      const std::shared_ptr<DataType>& operator()(const std::string&) { return utf8(); }


We can add a // TODO(GH-44579) here?

mapleFU

General LGTM but I'm not an expert on C ABI and data layer

mapleFU · 2024-11-04T14:50:33Z

cpp/src/arrow/record_batch.cc

+
+  // Statistics schema doesn't define static dense union type for
+  // values. Each statistics schema have a dense union type that has
+  // needled value types. The following block collects these types.


So actually this is logically a "set" prepared for items?

Right.

If there are the same types, the first type is only used.

mapleFU · 2024-11-04T14:55:56Z

cpp/src/arrow/record_batch.cc

+};
+using OnStatistics =
+    std::function<Status(const EnumeratedStatistics& enumerated_statistics)>;
+Status EnumerateStatistics(const RecordBatch& record_batch, OnStatistics on_statistics) {


So actually this is for a two-phase building, one pass for types, and one-pass for data?

Right.

I think that it's one of complexities.
So I sent https://lists.apache.org/thread/0c9jftkspvj7yw1lpo73s3vtp6vfjqv8 to the mailing list. But nobody agreed it. So this complexity will be acceptable...

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

kou commented Sep 30, 2024

View reviewed changes

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

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

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 30, 2024

kou force-pushed the cpp-record-batch-make-statistics-array branch from 903e3f4 to 92afc83 Compare September 30, 2024 08:17

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 30, 2024

kou force-pushed the cpp-record-batch-make-statistics-array branch from 92afc83 to b194430 Compare September 30, 2024 09:03

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Sep 30, 2024

kou force-pushed the cpp-record-batch-make-statistics-array branch from 5a00c48 to 12b1a97 Compare October 30, 2024 06:32

mapleFU reviewed Oct 30, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 30, 2024

kou added 6 commits November 1, 2024 08:48

apacheGH-44010: [C++] Add RecordBatch::MakeStatisticsArray()

089f0a3

It's a convenient function that converts `arrow::ArrayStatistics` in a `arrow::RecordBatch` to `arrow::Array` for the Arrow C data interface.

Add static_cast

031024d

Add explicit cast

4a21c30

Extract as a function

69f00f0

Add comments

cc41a0f

Set start_new_column = false after yielding a table level statistics

9c529d1

kou force-pushed the cpp-record-batch-make-statistics-array branch from 8e4d618 to 9c529d1 Compare October 31, 2024 23:48

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

mapleFU reviewed Nov 4, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44010: [C++] Add `arrow::RecordBatch::MakeStatisticsArray()` #44252

GH-44010: [C++] Add `arrow::RecordBatch::MakeStatisticsArray()` #44252

kou commented Sep 30, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Sep 30, 2024

kou commented Oct 2, 2024

mapleFU Oct 30, 2024

kou Oct 30, 2024

mapleFU Nov 4, 2024

kou Nov 4, 2024

mapleFU Oct 30, 2024

kou Oct 30, 2024

mapleFU Oct 30, 2024

kou Oct 30, 2024

kou Oct 30, 2024

mapleFU left a comment

mapleFU Nov 4, 2024

mapleFU left a comment

mapleFU Nov 4, 2024

kou Nov 4, 2024

mapleFU Nov 4, 2024

kou Nov 4, 2024

GH-44010: [C++] Add arrow::RecordBatch::MakeStatisticsArray() #44252

Are you sure you want to change the base?

GH-44010: [C++] Add arrow::RecordBatch::MakeStatisticsArray() #44252

Conversation

kou commented Sep 30, 2024 • edited by github-actions bot 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 Sep 30, 2024

kou commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GH-44010: [C++] Add `arrow::RecordBatch::MakeStatisticsArray()` #44252

GH-44010: [C++] Add `arrow::RecordBatch::MakeStatisticsArray()` #44252

kou commented Sep 30, 2024 •

edited by github-actions bot

Loading