Skip to content

GH-36643: [C++][Parquet] Use nested namespace in parquet#36647

Merged
pitrou merged 3 commits intoapache:mainfrom
mapleFU:parquet/using-nested-namespace
Jul 13, 2023
Merged

GH-36643: [C++][Parquet] Use nested namespace in parquet#36647
pitrou merged 3 commits intoapache:mainfrom
mapleFU:parquet/using-nested-namespace

Conversation

@mapleFU
Copy link
Copy Markdown
Member

@mapleFU mapleFU commented Jul 12, 2023 •
edited by github-actions bot
Loading

Rationale for this change

Just for code style, using nested namespace rather than before

What changes are included in this PR?

Using nested namespace

Are these changes tested?

no

Are there any user-facing changes?

no

@mapleFU mapleFU requested a review from wgtmac as a code owner July 12, 2023 16:53
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 12, 2023

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

@mapleFU mapleFU changed the title GH-36643: [C++][{arquet] using nested namespace in parquet GH-36643: [C++][Parquet] using nested namespace in parquet Jul 12, 2023
@mapleFU mapleFU force-pushed the parquet/using-nested-namespace branch from 00d4b74 to 6ffc9ff Compare July 12, 2023 17:10
@wgtmac wgtmac changed the title GH-36643: [C++][Parquet] using nested namespace in parquet GH-36643: [C++][Parquet] Use nested namespace in parquet Jul 13, 2023
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 13, 2023
kou
kou approved these changes Jul 13, 2023
View reviewed changes
Copy link
Copy Markdown
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.

+1

cpp/src/parquet/level_conversion_inc.h
namespace parquet::internal {
#ifndef PARQUET_IMPL_NAMESPACE
#error "PARQUET_IMPL_NAMESPACE must be defined"
#endif
Copy link
Copy Markdown
Member

@kou kou Jul 13, 2023

Choose a reason for hiding this comment

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

We may want to use namespace parquet::internal::PARQUET_IMPL_NAMESPACE.

Copy link
Copy Markdown
Member Author

@mapleFU mapleFU Jul 13, 2023

Choose a reason for hiding this comment

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

Done

@github-actions github-actions bot added awaiting merge Awaiting merge awaiting review Awaiting review and removed awaiting committer review Awaiting committer review labels Jul 13, 2023
@github-actions github-actions bot added awaiting review Awaiting review and removed awaiting review Awaiting review awaiting merge Awaiting merge labels Jul 13, 2023
@mapleFU
Copy link
Copy Markdown
Member Author

mapleFU commented Jul 13, 2023 

error    libmamba Failed to download package from https://conda.anaconda.org/conda-forge/nomkl-1.0-h5ca1d4c_0.tar.bz2 (status 404)
Multi-download failed. Reason: Transfer finalized, status: 404 [https://conda.anaconda.org/conda-forge/nomkl-1.0-h5ca1d4c_0.tar.bz2] 568 bytes

Seems this doesn't caused by my patch https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/47536246 ?

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 13, 2023
pitrou
pitrou approved these changes Jul 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

A bit gratuitous, but LGTM

@pitrou pitrou merged commit c9fbc88 into apache:main Jul 13, 2023
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jul 13, 2023
@pitrou
Copy link
Copy Markdown
Member

pitrou commented Jul 13, 2023

Seems this doesn't caused by my patch https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/47536246 ?

No, this build has been failing lately :-(

@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Jul 13, 2023
@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow bot commented Jul 23, 2023

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

There were 3 benchmark results indicating a performance regression:

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

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

Reviewers

@kou kou kou approved these changes

@pitrou pitrou pitrou approved these changes

@wgtmac wgtmac wgtmac approved these changes

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++ Component: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++][Parquet] Use C++17's nested namespaces

4 participants

@mapleFU @pitrou @kou @wgtmac