WIP: [C++][Parquet] Add RowRanges API #45234

wgtmac · 2025-01-13T03:05:05Z

Rationale for this change

This is part of the effort to support efficient filtering to the Parquet reader. The overall design has been discussed at https://docs.google.com/document/d/1SeVcYudu6uD9rb9zRAnlLGgdauutaNZlAaS0gVzjkgM

What changes are included in this PR?

Add a new RowRanges API and support basic operations on it.

Are these changes tested?

TODO

Are there any user-facing changes?

No.

github-actions · 2025-01-13T03:05:32Z

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}

See also:

wgtmac · 2025-01-17T15:10:07Z

@emkornfield @mapleFU @pitrou Could you help check the API (which has been discussed in the Google doc from the PR description)? I will add the BitmapRange support as well as test cases if the interfaces are looking good. Thanks!

mapleFU · 2025-02-06T09:56:27Z

cpp/src/parquet/row_range.h

+namespace parquet {
+
+/// RowRanges is a collection of non-overlapping and ascendingly ordered row ranges.
+class PARQUET_EXPORT RowRanges {


should we mark internal or experimental first? I think this might change fastly

mapleFU · 2025-02-06T09:57:02Z

cpp/src/parquet/row_range.h

+  class Iterator {
+   public:
+    virtual ~Iterator() = default;
+    virtual std::variant<IntervalRange, BitmapRange, End> NextRange() = 0;


Would this virtual function being slow?

I agree that doesn't look terrific. Perhaps something like:

/// Return a batch of ranges. The returned batch has size 0 iff the iterator is exhausted. virtual util::span<std::variant<IntervalRange, BitmapRange>> NextRanges() = 0;

mapleFU · 2025-02-06T10:00:52Z

cpp/src/parquet/row_range.cc

+  }
+
+ private:
+  decltype(RowRanges::ranges_.cbegin()) iter_;


should we using IterT = decltype(RowRanges::ranges_.cbegin()); here?

pitrou

I haven't looked at the implementation, just commenting on the API.

pitrou · 2025-02-06T10:16:11Z

cpp/src/parquet/row_range.h

+  struct IntervalRange {
+    /// Start row of the range (inclusive).
+    int64_t start;
+    /// End row of the range (inclusive).


I would suggest either a length or a exclusive end. Inclusive is more confusing and error-prone IMHO.

We commonly use the (start, length) convention in other places.

(start, length) is good for API but not good for computation because we have to transform it back and forth to (start, start + length).

pitrou · 2025-02-06T10:18:10Z

cpp/src/parquet/row_range.h

+  struct End {};
+
+  /// \brief An iterator for accessing row ranges in order.
+  class Iterator {


We have iteration facilities in arrow/util/iterator.h, should they be reused instead of defining our own here?

pitrou · 2025-02-06T10:19:06Z

cpp/src/parquet/row_range.h

+
+#pragma once
+
+#include <variant>


Also need to include <cstdint>

pitrou · 2025-02-06T10:19:49Z

cpp/src/parquet/row_range.h

+namespace parquet {
+
+/// RowRanges is a collection of non-overlapping and ascendingly ordered row ranges.
+class PARQUET_EXPORT RowRanges {


Plural class names are always a bit weird, should we perhaps name it RowSelection?

pitrou · 2025-02-06T10:20:19Z

cpp/src/parquet/row_range.h

+  static RowRanges MakeSingle(int64_t start, int64_t end);
+
+  /// \brief Make a row range from a list of intervals.
+  static RowRanges MakeIntervals(const std::vector<IntervalRange>& intervals);


Could take a span<const IntervalRange> instead.

Also, perhaps name this FromIntervals.

pitrou · 2025-02-06T10:21:50Z

cpp/src/parquet/row_range.h

+  /// \brief Make a single row range of [0, row_count - 1].
+  static RowRanges MakeSingle(int64_t row_count);
+
+  /// \brief Make a single row range of [start, end].
+  static RowRanges MakeSingle(int64_t start, int64_t end);


This is basically a simpler version of the IntervalRange-taking constructor?

pitrou · 2025-02-06T10:23:06Z

cpp/src/parquet/row_range.h

+  };
+
+  /// \brief A range of contiguous rows represented by a bitmap.
+  struct BitmapRange {


Is this actually convenient to use? I see it's not supported yet in the implementation...

There are other possible representations, for example:

/// \brief A range of 8 rows represented by a start offset and small indices struct IndexedRange { /// Start row of the range int64_t offset; /// Indices from `offset` that point to selected rows, in ascending order. std::array<uint8_t, 8> indices; };

This is proposed by @emkornfield from the discussion https://docs.google.com/document/d/1SeVcYudu6uD9rb9zRAnlLGgdauutaNZlAaS0gVzjkgM

Okay, two comments:

this shows that a Google Doc is really not the right medium to discuss API changes

I would ask that any API comes with an actual implementation; if it's too complicated to make use of the proposed BitmapRange efficiently, then it shows that BitmapRange is not the right abstraction

I'm also a little bit skeptical about the effectiveness of mixing BitmapRange and IntervalRange. According to my experience, I need either a list of IntervalRange (like the RowRanges API in parquet-java) or simply a roaring bitmap, but not mixed. The former is used for intermediate result of filter pushdown (e.g. compute selected row ranges of a row group when evaluating different filters) and the latter is for row-level selection/deselection (e.g. apply iceberg position delete file to a base file).

The former is used for intermediate result of filter pushdown (e.g. compute selected row ranges of a row group when evaluating different filters) and the latter is for row-level selection/deselection (e.g. apply iceberg position delete file to a base file).

Ok, so several questions follow:

at which use case is this API targeted?

what is the typical bitmap length this is meant to represent?

do we want a structure to select across an entire row group, or is it better to have one selection per data page, or perhaps a two-level selection (with a page bitmap and then a per-page bitmap for those pages that haven't been pruned)?

For example, given the typical size of a page, an uncompressed bitmap might be fine if selecting inside a page.

at which use case is this API targeted?

a. Specify the required row ranges when creating a Parquet reader (be it a ParquetFileReader or RowGroupReader). This applies to both interval and bitmap forms depending on the use case. Perhaps this is the main reason to use a mixed form.
b. Used internally when computing matched row ranges from filter pushdown evaluation result with the help of page index. This applies to the interval range because operations are usually based on the page level.
c. Advanced row-level filtering discussed in #37559. This should be bitmap range used by ColumnReader and Decoder.

what is the typical bitmap length this is meant to represent?

It depends on the use case above and can be any of num_rows_in_file, num_rows_in_a_row_group and batch_size.

do we want a structure to select across an entire row group, or is it better to have one selection per data page, or perhaps a two-level selection (with a page bitmap and then a per-page bitmap for those pages that haven't been pruned)?

I don't think we need an intra-page selection since page boundaries are usually not aligned across different columns. I think file-level selection is easy to use for users and row-group-level selection is required by the RowGroupReader implementation.

pitrou · 2025-02-06T10:34:28Z

cpp/src/parquet/row_range.h

+  class Iterator {
+   public:
+    virtual ~Iterator() = default;
+    virtual std::variant<IntervalRange, BitmapRange, End> NextRange() = 0;


I agree that doesn't look terrific. Perhaps something like:

/// Return a batch of ranges. The returned batch has size 0 iff the iterator is exhausted. virtual util::span<std::variant<IntervalRange, BitmapRange>> NextRanges() = 0;

mapleFU · 2025-02-07T03:30:11Z

cpp/src/parquet/row_range.cc

+
+  ~IteratorImpl() override = default;
+
+  std::variant<RowRanges::IntervalRange, RowRanges::BitmapRange, RowRanges::End>


I'm thinking that would variant being slow in storage? What would an internal:

enum class Type { IntervalRange, BitmapRange }; struct Range { int64_t begin; /// for Interval this means range /// for bitmap this is a 64-bits map int64_t payload; }; std::vector<Type> types_; std::vector<Range> ranges_;

I think this is more space efficient than current storage api...

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Jan 13, 2025

wgtmac mentioned this pull request Jan 16, 2025

[Parquet][C++] PageIndex is useless with current API #45284

Open

WIP: [C++][Parquet] Add RowRanges API

ae15c3f

wgtmac force-pushed the row_range branch from 6ebf279 to ae15c3f Compare January 17, 2025 15:05

wgtmac marked this pull request as ready for review January 17, 2025 15:05

wgtmac marked this pull request as draft January 17, 2025 15:06

wgtmac marked this pull request as ready for review January 21, 2025 06:24

mapleFU reviewed Feb 6, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 6, 2025

pitrou reviewed Feb 6, 2025

View reviewed changes

mapleFU reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [C++][Parquet] Add RowRanges API #45234

WIP: [C++][Parquet] Add RowRanges API #45234

wgtmac commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025

wgtmac commented Jan 17, 2025

mapleFU Feb 6, 2025

mapleFU Feb 6, 2025

pitrou Feb 6, 2025

mapleFU Feb 6, 2025

pitrou left a comment

pitrou Feb 6, 2025

pitrou Feb 6, 2025

wgtmac Feb 8, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

pitrou Feb 6, 2025

wgtmac Feb 7, 2025

pitrou Feb 7, 2025

wgtmac Feb 7, 2025 •

edited

Loading

pitrou Feb 7, 2025

wgtmac Feb 8, 2025

pitrou Feb 6, 2025

mapleFU Feb 7, 2025


		~IteratorImpl() override = default;

		std::variant<RowRanges::IntervalRange, RowRanges::BitmapRange, RowRanges::End>


		#pragma once

		#include <variant>

WIP: [C++][Parquet] Add RowRanges API #45234

Are you sure you want to change the base?

WIP: [C++][Parquet] Add RowRanges API #45234

Conversation

wgtmac commented Jan 13, 2025 • 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 Jan 13, 2025

wgtmac commented Jan 17, 2025

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

pitrou 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

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

wgtmac Feb 7, 2025 • edited Loading

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

wgtmac commented Jan 13, 2025 •

edited

Loading

wgtmac Feb 7, 2025 •

edited

Loading