Add guidlines for AI contributions and reviews

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.

CLAUDE.md

@@ -0,0 +1 @@
+@AGENTS.md

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

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

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.

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.

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