GH-38865 [C++][Parquet] support passing a RowRange to RecordBatchReader by binmahone · Pull Request #38867 · apache/arrow

binmahone · 2023-11-23T13:22:36Z

!!! this PR is based on arrow-13 for early review

Rationale for this change

see #38865

What changes are included in this PR?

add a parameter in GetRecordBatchReader to accept row_ranges.
The row ranges can be used to:

skip decompressing and decoding unnecessary pages. This part is done by leveraging an existing hook called DataPageFilter
Now only necessary pages are remaining. In Parquet, row number is not aligned across different columns' pages, so the remaining rows for different columns are different. We introduced a new class called RecordSkipper to i. coordinate all Column Readers' progress so that they can still resemble row records correctly ii. skip unnecessary rows in necessary pages

Are these changes tested?

a new test file callled range_reader_test.cc is added

Are there any user-facing changes?

a new GetRecordBatchReader API overload is added. NO existing API is broken

Closes: [C++][Parquet] support passing a RowRange to RecordBatchReader #38865

github-actions · 2023-11-23T13:23:43Z

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

mapleFU

The RowRanges API LGTM. I think we can separate a patch for RowRanges

The underlying implement logic is ok, but I need to review a pass for it. Also, Seems that this patch is merged into 13.0.0...

mapleFU · 2023-11-23T13:44:32Z

cpp/src/parquet/arrow/reader.h


