Repository structure modernization
还没有人认领这个 Issue。
评估
- 难度
- 5/5
- 预计耗时
- 一周以上
- 新手友好度
- 35/100
- Issue 类型
- 重构
- 描述清晰度
- 需要澄清
- 活跃度
- 活跃
- 技术栈
- cmake, cpp
调研方向
首先阅读 issue 中提到的 hll/CMakeLists.txt,以及每个 sketch 对应的 include 和 test CMakeLists 文件。追踪安装了哪些 headers,以及 test discovery 是如何重复进行的,然后将提议的 include 布局和 detail 目录与当前的 consumer 进行比较。在开始实现之前,项目就目标位置和迁移范围达成一致,即表示完成。
由索引模型根据 Issue 内容生成。
描述
We'd like to discuss two friction points in the repo layout, both about the public include surface. Filing as a discussion because any change here would break the public API and warrant a major version bump; better to agree on the destination before anyone writes a patch.
Problem 1: Generic top-level header names collide across libraries
Public headers sit at shallow paths with generic filenames, installed flat under a single directory:
hll/include/hll.hpp
theta/include/theta_sketch.hpp
common/include/serde.hpp
common/include/optional.hpp
# ...installed via `DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/DataSketches"`
Consumers write #include <hll.hpp>. Nothing in the path identifies the library, and names like hll.hpp, serde.hpp, optional.hpp, and tdigest.hpp are plausible choices for any number of unrelated projects. A consumer linking against datasketches alongside another library that also ships an hll.hpp ends up with two headers competing for the same include path, with no clean way to disambiguate.
The fix is to namespace the include path under the library name. Two shapes are worth considering.
Option A: Nested per-sketch directories, drop the filename prefix
include/datasketches/
hll/
sketch.hpp # was hll.hpp
detail/...
theta/
sketch.hpp # was theta_sketch.hpp
union.hpp # was theta_union.hpp
intersection.hpp
a_not_b.hpp
jaccard_similarity.hpp
detail/...
cpc/
sketch.hpp
detail/...
...
Consumer: #include <datasketches/theta/union.hpp>
The directory does the namespacing work; the filename drops the now-redundant theta_ prefix. Matches Boost (<boost/graph/adjacency_list.hpp>).
Option B: Flat namespace, keep descriptive filenames
include/datasketches/
hll_sketch.hpp # was hll.hpp
theta_sketch.hpp
theta_union.hpp
theta_intersection.hpp
theta_a_not_b.hpp
theta_jaccard_similarity.hpp
cpc_sketch.hpp
...
detail/
hll/...
theta/...
Consumer: #include <datasketches/theta_union.hpp>
The filename prefix carries the grouping, and the public surface stays on a single flat level. Matches Abseil (<absl/container/flat_hash_map.h>).
Either way, the install path becomes ${CMAKE_INSTALL_INCLUDEDIR}/datasketches/.
Problem 2: Public and internal headers live in the same directory
hll/include/ mixes the public hll.hpp with roughly 30 implementation headers (e.g. HllSketchImpl.hpp, AuxHashMap.hpp) plus another 15-odd *-internal.hpp files. All of them get installed by hll/CMakeLists.txt. From a consumer standpoint, this seems valid:
#include <DataSketches/HllSketchImpl.hpp>
#include <DataSketches/AuxHashMap-internal.hpp>
There is no clear API boundary between public and internal headers.
The fix is a per-sketch detail/ subdirectory, a convention Boost has used since the early 2000s and that Abseil mirrors with its internal/ directories. Implementation headers move there, and the contract for consumers is straightforward: anything inside detail/ is internal, and reaching in is at your own risk.
Low-priority items
- Header guards. Two styles coexist:
hll/include/HllArray.hppuses_HLLARRAY_HPP_(leading underscore, reserved by the standard);theta/include/theta_sketch.hppusesTHETA_SKETCH_HPP_. Switching all headers to#pragma onceremoves the inconsistency, the reserved-identifier risk, and a class of copy-paste mistakes. - No
.clang-format. Style is enforced by convention only. Committing a single file would replace formatting back-and-forth in code review. - Pick one casing convention. File names and code currently mix CamelCase and snake_case. Low-priority but easy on the eyes.
- Tests scattered per-sketch. Each
<sketch>/test/CMakeLists.txtrepeats the Catch2 wiring and test-discovery setup. Consolidating into a singletest/tree (with per-sketch subdirs) would declare the test framework once, give shared fixtures (random key generators, serde helpers) an obvious home. The current setup works; this is cleanup, not a fix.
- 主要语言
- C++
- 星标
- 273
- 派生
- 88
- 平均合并
- 1 天 19 小时
- 30 天内合并 PR
- 9
贡献指南
从这里开始
- 先读完整个 Issue,再读项目的贡献指南。
- 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
- Fork 仓库,在一个分支上完成修改。
- 提交 Pull Request,并在描述里引用这个 Issue 编号。
apache/datasketches-cpp 的其他 Issue
-
难度 2/5 1-3 小时 新手友好度 82/100
apache/datasketches-cpp#499 ·
-
难度 3/5 1-2 天 新手友好度 45/100
apache/datasketches-cpp#460 · 6 条评论 ·
-
难度 5/5 一周以上 新手友好度 45/100
apache/datasketches-cpp#457 · 8 条评论 ·
-
难度 5/5 一周以上 新手友好度 20/100
apache/datasketches-cpp#419 · 6 条评论 ·
-
难度 5/5 一周以上 新手友好度 25/100
apache/datasketches-cpp#416 · 13 条评论 ·
查看 apache/datasketches-cpp 的全部 Issue
相似的 Issue
-
enhancement
难度 1/5 1 小时以内 新手友好度 88/100
QuantStack/git2cpp#187 ·
-
难度 2/5 1-3 小时 新手友好度 86/100
-
难度 2/5 1-3 小时 新手友好度 84/100
mlcommons/mobile_app_open#1182 ·
-
Needs-Triage
难度 2/5 1-3 小时 新手友好度 78/100
microsoft/winget-cli#6547 ·
-
难度 1/5 1 小时以内 新手友好度 90/100
AXERA-TECH/ax-llm#77 ·