From e3c4c53bb0f834eebad3c1f96591c44696c66783 Mon Sep 17 00:00:00 2001 From: "jinli.zjw" Date: Fri, 6 Nov 2026 19:25:25 +0800 Subject: [PATCH] docs: clarify coding and development guidelines --- AGENTS.md | 6 +- CONTRIBUTING.md | 104 +++++++++++++++++-- docs/code-style.md | 178 +++++++++++++++++++++------------ docs/development-guidelines.md | 129 ++++++++++++++++++++++++ 4 files changed, 340 insertions(+), 77 deletions(-) create mode 100644 docs/development-guidelines.md diff --git a/AGENTS.md b/AGENTS.md index 9511a133f..aeaf40eca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ # AGENTS.md This file provides repository-specific instructions for coding agents working on Paimon C++. -For contributor-facing setup and the complete coding conventions, also read [`CONTRIBUTING.md`](CONTRIBUTING.md) and [`docs/code-style.md`](docs/code-style.md). +For contributor-facing setup, coding conventions, and design and testing principles, also read [`CONTRIBUTING.md`](CONTRIBUTING.md), [`docs/code-style.md`](docs/code-style.md), and [`docs/development-guidelines.md`](docs/development-guidelines.md). ## Scope @@ -58,7 +58,9 @@ When changing a public API, check the declaration under `include/paimon/`, its i ## C++ Requirements -The full rules are in [`docs/code-style.md`](docs/code-style.md). In particular: +The full coding rules are in [`docs/code-style.md`](docs/code-style.md). +For design choices and regression test coverage, follow [`docs/development-guidelines.md`](docs/development-guidelines.md). +In particular: - Use C++17; do not introduce C++20 or later features. - Use `Status` and `Result` for fallible operations. Do not use exceptions for production error propagation. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2355f95af..a35d596a6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,7 +53,7 @@ Choose a type and optional scope that accurately describe the change. Keep the d ## Submitting Pull Requests 1. **Fork** the repository and create a feature branch from `main`. -2. Make your changes following the [Code Style Guide](docs/code-style.md). +2. Make your changes following the [coding and development guidelines](#coding-guidelines). 3. Add or update tests for the functionality you changed. 4. Ensure all checks pass. 5. Open a pull request against `main`. Fill in the [PR template](.github/PULL_REQUEST_TEMPLATE.md). @@ -71,6 +71,8 @@ Before submitting, please verify: - [ ] Error handling uses `Status` / `Result` — no exceptions. - [ ] New utility code checks for existing helpers before reinventing (see the [utility reference](docs/code-style.md#reuse-existing-utilities)). - [ ] Tests are added or updated for the changed functionality. +- [ ] Design and regression coverage follow the [Development Guidelines](docs/development-guidelines.md). +- [ ] Copied or adapted external code follows the [attribution requirements](#external-code-attribution). - [ ] PR description follows the [template](.github/PULL_REQUEST_TEMPLATE.md). --- @@ -83,18 +85,87 @@ Before submitting, please verify: - **CMake** ≥ 3.22 - **Python 3** (for linting scripts and pre-commit) +### Pre-commit Hooks (Recommended) + +Install [pre-commit](https://pre-commit.com/) and the Git hooks: + +```bash +pip install pre-commit +pre-commit install +``` + +The hooks automatically run on every commit: + +| Hook | What it checks | +|------|----------------| +| `clang-format` | C++ formatting (`.clang-format`) | +| `cpplint` | Google C++ lint rules | +| `cmake-format` | CMake file formatting (`.cmake-format.py`) | +| `codespell` | Spelling errors | +| `trailing-whitespace` | Trailing whitespace | +| `end-of-file-fixer` | Missing newline at end of file | +| `check-added-large-files` | Files > 5 MB | +| `check-yaml` | YAML syntax | +| `sphinx-lint` (alias `docs`) | reStructuredText syntax under `docs/source/` | + +Run hooks for changed files during development, or for the entire repository before submitting: + +```bash +pre-commit run --files +pre-commit run --all-files +``` + +### Building and Testing + +Run the following commands from the project root. Reuse an existing configured build directory +when it is compatible with the change. Otherwise, configure a debug build with tests: + +```bash +cmake -S . -B build \ + -DCMAKE_BUILD_TYPE=Debug \ + -DPAIMON_BUILD_TESTS=ON +``` + +Start with the relevant test target and suite. For example, for `CoreOptionsTest`: + +```bash +cmake --build build --target paimon-core-test -j "$(nproc)" +./build/debug/paimon-core-test --gtest_filter='CoreOptionsTest.*' +``` + +Broaden validation for public API, shared infrastructure, build configuration, or cross-module +changes. To build and run all unit tests: + +```bash +cmake --build build --target unittest -j "$(nproc)" +``` + +The CI build entry point is `ci/scripts/build_paimon.sh`; it may rebuild dependencies and take +substantially longer than a focused target. For complete build options and dependency setup, +see [Building Paimon C++](docs/source/build_system.rst). + +Before handing off changes, review the diff and check whitespace: + +```bash +git diff --check +``` + +Report which validation ran and any checks that could not run, including the reason. Use the +[PR template](.github/PULL_REQUEST_TEMPLATE.md) to describe test coverage in the pull request. + --- -## Code Style +## Coding Guidelines -Please read the full [Code Style Guide](docs/code-style.md) before writing code. Key highlights: +Read both guides before writing code. Each guide owns a different set of rules: -- **Formatting**: Google C++ Style base, 100-column limit, 4-space indent. Let clang-format handle it. -- **Error handling**: Use `Status` / `Result` and project macros (`PAIMON_RETURN_NOT_OK`, `PAIMON_ASSIGN_OR_RAISE`). No exceptions. -- **Naming**: PascalCase for classes and methods, snake_case for variables, trailing `_` for members. -- **Headers**: `#pragma once`, includes ordered by category, Apache 2.0 license header on every file. -- **Factory pattern**: Use `static Create()` + private constructor when construction can fail. -- **Testing**: Google Test, `ASSERT_*` preferred over `EXPECT_*`, test files named `*_test.cpp` next to source. +| Guide | Scope | +|-------|-------| +| [Code Style Guide](docs/code-style.md) | Code expression: declarations, naming, ownership types, initialization, formatting, comments, and test macros | +| [Development Guidelines](docs/development-guidelines.md) | Design and testing decisions: defaults and overloads, function responsibilities, reuse, dependencies, return semantics, validation boundaries, and regression coverage | + +This contributing guide is the entry point for setup, validation, contribution workflow, and +external code attribution. Link to the detailed rules rather than maintaining parallel copies. --- @@ -117,3 +188,18 @@ If you make improvements that could benefit all developers, please update the te ## License By contributing to Paimon C++, you agree that your contributions will be licensed under the [Apache License, Version 2.0](http://www.apache.org/licenses/LICENSE-2.0). + +### External Code Attribution + +When copying or adapting external code, including code from another Apache project: + +- Record the upstream project, source file, and version or commit in an attribution comment near + the derived code. Explain local modifications where required by the upstream license. +- Preserve required copyright, license, and attribution notices. Follow the + [ASF source header policy](https://www.apache.org/legal/src-headers.html) when handling + third-party files; do not replace their headers with the standard ASF header. +- Update [LICENSE](LICENSE) with the affected files and applicable license information, including + license texts under `licenses/` when needed. Add required third-party notices to [NOTICE](NOTICE) + according to the [ASF third-party notice policy](https://www.apache.org/legal/resolved.html#required-third-party-notices). +- Include attribution changes in the same pull request as the code they describe, and identify + the source in the PR description so reviewers can check the adaptation. diff --git a/docs/code-style.md b/docs/code-style.md index 3772f0f33..d1ac58ae3 100644 --- a/docs/code-style.md +++ b/docs/code-style.md @@ -21,6 +21,10 @@ This document defines the coding conventions for the Paimon C++ project. All pull requests are expected to follow these rules. Automated tooling (clang-format, clang-tidy, cpplint, pre-commit) enforces many of them. +For design and testing decisions, see the [Development Guidelines](development-guidelines.md). +For setup, validation commands, and the pull request checklist, see the +[Contributing Guide](../CONTRIBUTING.md). + --- ## Language Standard @@ -75,6 +79,18 @@ Formatting is based on **Google C++ Style** with the following overrides (define | `AccessModifierOffset` | -3 | | `AllowShortFunctionsOnASingleLine` | Empty | +### Control Flow Braces + +Use braces for every `if` and `else` body, even when it contains only one statement: + +```cpp +if (rows.empty()) { + return Status::OK(); +} else { + return WriteRows(rows); +} +``` + --- ## Naming Conventions @@ -93,11 +109,34 @@ Formatting is based on **Google C++ Style** with the following overrides (define --- +## Function Declarations and Parameter Order + +- Write parameter names in function declarations so readers can understand each argument. +- Put input parameters before output parameters. +- Use non-const pointers (`T*`) for output and input/output parameters. Do not use non-const + references (`T&`) for these parameters. Document whether `nullptr` is allowed. +- When adding a business parameter, put it before trailing context parameters such as `pool` + and `executor`, while keeping output parameters last. Keep the order consistent across related + interfaces and their implementations. + +```cpp +Status ReadOffsets(const std::string& path, int64_t limit, MemoryPool* pool, + std::vector* offsets); +``` + +Choose parameter types according to the [ownership conventions](#smart-pointers--memory). +For when to introduce defaults and overloads, see the +[Development Guidelines](development-guidelines.md#default-arguments-and-overloads). + +--- + ## File Layout ### License Header -Every file **must** start with the Apache 2.0 license header. +Project-authored source and documentation files **must** start with the Apache 2.0 license header. +For copied or adapted external code, follow the +[attribution requirements](../CONTRIBUTING.md#external-code-attribution). ```cpp /* @@ -204,6 +243,20 @@ class TableScan { - All fallible initialization goes in `Create()`, **after** construction. - If construction cannot fail (POD, data-only classes), a public constructor is fine. +### Member Initialization + +- Give default-constructible structs and classes an explicit initial state. Initialize scalar + members in their declarations or in every constructor that can create the object. +- A default value must represent a meaningful state. Require business-critical inputs through a + constructor or `Create()` rather than supplying a placeholder solely to allow default construction. + +```cpp +struct ReadRange { + int64_t offset = 0; + int64_t length = 0; +}; +``` + ### Utility Classes Static-only utility classes delete their constructors and deconstructors: @@ -237,6 +290,26 @@ virtual ~ClassName() = default; - Use `std::optional` for optional values. - Use `ScopeGuard` (in `src/paimon/common/utils/`) for RAII cleanup. +### Parameter Types and Ownership + +Express whether an argument is borrowed, transferred, or shared: + +| Relationship | Parameter convention | +|--------------|----------------------| +| Borrow a required input object | A reference, usually `const T&` for read-only access | +| Borrow an optional input object | A pointer, usually `const T*` for read-only access; document `nullptr` | +| Transfer sole ownership | `std::unique_ptr` by value | +| Retain shared ownership | `std::shared_ptr` by value | + +Match established pointer or reference conventions in the interface when borrowing input parameters, +and document whether a pointer may be null. A borrowed argument must remain alive for the entire use, +including asynchronous work. Do not introduce a `shared_ptr` merely to pass a borrowed object. + +Pass complex objects by `const T&` when only reading them. Accept them by value when the function +needs a deliberate copy or consumes the value, moving it into retained storage when appropriate. +Avoid redundant conversions and temporary ownership wrappers; use the required target type +directly when a project macro already performs the move or conversion. + ### Checked Class-Pointer Casts Include `paimon/common/utils/checked_cast.h` and use the Paimon cast helpers for conversions @@ -284,8 +357,13 @@ cast for those cases. - Use `///` (Doxygen) for classes and public methods. - Use `@param` and `@return` for parameters and return values. +- Explain non-obvious boolean meanings, map keys and values, units, null or empty return values, + and ownership or lifetime requirements in the interface contract. +- Explain the reason for a surprising implementation or workaround, including any constraint + that a future change must preserve. - Do **not** comment obvious code. -- Inline comments go **above** the line, not at the end. +- Explanatory comments go **above** the line, not at the end. Use the parameter labels below + when a comment needs to identify an argument at a call site. ```cpp /// Create a scan plan from the current snapshot. @@ -295,6 +373,15 @@ cast for those cases. Result> CreatePlan(int64_t snapshot_id); ``` +### Call-Site Parameter Labels + +When a literal or sentinel argument is unclear at the call site, use `/*parameter_name=*/` +immediately before it. Match the name in the declaration: + +```cpp +CreateReader(path, /*enable_prefetch=*/true, pool); +``` + ### TODO Comments TODOs are allowed but **must** include an author tag: @@ -409,7 +496,7 @@ Before writing new helper code, check whether the project already provides what | `GatedAsyncInputStream` | `InputStream` wrapper holding `ReadAsync` callbacks until `ReleaseAll()`, so a test can observe a cache while its fetches are in flight | | `BuildTestSnapshot()` | Build a `Snapshot` from an id and optional uuid, filling in every other field | | `VariantTestData` | Build variant test data, including an Arrow `StructArray` batch | -| `testharness.h` | Test macros: `ASSERT_OK`, `ASSERT_NOK`, `ASSERT_NOK_WITH_MSG`, `ASSERT_RAISES` | +| `testharness.h` | Status and Result assertion macros; see [Testing](#testing) | ### Mock Objects (`src/paimon/testing/mock/`) @@ -437,42 +524,34 @@ Before writing new helper code, check whether the project already provides what - Test files live **next to** the source file they test, named `*_test.cpp`. - Use **Google Test** (`gtest`). - Test classes go in `namespace paimon::test`. -- Use project test macros: `ASSERT_OK`, `ASSERT_NOK`, `ASSERT_NOK_WITH_MSG`. +- Name suites after the component or behavior under test, and test cases after the scenario and + expected behavior. For example, `ReadAheadCacheTest.EmptyReadDoesNotFetch` describes what fails. +- Use the macros from `paimon/testing/utils/testharness.h` for `Status` and `Result` checks: + + | Macro | Purpose | + |-------|---------| + | `ASSERT_OK(expr)` | Require successful `Status` or `Result` | + | `ASSERT_OK_AND_ASSIGN(lhs, rexpr)` | Require success and extract a `Result` value | + | `ASSERT_NOK(expr)` | Require failure when the reason is not relevant to the test | + | `ASSERT_NOK_WITH_MSG(expr, message)` | Require failure with the expected message substring | + +- For error cases, check the specific failure reason, normally with `ASSERT_NOK_WITH_MSG`, so + an unrelated error does not make the test pass. Check the status code too when it is part of + the contract being tested. - **Prefer `ASSERT_*` over `EXPECT_*`** — `ASSERT_*` stops the test immediately on failure, preventing cascading errors. - - **Exception**: In non-void helper functions, use `EXPECT_*` because `ASSERT_*` expands to `return;` which is incompatible with non-void return types. + - **Exception**: `ASSERT_*` cannot return from a non-void helper. Use `EXPECT_*` only when it is + safe to continue, or return `Status` / `Result` and assert success in the test. Do not + access a failed `Result` after a nonfatal assertion. + +For test coverage and asynchronous or sanitizer regressions, see +[Effective Regression Tests](development-guidelines.md#effective-regression-tests). --- ## Tooling -### Pre-commit Hooks (Recommended) - -The easiest way to stay compliant is to install [pre-commit](https://pre-commit.com/): - -```bash -pip install pre-commit -pre-commit install -``` - -This installs Git hooks that automatically run on every commit: - -| Hook | What it checks | -|------|----------------| -| `clang-format` | C++ formatting (`.clang-format`) | -| `cpplint` | Google C++ lint rules | -| `cmake-format` | CMake file formatting (`.cmake-format.py`) | -| `codespell` | Spelling errors | -| `trailing-whitespace` | Trailing whitespace | -| `end-of-file-fixer` | Missing newline at end of file | -| `check-added-large-files` | Files > 5 MB | -| `check-yaml` | YAML syntax | -| `sphinx-lint` (alias `docs`) | reStructuredText syntax under `docs/source/` | - -To run all hooks on all files manually: - -```bash -pre-commit run --all-files -``` +See [Development Setup](../CONTRIBUTING.md#development-setup) for installing and running pre-commit +hooks. The sections below describe the static analysis conventions. ### clang-tidy @@ -484,36 +563,3 @@ Static analysis is configured in `.clang-tidy`. Key enabled check groups: - `modernize-*` — C++ modernization suggestions, except `modernize-return-braced-init-list`, `modernize-avoid-c-arrays`, `modernize-use-trailing-return-type`, `modernize-use-nodiscard` and `modernize-pass-by-value`. - `clang-analyzer-*` — Clang static analyzer, except the `clang-analyzer-alpha*` checkers and `clang-analyzer-cplusplus.NewDeleteLeaks`. - `clang-diagnostic-*` — Compiler diagnostics, except `clang-diagnostic-global-constructors` and `clang-diagnostic-sign-compare`. - -### Building & Testing - -```bash -# Configure (from project root) -mkdir -p build && cd build -cmake ../ \ - -DCMAKE_BUILD_TYPE=debug \ - -DPAIMON_BUILD_TESTS=ON - -# Build -make -j$(nproc) - -# Run a specific test suite -./debug/paimon-core-test --gtest_filter="CoreOptionsTest*" -``` - ---- - -## Pull Request Checklist - -Before submitting a PR, please verify: - -- [ ] Code compiles without warnings under `-Wall` (enabled by default in the build system). -- [ ] `pre-commit run --all-files` passes (or individual tools: clang-format, cpplint, codespell). -- [ ] New / modified public APIs are marked with `PAIMON_EXPORT`. -- [ ] Every new file has the Apache 2.0 license header. -- [ ] Error handling uses `Status` / `Result` — no exceptions. -- [ ] New utility code checks for existing helpers before reinventing. -- [ ] Tests are added or updated for the changed functionality. -- [ ] `ASSERT_*` is preferred over `EXPECT_*` in tests. -- [ ] No raw `new` / `delete` outside factory methods. -- [ ] PR description follows the [template](../.github/PULL_REQUEST_TEMPLATE.md). diff --git a/docs/development-guidelines.md b/docs/development-guidelines.md new file mode 100644 index 000000000..01e60aa47 --- /dev/null +++ b/docs/development-guidelines.md @@ -0,0 +1,129 @@ + + +# Apache Paimon C++ Development Guidelines + +This document covers design and testing decisions that depend on the problem being solved. +Use the [Code Style Guide](code-style.md) for coding conventions and the +[Contributing Guide](../CONTRIBUTING.md) for setup, validation commands, and pull request workflow. + +## Default Arguments and Overloads + +- Give a parameter a default value only when omission has a clear meaning for production callers. + A sentinel such as `-1` or `nullptr` needs a documented meaning and handling path. +- Do not introduce a default argument or constructor overload solely to simplify a test or avoid + updating callers when adding a required dependency. Supply the dependency explicitly and use + existing test helpers to construct it. +- Add an overload when it represents a useful operation for callers. Keep initialization and + validation consistent across overloads by delegating to a common implementation where useful. + +For example, if a reader requires a context in production, tests should provide a valid context +rather than motivate a constructor that silently accepts a missing one. + +## Function Responsibilities and Reuse + +- Split a function when it mixes separate responsibilities or its control flow is hard to follow. + Extract a coherent operation with clear inputs, outputs, and error handling. There is no fixed + line count that requires a split. +- Check the [existing utility reference](code-style.md#reuse-existing-utilities) before adding a + helper. Reuse logic when its behavior and responsibility match the caller's needs. +- A small amount of local repetition or a short wrapper does not by itself justify a new + abstraction. Consider whether the helper gives a meaningful name to an operation and whether + callers share the same contract, rather than merely similar syntax. +- Keep a helper near its callers until there is a reason to share it more widely. Avoid exposing + an implementation detail through a new public API just to make testing easier. + +For example, extracting schema validation from a function that also opens files can clarify both +operations. Wrapping a single expression used once may only add another place for readers to look. + +## Module Organization and Dependencies + +- Keep generic `common` and `core` code independent of format plugin implementation headers. + When both layers need a contract, put the shared interface in an appropriate common layer and + let the plugin implement it. +- Keep a feature's readers, writers, and related utilities together in the module that owns the + behavior. Put a helper in a shared utility directory only if it has a suitable shared purpose. +- Define a shared type alias once at the narrowest layer that all users can depend on. Avoid + repeating aliases across headers or moving plugin-specific types into a generic layer. + +For example, core code should consume a reader interface instead of including a particular +format's reader implementation to access a shared type. + +## Return Semantics and Validation Boundaries + +Choose and document return values so callers can distinguish these outcomes: + +| Outcome | Contract | +|---------|----------| +| Operation succeeded with no data | A valid empty result, or a documented end-of-input representation | +| Operation was not performed | An explicit state or documented return value that callers handle | +| Operation is unsupported | A documented unsupported outcome or an appropriate error `Status` | +| Operation failed | An error `Status` or `Result` that preserves the failure reason | + +The representation depends on the interface. A documented `nullptr` sentinel can be appropriate; +do not replace it with an empty object without checking how callers distinguish the states. + +- Validate required dependencies and input at the boundary that owns them, such as a builder, + `Create()`, or an entry point consuming external data. Reject invalid state before starting + work that relies on it. +- Internal code can rely on established invariants. Add another runtime check only when that + layer can encounter a new invalid state, rather than repeating the same check at every call. +- Keep defensive code proportionate to reachable failure paths and interface contracts. Avoid + speculative checks and fallback branches for states excluded by the input domain or established + invariants. Add overflow checks where arithmetic can exceed its range for inputs that can + reach the operation, including external input. +- Files, schemas, and imported data still need runtime validation. A debug assertion or + [checked class-pointer cast](code-style.md#checked-class-pointer-casts) does not validate + recoverable external input. +- Propagate failures without turning them into success, empty data, or an unsupported outcome. + +For example, validate a required executor when constructing an asynchronous reader. Its internal +methods can then use that invariant, while each external byte range still needs the validation +appropriate to that read. + +## Effective Regression Tests + +- Exercise the affected production entry point and assert behavior that would expose the target + defect. When practical, confirm that the test fails with the fix removed. A test that reproduces + an algorithm locally without calling the changed production code does not cover that code. +- Use mocks at the relevant dependency boundary to make failures deterministic while keeping the + behavior under test in production code. Test failures and boundary cases affected by the change. + Configure only the mock outcomes needed for the scenario, and assert production behavior + rather than merely rechecking configured mock responses. +- Keep boundary tests focused on meaningful input ranges and concrete defects. Add overflow and + extreme-value cases when they exercise a reachable failure, an API or format requirement, or a + known regression. Do not expand test matrices solely to cover speculative states introduced by + unnecessary defensive checks. +- Wait for asynchronous work and callbacks to finish before checking their results or destroying + their dependencies. Use synchronization primitives or existing test helpers instead of sleeps + that assume a particular execution speed. +- Make sanitizer findings fail the check. A regression test that exits successfully after the + sanitizer reports the target defect cannot guard against that defect. Configure the sanitizer + or runner to return failure instead of merely printing diagnostics. +- In unit and integration tests, check observable results such as returned rows, statuses, and + committed state. Assert mock interactions, call counts, or call order only when needed to verify + a contract or a specific resource or performance requirement, such as avoiding redundant I/O. + Avoid numerous variations of these assertions that lock tests to incidental implementation + details and fail after a refactor that preserves the contract. + +For example, a fix to read-ahead behavior needs a test that reads through the affected reader or +cache entry point. A separate byte-copy exercise cannot establish that the reader uses the fix. + +See the [testing conventions](code-style.md#testing) for assertion macros and test names, and +[Building and Testing](../CONTRIBUTING.md#building-and-testing) for validation commands.