+  virtual ::arrow::Status GetRecordBatchReader(
+      const std::vector<int>& row_group_indices, const std::vector<int>& column_indices,
+      const std::shared_ptr<std::map<int, RowRangesPtr>>& row_ranges_map,


So, does RowRanges index means row-idx in this RowGroup or the row-idx in this file?

row-idx in this RowGroup

Please document this in the method contract. The way this is being passed (as a shared pointer and values being pointer seems a little strange to me) is there a reason how this contract was arrived at (I need to take a closer look at how Range was arrived at).

I'm not sure if keeping consistency for a brand new method is worth it or if this should return arrow::Result<std::unique_ptr> at some point we should convert all the methods to return result.

Its also not clear to me that we should include both RowGroupIndices and ranges within those indicies. It seems like a cleaner API to only include one or the other (which also makes passing in ranges easier?)

changed to File based

wgtmac · 2023-11-26T15:35:07Z

cpp/src/parquet/column_reader.h

+  RecordSkipper(RowRanges& pages, RowRanges& row_ranges_)
+      : row_ranges(row_ranges_) {  // copy row_ranges
+    RowRanges will_process_pages, skip_pages;
+    for (auto& page : pages.getRanges()) {


This requires records not to exceed page boundary. So we may need to check when RecordSkipper cannot be used.

When will this case happen?

wgtmac · 2023-11-26T15:41:50Z

cpp/src/parquet/arrow/reader.cc

+    }
+    auto offset_index = rg_pg_index_reader->GetOffsetIndex(input_->column_index());
+
+    if (!offset_index) {


Is this too strict? Sometimes not all columns have populated offset index.

it requires more code to proper handle the case in which page index is missing. Exception on missing seems to be a simple&easy solution for now

emkornfield · 2023-11-27T04:02:55Z

cpp/src/parquet/column_reader.h

                                          int32_t* dict_len) = 0;
 };

+struct Range {


Please document the struct.

emkornfield · 2023-11-27T04:05:27Z

cpp/src/parquet/column_reader.h

 };

+struct Range {
+  static Range unionRange(const Range& left, const Range& right) {


Generally, the Google style guide is used this means typically method names start with an upper-case character.

emkornfield · 2023-11-27T04:06:15Z

cpp/src/parquet/column_reader.h

 };

+struct Range {
+  static Range unionRange(const Range& left, const Range& right) {


please provide documentation on what this method is intended to do.

this method is deleted. and I have added some comments to critical classes & methods

emkornfield · 2023-11-27T04:08:58Z

cpp/src/parquet/column_reader.h


+struct Range {
+  static Range unionRange(const Range& left, const Range& right) {
+    if (left.from <= right.from) {


I think in general too much code is inlined in these classes.

Will do this before merging PR

emkornfield · 2023-11-27T04:13:36Z

cpp/src/parquet/column_reader.h

+  /// if return values is positive, it means to read N records
+  /// if return values is negative, it means to skip N records
+  /// if return values is 0, it means end of RG
+  int64_t advise_next(const int64_t current_rg_procesed) {


its not claear to me what the input parameter is here?

total rows that current RG has processed

emkornfield · 2023-11-27T04:16:11Z

cpp/src/parquet/column_reader.h

+};
+
+class RowRanges {
+  std::vector<Range> ranges;


please follow class layout conventions of putting private members at the end.

emkornfield · 2023-11-27T04:20:48Z

cpp/src/parquet/column_reader.h

+  }
+};
+
+class RowRanges {


This seems like a relatively heavy-weight represenation if most ranges aren't runs. did you consider other alternatives?

CC @fatemehp

added a bitmap interface in RowRanges. However it is not implemented yet.

emkornfield · 2023-11-27T04:22:17Z

cpp/src/parquet/column_reader.h

+    return result;
+  }
+
+  RowRanges slice(const int64_t from, const int64_t to) const {


please document intended semantics here. Is this method really necessary? why wouldn't it consider partial ranges?

this is deleted

emkornfield · 2023-11-27T04:23:25Z

cpp/src/parquet/column_reader.h


+class PARQUET_EXPORT RecordSkipper {
+ public:
+  RecordSkipper(RowRanges& pages, RowRanges& row_ranges_)


second variable looks off

can you elaborate on this? I don't quite get it.

cpp/src/parquet/column_reader.h

binmahone · 2024-01-01T16:14:53Z

I think it might be worth having more discussion on the issue with representation of RowRanges and on the newly exposed API on the issue to make sure we come to a consensus

The latest version follows the design in https://docs.google.com/document/d/1SeVcYudu6uD9rb9zRAnlLGgdauutaNZlAaS0gVzjkgM/edit

binmahone · 2024-01-01T16:17:53Z

hi @emkornfield , I've just pushed my refinements to this PR and will go through a review with @wgtmac . It may save you your time if you wait until @wgtmac has approved it

wgtmac

I just did another pass, mainly on the API and style. Please directly open a PR on the main branch. Thanks!

wgtmac · 2024-01-07T12:30:55Z

cpp/src/parquet/arrow/reader.h

                                       std::shared_ptr<::arrow::RecordBatchReader>* out);
  ::arrow::Status GetRecordBatchReader(std::shared_ptr<::arrow::RecordBatchReader>* out);

+


remove the blank line.

wgtmac · 2024-01-07T12:38:48Z

cpp/src/parquet/column_reader.h

+struct End {};
+
+// Represent a set of ranges to read. The ranges are sorted and non-overlapping.
+class RowRanges {


It would be good to relocate the RowRanges related classes to a separate source file.

wgtmac · 2024-01-07T12:41:48Z

cpp/src/parquet/column_reader.h

+  //         virtual std::unique_ptr<Iterator> NewIterator() = 0;
+
+ private:
+  std::vector<IntervalRange> ranges;


IMHO, RowRanges class should hide the implemetation detail and the private member std::vector<IntervalRange> ranges should not appear here. If we don't want to pay the cost for virtual class, we can use PIMPL idiom by following example in parquet/metadata.h

wgtmac · 2024-01-07T12:48:05Z

cpp/src/parquet/column_reader.h

 };

+// Represent a range to read. The range is inclusive on both ends.
+struct IntervalRange {


Should we simply define the IntervalRange class as below and move any functions to an individual utility class?

struct IntervalRange { // inclusive int64_t start; // inclusive int64_t end; }; struct IntervalRangeUtils { static IntervalRange Intersection(const IntervalRange& rhs, const IntervalRange& rhs); static bool IsBefore(const IntervalRange& lhs, const IntervalRange& rhs); ... }

IMO, the above separation would make the public API RowRanges looks much simpler to external user and those utility functions are what we need only in the implementation.

wgtmac · 2024-01-07T14:29:28Z

cpp/src/parquet/column_reader.h

+  size_t row_range_idx = 0;
+  size_t total_rows_to_process = 0;


Suggested change

size_t row_range_idx = 0;

size_t total_rows_to_process = 0;

size_t row_range_idx_ = 0;

size_t total_rows_to_process_ = 0;

This is the style convection for class member variables.

Got. The convention seems not enforced everywhere, for example: ReaderContext::reader

I'll change all member variables in my PR

wgtmac · 2024-01-07T14:51:24Z

cpp/src/parquet/arrow/reader.cc

+  bool operator()(const DataPageStats& stats) {
+    ++page_range_idx;
+
+    IntervalRange current_page_range = (*page_ranges)[page_range_idx];


Could we find some way to not directly assume all ranges are IntervalRange? Probably we can use the iterator of RowRanges and throw when a BitmapRange is obtained for now. Otherwise it would be difficult for future refactoring.

wgtmac · 2024-01-07T14:52:29Z

cpp/src/parquet/arrow/reader.cc

 // Column reader implementations

+struct RowRangesPageFilter {
+  explicit RowRangesPageFilter(const RowRanges& row_ranges_,


Please remove underscore from input parameters.

wgtmac · 2024-01-07T14:53:16Z

cpp/src/parquet/arrow/reader.cc

 // ----------------------------------------------------------------------
 // Column reader implementations

+struct RowRangesPageFilter {


Please add some comment to explain what it does.

wgtmac · 2024-01-07T14:54:15Z

cpp/src/parquet/arrow/reader.cc

+  void checkAndGetPageRanges(const RowRanges & row_ranges,
+                             std::shared_ptr<RowRanges>& page_ranges) const {


Suggested change

void checkAndGetPageRanges(const RowRanges & row_ranges,

std::shared_ptr<RowRanges>& page_ranges) const {

void CheckAndGetPageRanges(const RowRanges& row_ranges,

std::shared_ptr<RowRanges>* page_ranges) const {

why std::shared_ptr& -> std::shared_ptr* ?

wgtmac · 2024-01-07T15:01:22Z

cpp/src/parquet/arrow/reader.cc

+    if (page_locations.size() >= 1) {
+      page_ranges->Add(
+          {page_locations[page_locations.size() - 1].first_row_index,
+           ctx_->reader->metadata()->RowGroup(input_->current_row_group())->num_rows() -


This cost (RowGroup(input_->current_row_group())) may not be negligible if there are many columns to do this repeatedly.

mapleFU · 2024-01-08T10:40:17Z

cpp/src/parquet/column_reader.h

+    }
+  }
+
+  size_t Count() const { return end - start + 1; }


IntervalRange({-1, -1}).Count() == 1 ? Would this be weird?

added a check of validness

mapleFU · 2024-01-08T10:42:36Z

cpp/src/parquet/column_reader.h

+  RowRanges(const RowRanges& other) { ranges = other.ranges; }
+
+  RowRanges(RowRanges&& other) noexcept { ranges = std::move(other.ranges); }


mapleFU · 2024-01-08T10:47:42Z

cpp/src/parquet/column_reader.h

+    std::string result = "[";
+    for (const IntervalRange& range : ranges) {
+      result +=
+          "(" + std::to_string(range.start) + ", " + std::to_string(range.end) + "), ";


Uses range.ToString()?

thisisnic · 2025-11-18T12:32:14Z

Thank you for your contribution. Unfortunately, this
pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label
or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you
do not have repository permissions to reopen the PR, please tag a maintainer.

binmahone added 7 commits November 22, 2023 21:36

happy path pass

2ead017

happy path pass 2

3e9af38

happy path pass 3

a185d54

happy path pass 4

bdebb74

happy path pass 5

29c471a

happy path pass 6

7bf0e97

happy path pass 7

c97ea48

binmahone requested a review from wgtmac as a code owner November 23, 2023 13:22

github-actions bot added the awaiting review Awaiting review label Nov 23, 2023

happy path pass 8

f11d6a8

binmahone marked this pull request as draft November 23, 2023 13:32

github-actions bot added Component: Parquet Component: C++ labels Nov 23, 2023

mapleFU reviewed Nov 23, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 23, 2023

wgtmac reviewed Nov 26, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 27, 2023

emkornfield reviewed Nov 27, 2023

View reviewed changes

cpp/src/parquet/column_reader.h Outdated Show resolved Hide resolved

binmahone mentioned this pull request Nov 29, 2023

[CH][Parquet] support page pruning on first column apache/incubator-gluten#3884

Open

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 29, 2023

binmahone mentioned this pull request Nov 29, 2023

support rowrange interface ClickHouse/arrow#50

Draft

binmahone added 6 commits November 30, 2023 12:18

fix bug

dca69af

camel naming

2c8b068

simply map

945e543

api refined

ed9d02b

clean code

14974c0

clean code

ef8a7c8

wgtmac requested changes Jan 7, 2024

View reviewed changes

wgtmac mentioned this pull request Jan 7, 2024

GH-39392: [C++][Parquet] Support page pruning #39393

Open

mapleFU reviewed Jan 8, 2024

View reviewed changes

binmahone added 8 commits January 14, 2024 22:38

RowRangesPageFilter refactored

7b5d4a6

checkAndGetPageRanges refactored

25f83f8

RecordSkipper refactored

2e43866

refactor complete

cb0d67b

fix style

5805b97

separete definition

639d94a

separete definition 2

8f5a88a

separete definition 3

09286d7

binmahone mentioned this pull request Jan 15, 2024

GH-38865 [C++][Parquet] support passing a RowRange to RecordBatchReader #39608

Closed

minor

b75abdf

binmahone force-pushed the mhb_page_pruning branch from 1fd4bba to b75abdf Compare January 15, 2024 13:09

fix comments

e361c66

github-actions bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Nov 18, 2025

thisisnic closed this Dec 19, 2025

		std::shared_ptr<::arrow::RecordBatchReader>* out);
		::arrow::Status GetRecordBatchReader(std::shared_ptr<::arrow::RecordBatchReader>* out);

		void checkAndGetPageRanges(const RowRanges & row_ranges,
		std::shared_ptr<RowRanges>& page_ranges) const {

		RowRanges(const RowRanges& other) { ranges = other.ranges; }

		RowRanges(RowRanges&& other) noexcept { ranges = std::move(other.ranges); }

Comments

Conversation

binmahone commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

!!! this PR is based on arrow-13 for early review

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 Nov 23, 2023

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

binmahone commented Jan 1, 2024

Uh oh!

binmahone commented Jan 1, 2024

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

binmahone commented Nov 23, 2023 •

edited

Loading