Skip to content
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

GH-15185: [C++][Parquet] Improve documentation for Parquet Reader column_indices #15184

Merged
merged 7 commits into from
Jan 6, 2023

Conversation

LouisClt
Copy link
Contributor

@LouisClt LouisClt commented Jan 4, 2023

This aims to fix the documentation and improve it,, by better specifying to what level the column_indices given in argument refer to.

@github-actions
Copy link

github-actions bot commented Jan 4, 2023

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 old issues on JIRA the title also supports:

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

See also:

@LouisClt LouisClt changed the title [C++][Parquet] Improve documentation for Parquet Reader column_indices GH-15185: [C++][Parquet] Improve documentation for Parquet Reader column_indices Jan 4, 2023
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

@github-actions
Copy link

github-actions bot commented Jan 4, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 4, 2023

⚠️ GitHub issue #15185 has no components, please add labels for components.

Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: I think this doc comment is misleading as well:

/// \brief The number of top-level columns in the schema.

It returns the number of leaf fields in the schema, not the number of top-level ones (at least as most people would understand). Would you mind updating that as well?

cpp/src/parquet/arrow/reader.h Outdated Show resolved Hide resolved
cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved
Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks @LouisClt!

@wjones127 wjones127 merged commit a580f27 into apache:master Jan 6, 2023
@ursabot
Copy link

ursabot commented Jan 6, 2023

Benchmark runs are scheduled for baseline = 6bd847b and contender = a580f27. a580f27 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.97% ⬆️0.39%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.17% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] a580f271 ec2-t3-xlarge-us-east-2
[Finished] a580f271 test-mac-arm
[Finished] a580f271 ursa-i9-9960x
[Finished] a580f271 ursa-thinkcentre-m75q
[Finished] 6bd847b2 ec2-t3-xlarge-us-east-2
[Finished] 6bd847b2 test-mac-arm
[Finished] 6bd847b2 ursa-i9-9960x
[Finished] 6bd847b2 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Jan 6, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request Jan 9, 2023
…er column_indices (apache#15184)

This aims to fix the documentation and improve it,, by better specifying to what level the column_indices given in argument refer to.
* Closes: apache#15185

Lead-authored-by: LouisClt <louis1110@hotmail.fr>
Co-authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
EpsilonPrime pushed a commit to EpsilonPrime/arrow that referenced this pull request Jan 10, 2023
…er column_indices (apache#15184)

This aims to fix the documentation and improve it,, by better specifying to what level the column_indices given in argument refer to.
* Closes: apache#15185

Lead-authored-by: LouisClt <louis1110@hotmail.fr>
Co-authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement for the documentation of Parquet reader
4 participants