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.

Sign up for GitHub

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

Jump to bottom

GH-43393: [C++][Parquet] parquet-dump-footer: Remove redundant link and fix --debug processing #43375

Merged
merged 2 commits into from
Jul 23, 2024
Merged

GH-43393: [C++][Parquet] parquet-dump-footer: Remove redundant link and fix --debug processing #43375

merged 2 commits into from
Jul 23, 2024

Conversation

alkis
Copy link
Contributor

@alkis alkis commented Jul 22, 2024 •
edited by kou
Loading

Rationale for this change

  • We don't need to link to libarrow explicitly because parquet_shared/parquet_static does it automatically
  • --help shows --debug but the implementation accepts --json not --debug

What changes are included in this PR?

  • Remove the redundant libarrow link
  • Accept --debug not --json

Are these changes tested?

Manually.

Are there any user-facing changes?

Yes.

@alkis alkis mentioned this pull request Jul 22, 2024
Merged
@alkis alkis requested a review from wgtmac as a code owner July 22, 2024 21:49
kou
kou reviewed Jul 23, 2024
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Could you open a new issue instead of reusing an existing issue?
Our script doesn't support multiple PRs per issue.

cpp/tools/parquet/CMakeLists.txt Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 23, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 23, 2024
@alkis alkis changed the title GH-42102: [C++][Parquet] Address comments MINOR: [C++][Parquet] Address comments Jul 23, 2024
@alkis alkis requested a review from kou July 23, 2024 05:57
@alkis alkis changed the title MINOR: [C++][Parquet] Address comments MINOR: [C++][Parquet] Address comments #42174 Jul 23, 2024
@alkis alkis changed the title MINOR: [C++][Parquet] Address comments #42174 MINOR: [C++][Parquet] Address comments from #42174 Jul 23, 2024
@kou
Copy link
Member

kou commented Jul 23, 2024

Could you open a new issue instead of reusing an existing issue?
Our script doesn't support multiple PRs per issue.

@alkis
Copy link
Contributor Author

alkis commented Jul 23, 2024

Could you open a new issue instead of reusing an existing issue? Our script doesn't support multiple PRs per issue.

I opted for MINOR.

@kou kou changed the title MINOR: [C++][Parquet] Address comments from #42174 GH-43393: [C++][Parquet] parquet-dump-footer: Remove redundant link and fix --debug processing Jul 23, 2024
@github-actions GitHub Actions
Copy link

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

1 similar comment
@github-actions GitHub Actions
Copy link

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

@kou kou merged commit bc98167 into apache:main Jul 23, 2024
39 of 40 checks passed
@kou kou removed the awaiting change review Awaiting change review label Jul 23, 2024
@kou kou mentioned this pull request Jul 23, 2024
Closed
@conbench-apache-arrow conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner

@kou kou Awaiting requested review from kou

Assignees
No one assigned
Labels
Component: C++ Component: Parquet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alkis @kou