Repository navigation
Conversation
|
|
| 'hasher.h', | ||
| 'index_location.h', | ||
| 'level_comparison.h', | ||
| 'level_comparison_inc.h', |
There was a problem hiding this comment.
I'm a little bit curious why level_comparison_inc.h is listed here
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@pitrou Let's mark both of them to internal and removing from exporting?
|
General LGTM |
Rationale for this change
parquet/level_comparison.his only included by Parquet implementation files, but Meson installs it with the public Parquet headers. Renaming it as an internal header makes that boundary explicit and prevents it from being treated as public API.What changes are included in this PR?
level_comparison.htolevel_comparison_internal.h.This addresses #55455 without changing runtime behavior.
Are these changes tested?
git diff --checkpassed.parquet/level_comparison.hinclude or install reference.Are there any user-facing changes?
The header is no longer installed as a public Parquet header. No runtime behavior changes.
Was AI used for this PR?
In accordance with the Arrow AI-generation guidance:
PR code and description written by:
Reviewed before submission by:
Codex was used to inspect the issue, implement the scoped rename, and prepare this description. The complete diff was reviewed in this run, and the validation limits are stated above.
parquet/level_comparison.hinternal #55455