Skip to content

GH-55455: [C++] Make level comparison header internal #55456

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

Open
09Catho wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −9
Open

GH-55455: [C++] Make level comparison header internal #55456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_comparison.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

#include <array>

#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"
#include "parquet/level_comparison_simd_internal.h"

#define PARQUET_IMPL_NAMESPACE standard
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_comparison_inc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#include "arrow/util/bit_util.h"
#include "arrow/util/endian.h"
#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"

// Used to make sure ODR rule isn't violated.
#ifndef PARQUET_IMPL_NAMESPACE
Expand Down
0 cpp/src/parquet/level_comparison.h → cpp/src/parquet/level_comparison_internal.h
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_comparison_simd_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#include <cstdint>

#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"

namespace parquet::internal {

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_comparison_simd_kernel_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include <limits>

#include "arrow/util/endian.h"
#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"

namespace parquet::internal {

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_conversion_benchmark.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <vector>

#include "benchmark/benchmark.h"
#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"
#include "parquet/level_conversion.h"

constexpr int64_t kLevelCount = 2048;
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_conversion_inc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/simd.h"
#include "parquet/exception.h"
#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"

#ifndef PARQUET_IMPL_NAMESPACE
# error "PARQUET_IMPL_NAMESPACE must be defined"
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_conversion_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

#include "parquet/level_conversion.h"

#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"
#include "parquet/test_util.h"

#include <gmock/gmock.h>
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/parquet/level_decoder_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "arrow/util/rle_encoding_internal.h"
#include "arrow/util/ubsan.h"
#include "parquet/exception.h"
#include "parquet/level_comparison.h"
#include "parquet/level_comparison_internal.h"
#include "parquet/types.h"

namespace parquet {
Expand Down
1 change: 0 additions & 1 deletion cpp/src/parquet/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ install_headers(
'file_writer.h',
'hasher.h',
'index_location.h',
'level_comparison.h',
'level_comparison_inc.h',

@mapleFU mapleFU Nov 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little bit curious why level_comparison_inc.h is listed here

@09Catho 09Catho Nov 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It was already in the install list and I only removed the renamed header. But this points at a real problem with the change. level_comparison_inc.h and level_conversion_inc.h are only included from level_comparison.cc, level_conversion.cc and level_conversion_bmi2.cc, yet both still get installed: Meson lists them, and CMake's ARROW_INSTALL_ALL_HEADERS installs any header without "internal" in its name. After this PR both include parquet/level_comparison_internal.h, which is no longer installed, so the installed copies would include a header that isn't there.

I can give both an _internal name as well and drop them from the Meson list. Should I do that here, or keep this PR to level_comparison.h and open a follow-up?

'level_conversion.h',
'level_conversion_inc.h',
Expand Down
Loading