Repository structure modernization

オープン
#502 コメント 11 件 リアクション 1 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
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.hpp uses _HLLARRAY_HPP_ (leading underscore, reserved by the standard); theta/include/theta_sketch.hpp uses THETA_SKETCH_HPP_. Switching all headers to #pragma once removes 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.txt repeats the Catch2 wiring and test-discovery setup. Consolidating into a single test/ 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時間
マージ済み PR(30日)
9

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

apache/datasketches-cpp のほかの issue

apache/datasketches-cpp の issue をすべて見る

似ている issue

C++ の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。