leerho commented on code in PR #528: URL: https://github.com/apache/datasketches-cpp/pull/528#discussion_r4088419872
########## restructure-draft-v2.md: ########## @@ -0,0 +1,368 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> + +# datasketches-cpp restructure — draft v2 + +I have created this restructure document and its associated restructure-plan-detail.md with the help of Claude. I have personally reviewed this material and it makes sense to me. But, of course, it might contain some mistakes. Please review these two documents and give me your feedback. +-- [email protected] + +**Status: 2026-09-22** +* This is a draft for community discussion. +* All sequencing — the PR order, the per-area breakdown and the reviewer-load techniques — is in the companion `restructure-plan-detail.md`. +* Every header in the current tree is accounted for below: kept, moved, renamed, split, or deleted. + +**Goal:** When finished, I expect this to ship as a **major version** (6.0.0). + +--- + +## 0. Precedent + +This layout — a single include root, one subdirectory per component with a matching nested namespace, and an `internal/` subdirectory for everything outside the supported API — is the mainstream C++ library convention rather than anything novel. + +* **Boost** — `boost/asio/*.hpp` and `boost/asio/detail/` in `boost::asio` and `boost::asio::detail`. Boost is also the precedent for our one deviation from path ≡ namespace: `boost/core/*.hpp` declares into plain `boost::`, exactly as our `common/` declares into `datasketches::` (convention 7). +* **Apache Arrow (C++)** — `arrow/compute/*.h` in `arrow::compute`, with private code in `arrow::compute::internal`. The closest sibling to what we are doing, and a fellow ASF project. +* **Apache Thrift (C++)** — `thrift/protocol/` in `apache::thrift::protocol`. +* **POCO** — `Poco/Net/`, `Poco/Util/`, `Poco/XML/` in `Poco::Net`, `Poco::Util`, `Poco::XML`. Textbook path ≡ namespace. +* **AWS SDK for C++** — one directory and one namespace per service: `aws/s3/` in `Aws::S3`. It also accepts the same stutter we do, in `Aws::S3::S3Client`, for the same reason: the component name has to survive a `using namespace`. +* **Protocol Buffers** — `google/protobuf/` in `google::protobuf`, with `google::protobuf::internal` for the private half. + +Three more use the single include root and the `internal/` (or `detail/`) split but keep their namespaces deliberately flat, so they are precedent for the directory layout only: + +* **Abseil** (`absl/container/internal/`, namespace `absl::container_internal`) +* **Folly** (`folly/container/detail/`, namespace `folly::detail`) +* **LLVM** (`include/llvm/ADT/`, namespace `llvm::`). + +Both choices are respectable; we take the nested side because it matches the Java package layout. + +**Note:** `internal/` is a contract, not a wall: the library is header-only, so internal headers still ship and are still `#include`d by public headers. `internal` means "not for library users", not "private to one sketch" — tuple derives from `theta::internal` bases. + + +## 1. Definitions +**\<area>** - a place holder for a sketch family, grouping of similar sketches, or collection + +**User API** — dedicated to one sketch family. Public, user-facing, supported: stable across minor releases, changed only at a major release. Tested. Includes (a) anything a user is meant to call directly, and (b) any type that appears in a User API signature as a return type or parameter, even if users never spell it. + +* Location `include/datasketches/<area>/`, namespace `datasketches::<area>`. + +**Internal API** — dedicated to one sketch family, not **User API**. + +* Location `include/datasketches/<area>/internal/`, namespace `datasketches::<area>::internal`. + +**Shared User API** — User API used by more than one family. + +* Location `include/datasketches/common/`, namespace **`datasketches`** (not `datasketches::common` — see convention 7). + +**Shared Internal API** — shared across families, not User API. + +* Location `include/datasketches/internal/`, namespace `datasketches::internal`. + +**RPD** abbreviation for the file restructure-plan-detail.md + +**Shorthand** `X.hpp / _impl` means both `X.hpp` and `X_impl.hpp`; `←` marks a rename or move from today's location. + + + +## 2. Conventions + +1. **`_impl.hpp` files stay beside the header whose members they define.** They hold public-namespace member definitions; they are not internal. +2. **Constants namespaces stay intact.** `<area>_constants` namespaces and files are kept whole and re-parented (`datasketches::theta::theta_constants::DEFAULT_LG_K`), not folded or split. A constants file goes to `internal/` whole if nothing public references it (`hll_constants`, `req_constants`). If a file mixes public constants with internal code — only `cpc_common.hpp` — the constants namespace moves whole into the public sketch header. The same applies to other existing sub-namespaces (`random_utils`, `bit_array_ops`, `fdlibm`): re-parented, not dissolved. +3. **Tests mirror the include tree** under `test/datasketches/`. Cross-language `.sk` fixtures and `*_serialize_for_java.cpp` move with their sketch. +4. **Naming.** A User API header's prefix is the sketch name (`theta_sketch.hpp`, `var_opt_union.hpp`, `array_of_doubles_sketch.hpp`). The directory and namespace are the sketch name or an established abbreviation of it (`hll`, `cpc`, `kll`, `req`, `var_opt`, `aod`, `aos`). Internal headers have no naming requirement; if one already carries a prefix, leave it. +5. **Categories nest by category, not by dependency layer.** An area that is a category of sketches (`sampling`, `filters`, `tuple`) gets one subdirectory and sub-namespace per sketch or summary family, all siblings. Inheritance chains are not reflected in the tree (tuple depends on theta and is its sibling). +6. **A namespace never shares its name with a type inside it.** Hence `filters/bloom/` (type `bloom_filter`), `tuple/array_tuple/` (type `array<T>`). +7. **Path ≡ namespace, with one exception:** `include/datasketches/common/` declares into `datasketches::`, the way `boost/core/` declares into `boost::`. This keeps the top-level directory clean without forcing `common::` onto `serde`, `resize_factor`, `DEFAULT_SEED` and `string<A>` at their ~300 use sites. +8. **HLL** is brought to snake_case, `-internal.hpp` → `_impl.hpp`, and one public API per file, in its own PR before the move. + +## 3. Repo root + +``` +include/datasketches/… the library — the only thing that is installed +test/datasketches/… mirrors include/datasketches/ +benchmarks/ performance benchmarks; not installed +cmake/ CMake package config templates +tools/ rat-check.sh, Doxyfile, .rat-excludes, migration scripts +.github/ CI workflows +build/ tracked only for its .gitignore placeholder + +CMakeLists.txt +version.cfg.in base version; 5.3. → 6.0. at PR 1 +LICENSE required by ASF release policy +NOTICE required by ASF release policy +README.md +CODE_OF_CONDUCT.md +CONTRIBUTING.md +.asf.yaml +.clang-tidy stays at the root; clang-tidy and clangd search upward for it +.gitattributes +.gitignore +.pre-commit-config.yaml +``` + +## 4. Target tree + +``` +include/datasketches/ +├── version.hpp (generated) datasketches +│ +├── common/ datasketches +│ ├── common_defs.hpp DEFAULT_SEED, resize_factor, string<A> — public half of today's file +│ ├── serde.hpp +│ ├── quantiles_sorted_view.hpp / _impl exported by kll, quantiles, req +│ └── kolmogorov_smirnov.hpp / _impl user-called; compares two quantiles_sorted_views +│ +├── internal/ datasketches::internal Review Comment: _detail_ has good precedent (Boost, libstdc++), but I'd prefer to stay with _internal_. The saving is two characters per qualified name, and _internal_ explains itself to someone reading a path or an error message without knowing the Boost convention. It also has solid C++ precedent in Abseil (absl/container/internal/) and Arrow (arrow::compute::internal), and it's the word Java developers already know (jdk.internal). Most of our contributors work in both languages, so a term that means the same thing on both sides helps with cross-language work. Unless reviewers have a strong preference otherwise, I'd like to keep _internal_. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
