GH-37002: [C++][Parquet] Add api to get RecordReader from RowGroupReader by fatemehp · Pull Request #37003 · apache/arrow

fatemehp · 2023-08-03T00:10:14Z

Currently we only can get a ColumnReader for a column from RowGroupReader. We need an API to return a RecordReader for the column.

Moved ComputeLevelInfo from column_writer to level_conversion so that it can be shared between column_writer and file_reader.

Closes: [C++][Parquet] Add api to get RecordReader from RowGroupReader. #37002

github-actions · 2023-08-03T00:10:44Z

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}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

mapleFU

I'm generally ok with this patch. Would you mind add GH-37002 as a prefix?

mapleFU · 2023-08-03T02:40:18Z


+  // Construct a RecordReader for the indicated row group-relative column i.
+  // Ownership is shared with the RowGroupReader.
+  std::shared_ptr<internal::RecordReader> RecordReader(


Emm I'm ok with the logic here, but is it ok to expose internal:: logic?

I doubt tests checking exposure of internal namespace will complain about this.

We probably should remove the RecordReader from the internal namespace at some point. I am not sure how that is decided.

I'm fine with moving it out of the internal namespace.

Sounds good. I think it is better to remove the internal namespace in a separate pull request.

Hi @fatemehp . Should we first remove internal, then work on this patch? Or you'd like to let us check this first?

After checking I think maybe we can remove internal. cc @pitrou @wgtmac

I would prefer to check this in first.

IMHO, I'd prefer to move RecordReader out of internal namespace first. Because we can know the consequence of namespace change before exposing it.

github-actions · 2023-08-03T05:34:12Z

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

wgtmac · 2023-08-10T05:14:16Z


+  // Construct a RecordReader for the indicated row group-relative column i.
+  // Ownership is shared with the RowGroupReader.
+  std::shared_ptr<internal::RecordReader> RecordReader(


IMHO, I'd prefer to move RecordReader out of internal namespace first. Because we can know the consequence of namespace change before exposing it.

mapleFU · 2023-08-23T17:12:53Z

I've check the codebase and find somewhere export the internal in .h, so here look good to me if other issue fixed @fatemehp

mapleFU · 2023-09-12T07:58:10Z

ping @fatemehp , would we move forward?

fatemehp · 2023-10-03T19:44:58Z

I am sorry I got distracted for a while. I will make the changes requested and update here. @mapleFU are you suggesting that it is not necessary to remove the internal namespace?

the underlying record reader.

fatemehp · 2023-10-05T16:29:25Z

@wgtmac , @mapleFU are we ready to merge this?

mapleFU

Personally LGTM. We're at national holiday so might reply later.

Also cc @pitrou for review

fatemehp · 2023-10-09T21:24:09Z

@wgtmac I tried to address the comments.

wgtmac

Thanks! LGTM.

The CI failures should be unrelated. I have retriggered failed ones.

cc @pitrou @emkornfield

fatemehp · 2023-10-11T22:27:51Z

The error seems unrelated. @wgtmac, @mapleFU could you merge this?

wgtmac · 2023-10-12T01:50:44Z

The error seems unrelated. @wgtmac, @mapleFU could you merge this?

Let's wait a while for opinions from @pitrou @emkornfield

pitrou · 2023-10-12T16:45:31Z

@wgtmac I'll let you handle this if you don't mind.

wgtmac · 2023-10-13T01:14:06Z

No problem! @pitrou

emkornfield · 2023-10-16T19:33:45Z

I think if we are going to be exposing this publicly we should remove the internal namespace from it. This can likely be done as a follow-up. Otherwise seems good to me.

wgtmac · 2023-10-17T01:16:04Z

I think if we are going to be exposing this publicly we should remove the internal namespace from it. This can likely be done as a follow-up. Otherwise seems good to me.

Thanks @emkornfield! Then I will merge it as is.

Please remove the internal namespace in a follow-up change if you have time. Thanks! @fatemehp

conbench-apache-arrow · 2023-10-19T19:26:56Z

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

There were no benchmark performance regressions. 🎉

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

…oupReader (apache#37003) Currently we only can get a ColumnReader for a column from RowGroupReader. We need an API to return a RecordReader for the column. Moved ComputeLevelInfo from column_writer to level_conversion so that it can be shared between column_writer and file_reader. * Closes: apache#37002 Lead-authored-by: Fatemah Panahi <[email protected]> Co-authored-by: Fatemah Panahi <[email protected]> Signed-off-by: Gang Wu <[email protected]>

fatemehp and others added 4 commits August 2, 2023 23:53

Add api to get a RecordReader for a column from a RowGroupReader.

7190cf5

Merge branch 'apache:main' into master

78d3c00

Minor edit.

1b726c3

Merge branch 'master' of github.com:fatemehp/arrow

0fcb449

fatemehp requested a review from wgtmac as a code owner August 3, 2023 00:10

github-actions Bot added Component: Parquet Component: C++ awaiting review labels Aug 3, 2023

mapleFU reviewed Aug 3, 2023

View reviewed changes

github-actions Bot added awaiting committer review and removed awaiting review labels Aug 3, 2023

wgtmac changed the title ~~[C++][Parquet] Add api to get RecordReader from RowGroupReader. #37002~~ GH-37002: [C++][Parquet] Add api to get RecordReader from RowGroupReader Aug 3, 2023

fatemehp added 2 commits August 3, 2023 16:41

Fix lint errors

7d6e8db

Remove const_cast

2d073bf

wgtmac requested changes Aug 10, 2023

View reviewed changes

Merge branch 'apache:main' into master

64f9d3b

Add read_dense_for_nullable to ReaderProperties.

c315dab

mapleFU reviewed Oct 4, 2023

View reviewed changes

Comment thread cpp/src/parquet/file_reader.h Resolved

Comment thread cpp/src/parquet/reader_test.cc Resolved

Add a test that checks that read_dense_for_nullable is passed down to

bcf29bb

the underlying record reader.

mapleFU approved these changes Oct 5, 2023

View reviewed changes

wgtmac requested changes Oct 9, 2023

View reviewed changes

Comment thread cpp/src/parquet/reader_test.cc Resolved

Comment thread cpp/src/parquet/file_reader.h Resolved

Address reviewer comments.

7b4f896

wgtmac approved these changes Oct 10, 2023

View reviewed changes

wgtmac merged commit d3a576d into apache:main Oct 17, 2023

wgtmac removed the awaiting committer review label Oct 17, 2023

fatemehp mentioned this pull request Dec 1, 2023

[C++][Parquet] Using BMI to implement filter pushdown #37559

Open

Conversation

fatemehp commented Aug 3, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 3, 2023

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

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!

github-actions Bot commented Aug 3, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Aug 23, 2023

Uh oh!

mapleFU commented Sep 12, 2023

Uh oh!

fatemehp commented Oct 3, 2023

Uh oh!

Uh oh!

Uh oh!

fatemehp commented Oct 5, 2023

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fatemehp commented Oct 9, 2023

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

fatemehp commented Oct 11, 2023

Uh oh!

wgtmac commented Oct 12, 2023

Uh oh!

pitrou commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wgtmac commented Oct 13, 2023

Uh oh!

emkornfield commented Oct 16, 2023

Uh oh!

wgtmac commented Oct 17, 2023

Uh oh!

conbench-apache-arrow Bot commented Oct 19, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fatemehp commented Aug 3, 2023 •

edited by github-actions Bot

Loading

pitrou commented Oct 12, 2023 •

edited

Loading