From 750779da12e7bc662be2c04c70d4e8bb13f890fd Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 30 Jul 2026 18:04:05 +0200 Subject: [PATCH] Add guidlines for AI contributions and reviews --- AGENTS.md | 86 ++++++++++++++ CLAUDE.md | 1 + docs/source/contributor-guide/01-index.md | 5 +- docs/source/contributor-guide/03-tests.md | 73 +++++++++++- .../source/contributor-guide/04-benchmarks.md | 66 +++++++++-- .../contributor-guide/05-code-review.md | 105 ++++++++++++++++++ docs/source/index.rst | 1 + src/distributed_planner/AGENTS.md | 36 ++++++ src/distributed_planner/CLAUDE.md | 1 + src/events/AGENTS.md | 23 ++++ src/events/CLAUDE.md | 1 + 11 files changed, 379 insertions(+), 19 deletions(-) create mode 100644 AGENTS.md create mode 100644 CLAUDE.md create mode 100644 docs/source/contributor-guide/05-code-review.md create mode 100644 src/distributed_planner/AGENTS.md create mode 100644 src/distributed_planner/CLAUDE.md create mode 100644 src/events/AGENTS.md create mode 100644 src/events/CLAUDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..329f7c3c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,86 @@ +# DataFusion Distributed agent guide + +## Project purpose + +DataFusion Distributed is a Rust library that adds distributed execution to +Apache DataFusion. It is deliberately a library, not a complete distributed +engine: preserve its familiar DataFusion API and avoid imposing assumptions +about a user's networking or deployment stack. + +## Repository map + +- `src/`: library implementation. + - `codec/`: Protobuf codecs for the execution plans this project ships + + machinery for wiring up custom user-defined codecs. + - `common/`: internal shared helpers. These are collections of reusable + functions that are not coupled to anything else in the project, they are + standalone helpers. + - `coordinator/`: Main execution plan implementation in charge of sending + plans to workers and execute them. + - `distributed_planner/`: transforms DataFusion physical plans for + distributed execution. See further instructions in + [src/distributed_planner/AGENTS.md](./src/distributed_planner/AGENTS.md) + - `events/`: event handlers where users can customize how this project + reacts to certain distributed events. See further instructions in + [src/events/AGENTS.md](./src/events/AGENTS.md) + - `execution_plans/`: distributed physical-plan nodes and test-only + execution benchmarks. Any ExecutionPlan implementation should be + contributed here as its own module. + - `metrics/`: task-metric collection and distributed-plan metric rendering. + - `protocol/`: coordinator-worker messages, channel resolution, and gRPC + transport. Anything gRPC specific should be contributed to + `protocol/grpc`. + - `test_utils/`: shared test fixtures, plans, resolvers, and assertions. + - `work_unit_feed/`: work-unit definitions, providers, registries, and + remote-feed support. + - `worker/`: worker service, sessions, task execution, and connections. +- `tests/`: integration and correctness tests. +- `examples/`: runnable usage examples. +- `benchmarks/`: benchmark tooling; do not change benchmark behavior as an + incidental part of a library change. +- `docs/`: Sphinx documentation. Contributor documentation is in + `docs/source/contributor-guide/`. + +## Always + +- Keep changes narrowly scoped; do not include unrelated refactors or dependency + upgrades. +- Do not introduce a public or wire-format breaking change unless it is + explicitly requested. +- Keep public documentation and runnable examples aligned with public API + changes. +- Follow established patterns in the closest relevant code and tests before + introducing a new abstraction or mechanism. +- Reuse existing helpers, extension points, and test fixtures when they fit. +- Introduce a new pattern only when existing ones cannot express the required + behavior; the rationale for this should be clearly and briefly stated. +- When claiming a performance improvement, never move a PR out of draft before + first qualifying the performance benefits following the guidelines in + `docs/source/contributor-guide/04-benchmarks.md`. +- When submitting PRs, make sure they are scoped to one thing, and that they + fall in one of these categories: + - Feature additions (PR prefixed with `feat:`) + - Bug fixes (PR prefixed with `fix:`) + - Refactors (PR prefixed with `refactor:`) + - Docs and examples (PR prefixed with `docs:`) + - Performance (PR prefixed with `perf:`) + +## Task-specific guidance + +Read the relevant contributor guide before starting work in these areas: + +- Code review: `docs/source/contributor-guide/05-code-review.md` +- Tests: `docs/source/contributor-guide/03-tests.md` +- Performance and benchmark changes: + `docs/source/contributor-guide/04-benchmarks.md` + +## Validation + +Run the narrowest relevant check first. Use the repository's standard format, +lint, test, and documentation commands as appropriate; the contributor guide +documents the full validation matrix. + +## Code review + +Read the code-review guide and the closest scoped `AGENTS.md`. Report concrete +correctness risks, not style issues covered by CI. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..43c994c2 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/docs/source/contributor-guide/01-index.md b/docs/source/contributor-guide/01-index.md index 7c887c89..9c3e8edd 100644 --- a/docs/source/contributor-guide/01-index.md +++ b/docs/source/contributor-guide/01-index.md @@ -3,5 +3,6 @@ Welcome to the DataFusion Distributed contributor guide! - [Setup](02-setup.md) - Getting started with development -- [Tests](03-tests.md) - Running unit and integration tests -- [Benchmarks](04-benchmarks.md) - Local and remote performance benchmarks +- [Tests](03-tests.md) - Writing and running unit and integration tests +- [Benchmarks](04-benchmarks.md) - Running, qualifying, and writing benchmarks +- [Code review](05-code-review.md) - Reviewing changes for correctness diff --git a/docs/source/contributor-guide/03-tests.md b/docs/source/contributor-guide/03-tests.md index b61a0ffb..e154c503 100644 --- a/docs/source/contributor-guide/03-tests.md +++ b/docs/source/contributor-guide/03-tests.md @@ -1,10 +1,63 @@ # Tests -When submitting code, make sure it's always covered by tests. For every important feature, -it's recommended to add a dedicated integration test that tests it end-to-end. +When submitting code, make sure it's always covered by tests. For every +important feature, it's recommended to add a dedicated integration test that +tests it end-to-end. -Please note that LLMs like to make very verbose and redundant tests even for simple things, -so before committing LLM-generated tests, review them and simplify them as much as possible. +Please note that LLMs like to make very verbose and redundant tests even for +simple things, so before committing LLM-generated tests, review them and +simplify them as much as possible. + +## Choose the right test level + +- Add a unit test for a small, deterministic behavior that can be exercised + without constructing a distributed query. Never couple unit tests to + implementation details. Unit tests are appropriate for very scoped and clearly + separated pieces of work, but not great for when there needs to be too many + moving pieces in the tests. +- Add an integration test in `tests/` for a user-visible behavior. When in + doubt, always prefer this type of tests. Never expose internals of the project + to these tests just for the sake of finer grained testing, always test at the + public API level. + +In general, prefer writing integration tests rather than unit tests. + +## writing good tests + +- Always wrap tests in a `mod tests {};` block. +- Never make the body of a test very long. prefer adding the necessary helpers + at the bottom of the `mod tests {};` block, and reuse these helpers + extensively across many permutations of different test scenarios. A good + example of this is + [inject_network_boundaries.rs](../../../src/distributed_planner/inject_network_boundaries.rs) + or + [complexity_cpu.rs](../../../src/distributed_planner/statistics/complexity_cpu.rs) + where there are many test cases, but each test case has a very narrowed and + readable body. +- While building tests, prefer using builder patterns for ergonomically building + different permutations of test cases, keeping the builder pattern helpers at + the bottom of the `mod tests {};` block. +- Never test implementation details, always prefer testing at the public API + level. +- Prefer quality to quantity. Adding a lot of tests with overlapping intentions + not only does not increase coverage, but also introduces maintenance burden in + the project. + +## Red flags in tests + +- Too many tests with overlapping intentions. +- Tests with big bodies and a lot of in-lined preparation steps. +- Tests that do not follow the code pattern of adjacent tests. +- Exposing internal details of tested structs with #[cfg(test)] flags. +- PRs with a high ratio of tests VS actual code. + +## Review test changes + +- Confirm the test would fail before the production change. +- Check that it covers the intended planning or execution path rather than a + nearby helper only. +- Remove redundant cases and boilerplate. A small set of clear cases is more + valuable than generated variations that do not add coverage. ## Running Unit Tests @@ -22,12 +75,22 @@ Integration tests are slower but cover a wide range of functionality. ```bash # Run unit and integration tests cargo test --features integration +``` + +## Running Benchmark Tests +These tests are slower but provide good coverage, prefer using them just at the +latest stages of the development cycle, as they are slow. + +```bash # Run TPCH integration tests cargo test --features tpch # Run TPC-DS integration tests -cargo test --features tpc-ds +cargo test --features tpcds + +# Run ClickBench integration tests +cargo test --features clickbench ``` ## Resources diff --git a/docs/source/contributor-guide/04-benchmarks.md b/docs/source/contributor-guide/04-benchmarks.md index e32688ca..63dd6e83 100644 --- a/docs/source/contributor-guide/04-benchmarks.md +++ b/docs/source/contributor-guide/04-benchmarks.md @@ -1,10 +1,24 @@ # Benchmarks -There are two kinds of benchmarks in this project. +Use this page to run existing benchmarks, qualify performance improvements, and +write new benchmarks. + +## Micro benchmarks + +When contributing performance improvements to very specific pieces of code, +prefer using the existing micro-benches in `benchmarks/benches`. + +If there's no benchmark that exercises the code path you are aiming to +improve, consider creating a separate PR adding the necessary benchmark +first, following the same pattern as the existing ones. + +The structure of new benchmarks should be unsurprising and consistent with +existing ones. ## Local Benchmarks -It's recommended to run these benchmarks locally when contributing to ensure there are no performance regressions. +It's recommended to run these benchmarks locally when contributing to ensure +there are no performance regressions. ### Generating Test Data @@ -19,23 +33,51 @@ This might take a while. ### Running Benchmarks -After generating the data, it's recommended to use the `run.sh` script to run the benchmarks. -A good setup is to run 8 workers throttled at 2 physical threads per worker. This provides a relatively -accurate benchmarking environment for a distributed system locally. +After generating the data, it's recommended to use the `run.sh` script to run +the benchmarks. A good setup is to run 8 workers throttled at 2 physical threads +per worker. This provides a relatively accurate benchmarking environment for a +distributed system locally. ```bash WORKERS=8 ./benchmarks/run.sh --threads 2 --dataset tpch_sf10 ``` -Subsequent runs will compare results against the previous one, so a useful trick to measure the impact of a PR -is to first run the benchmarks on `main`, and then on the PR branch. +Subsequent runs will compare results against the previous one, so a useful trick +to measure the impact of a PR is to first run the benchmarks on `main`, and then +on the PR branch. -More information about these benchmarks can be found in the [benchmarks README](https://github.com/datafusion-contrib/datafusion-distributed/blob/main/benchmarks/README.md). +More information about these benchmarks can be found in +the [benchmarks README](https://github.com/datafusion-contrib/datafusion-distributed/blob/main/benchmarks/README.md). ## Remote Benchmarks -These benchmarks run on a remote EC2 cluster against parquet files stored in S3. These are the most realistic -benchmarks, but also the most expensive to run in terms of development iteration cycles (it requires AWS CDK deploys for -every code change) and cost, as it uses a real EC2 cluster. +These benchmarks run on a remote EC2 cluster against parquet files stored in S3. +These are the most realistic benchmarks, but also the most expensive to run in +terms of development iteration cycles (it requires AWS CDK deploys for every +code change) and cost, as it uses a real EC2 cluster. + +For running these benchmarks, refer to +the [CDK benchmarks README](https://github.com/datafusion-contrib/datafusion-distributed/blob/main/benchmarks/cdk/README.md). + +Always prefer this type of benchmarks VS the local ones. + +## Qualifying performance improvements + +A performance claim needs reproducible, representative evidence. A faster +microbenchmark alone is not sufficient evidence for a cross-cutting production +change. + +Most benchmarks in this project are based on real data, and therefore, the +variability of results is high. When reporting benchmarks results over TPC-H, +TPC-DS or ClickBench data, keep into account that certain queries can easily +show variabilities of +-50% just because of network noise when reading files +from S3. + +The single most important value is the total wall time of the full suite, +not individual query results. For example a +100% improvement in one +specific query is meaningless if the full test suite did just a +1% improvement. -For running these benchmarks, refer to the [CDK benchmarks README](https://github.com/datafusion-contrib/datafusion-distributed/blob/main/benchmarks/cdk/README.md). +Do not overindex in individual query results reported by a single benchmark +run, when in doubt, re-run that specific query with a high number of +iterations (e.g., -i 20), in `main` VS the new branch, and then evaluate if +indeed there was an issue there. \ No newline at end of file diff --git a/docs/source/contributor-guide/05-code-review.md b/docs/source/contributor-guide/05-code-review.md new file mode 100644 index 00000000..b364df2a --- /dev/null +++ b/docs/source/contributor-guide/05-code-review.md @@ -0,0 +1,105 @@ +# Code review + +Review changes for user-visible correctness and maintainability. Formatting and +common lint issues belong to CI; a review should focus on risks that those +checks cannot identify. + +## Review workflow + +First, classify the intent of the pull request. It can fall into several +categories, and the review workflow changes depending on the category: + +- Feature additions (PR prefixed with `feat:`) +- Bug fixes (PR prefixed with `fix:`) +- Refactors (PR prefixed with `refactor:`) +- Docs and examples (PR prefixed with `docs:`) +- Performance (PR prefixed with `perf:`) + +## Feature additions + +These PRs add a new capability or address an existing limitation. The proposed +feature needs to be clearly stated, and a PR should not mix multiple features +without good justification. + +These PRs are typically large, so it is recommended to split preparatory work +into an isolated PR. Preparatory refactors and other contributions that unlock +the intended feature are better contributed as preliminary PRs. + +When reviewing feature additions, reviewers need to weigh the amount of +complexity added to the codebase against the value of the feature. A feature +whose benefit is unclear but introduces substantial complexity should not be +accepted, and the author should be prompted to find a simpler solution. + +Some requirements for these types of PRs: + +- The PR needs to clearly state the new feature in the PR description. +- A PR should not mix multiple features; split it instead. +- The new feature must be covered by new integration tests and, optionally, + small, scoped unit tests for finer-grained pieces. Tests need to adhere to the + testing guidelines in [tests.md](./03-tests.md). +- Any additions to the public API need to be documented properly. +- The new capabilities need to be documented, and an example needs to be added. + +## Bug fixes + +The change should be accompanied by a test that fails on `main` but succeeds on +the PR. + +The bug fix needs to be scoped. When fixing several unrelated things, prefer +opening separate PRs. + +The added tests need to adhere to the test guidelines in +[tests.md](./03-tests.md). + +## Refactors + +These PRs can range from large and mechanical to small and scoped. + +The most important thing to verify is that these PRs should either maintain or +remove complexity from the codebase, not introduce it. For example, if a PR is +net-zero in terms of features and fixes but adds a lot of LOC that are not +examples or documentation, that is a red flag for a bad refactor. + +For refactors that move large chunks of code around, the git diff might not be +very helpful, so it's important to verify manually that the actual chunks of +code did not change, or if they did, to make sure the changes are harmless. + +There is no need for refactors to be covered by new tests. Existing tests are +assumed to catch regressions, as long as the refactor does not silently +introduce new behavior. + +These types of PRs are better isolated and scoped, and should not be mixed with +feature additions or other categories of PRs. + +## Docs and Examples + +Docs should be brief and concise, reducing AI slop as much as possible and going +straight to the point. + +Prefer real code samples for explaining concepts, and ASCII drawings for more +complex explanations. + +Examples should be brief; ideally, a whole executable example should be under +300 lines of code. Runnable examples should always be accompanied by a +co-located `.md` file (see existing examples). + +## Performance + +The PR should clearly show improvements in existing benchmarks, taking into +account the guidelines from [benchmarks.md](./04-benchmarks.md). + +Complexity introduced by a PR needs to be clearly justified. A PR that +introduces a large amount of code just for the sake of a small performance +improvement should be questioned. Always weigh complexity against real +performance improvements; typically, a PR that improves performance should not +be very large (under 300 LOC of added code). + +When reviewing these PRs, the first step is to reproduce the claimed performance +improvements yourself. Only after this is verified can the actual code review +begin. + +The PR description should show clear steps for reproducing the performance +improvement. + +If there are no current benchmarks that exercise the claimed performance +improvement, a preliminary PR adding those benchmarks may be appropriate. diff --git a/docs/source/index.rst b/docs/source/index.rst index 5d642832..bdded9dc 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -108,3 +108,4 @@ side of the screen for the answer. contributor-guide/02-setup contributor-guide/03-tests contributor-guide/04-benchmarks + contributor-guide/05-code-review diff --git a/src/distributed_planner/AGENTS.md b/src/distributed_planner/AGENTS.md new file mode 100644 index 00000000..74e57a70 --- /dev/null +++ b/src/distributed_planner/AGENTS.md @@ -0,0 +1,36 @@ +# Distributed planner guide + +This guide applies only to `src/distributed_planner/`. It supplements the +repository-root `AGENTS.md`; do not repeat repository-wide conventions here. + +## Scope + +This module converts DataFusion physical plans into distributed plans. It owns +network-boundary insertion, distribution-oriented rewrites, distributed planner +configuration, and planning statistics. + +## Guidelines + +While contributing to this part of the codebase, the following should be taken +into account: + +### No topology modifications inside [inject_network_boundaries.rs](./inject_network_boundaries.rs) + +The `inject_network_boundaries` should only be in charge of injecting +network boundaries, it should never change the shape or topology of the plan, +and it should never perform replacement of some nodes by their distributed +variants. + +Any plan that reaches here should be a valid executable single-node plan. + +### Preparatory modifications to the single-node plan + +Sometimes, the single-node plan needs to be prepared for distributed execution +before injecting network boundaries. + +For example, inserting `BroadcastExec` nodes under the build side of +`CollectLeft` joins. + +All these preparatory modifications of the plan should still leave a perfectly +executable single-node plan, there should be no undesired intermediate +unexecutable states. diff --git a/src/distributed_planner/CLAUDE.md b/src/distributed_planner/CLAUDE.md new file mode 100644 index 00000000..43c994c2 --- /dev/null +++ b/src/distributed_planner/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/src/events/AGENTS.md b/src/events/AGENTS.md new file mode 100644 index 00000000..7f464595 --- /dev/null +++ b/src/events/AGENTS.md @@ -0,0 +1,23 @@ +# Event handlers guide + +This guide applies only to `src/events/`. It supplements the +repository-root `AGENTS.md`; do not repeat repository-wide conventions here. + +## Scope + +This module contains event handler specifications where users can wire up their +own behavior for reacting to certain events during the lifetime of a +distributed query. + +## Guidelines + +All event handlers here should follow the same consistent structure, and they +should be made in a way where extending them does not require breaking changes. + +All handlers contributed here must use the common tools shipped in +[common.rs](./common.rs) scoped to this module. + +This project ships some sane defaults under [defaults/](./defaults), when +contributing changes, prefer modeling them in terms of existing event +handler implementations, rather than inlining behavior in other parts of the +codebase. \ No newline at end of file diff --git a/src/events/CLAUDE.md b/src/events/CLAUDE.md new file mode 100644 index 00000000..43c994c2 --- /dev/null +++ b/src/events/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md