-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-45284: [Parquet][C++] Proposed RowRanges API #48635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
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? or See also: |
|
@emkornfield any thoughts on this as a proposed first step? |
|
|
cpp/src/parquet/row_selection.h
Outdated
| /// \brief EXPERIMENTAL: Validate the row ranges. | ||
| /// \throws ParquetException if the row ranges are not in ascending order or | ||
| /// overlapped. | ||
| void Validate() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like validation should happen on construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That probably makes sense. I've moved it so that it gets called before any of the functions the create RowSelections return.
|
Took a look at the top-level public API, looks reasonable to me. If you need a full review please ping me. Apologies for the delay. |
Co-authored-by: emkornfield <[email protected]>
…tions that create a RowSelection.
No worries for the delay, January is just a difficult month. I would appreciate a full review if you have the time for it. |
Rationale for this change
Based on @wgtmac 's #45234 This attempts to address the comments in that PR
What changes are included in this PR?
add an experimental row_selection API
Are these changes tested?
Yes
Are there any user-facing changes?
Yes