Skip to content

[repo] Contribution guide #2859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chernser wants to merge 1 commit into
base: main
Choose a base branch
from
+176 −107
Open

[repo] Contribution guide #2859

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
283 changes: 176 additions & 107 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,125 +1,194 @@
## Getting started
ClickHouse-JDBC client is an open-source project, and we welcome any contributions from the community. Please share your ideas, contribute to the codebase, and help us maintain up-to-date documentation.
# Contributing to ClickHouse Java
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the guidance provided in the document (Testing, Contribution Process, significant contributions, and the PR checklist) is also relevant to AI coding agents. Considering the significant portion of our contributions made by agents, we should either mirror these rules into AGENTS.md or extract them into a shared file that both documents reference.
This can be done in a separate PR.


Contributions are always welcome: issues, pull requests, questions, ideas, tests, documentation, and examples all help the project.

ClickHouse Java is maintained by a team responsible for the stability and quality of released artifacts. Because this repository publishes client libraries and drivers used by applications, every contribution needs enough context and validation for maintainers to review its safety. At minimum, a code change should include a clear description and tests that prove the intended behavior.
Copy link
Copy Markdown
Member

@mshustov mshustov May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every contribution needs enough context and validation for maintainers to review its safety

I'd suggest rephrase to something > here's what makes them easy to review, otherwise, it reads too defensively

Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this repository publishes client libraries and drivers used by applications, every contribution needs enough context and validation for maintainers to review its safety.

I'd even strengthen it

The libraries are foundational infrastructure for the software built on top of them, thus a regression or breaking change in a client propagates to applications that depends on it, often with limited ability for users to work around the issue


## Contribution Process

### Issues, Discussion, and Proposals

Small changes do not require an issue or implementation proposal. A clear pull request description and tests are enough, although an issue is appreciated when it provides useful context.

Please open an issue before starting work on a new feature, a large behavior change, or a change that affects public API, configuration, protocol handling, serialization, JDBC/R2DBC behavior, or documented features.

Feature work and big changes must be discussed through an issue, and the implementation proposal should be approved before code changes begin. The proposal does not need to be perfect, but it should describe:

- what problem is being solved
- what user-visible behavior will change
- what compatibility risks exist
- what tests will prove the behavior
- whether documentation, `CHANGELOG.md`, or `docs/features.md` needs to change

It is fine to use AI tools to draft an implementation proposal, explore design options, or prepare a first implementation. AI-generated code is welcome when the specification is detailed, the resulting code follows project patterns, and the tests clearly validate the expected behavior. Special attention should be paid to tests because they are the main guardrails for code changes.
Copy link
Copy Markdown
Member

@mshustov mshustov May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-generated code is welcome when

should we add AI_POLICY.md and link it here?


Some good ideas need time to design and implement. We are open to discussion and welcome collaboration in issues, even when the final implementation is not ready yet.

### Big Contributions
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One logical change per PR: a feature, a bug fix, a refactor, or a doc update. Mixed-purpose PRs complicate reviews and increase the likelihood of unintended regressions.


Large changes are easier to review and merge when they are split into a few smaller pull requests. From our experience, it is often better to prepare the codebase first with focused cleanup, refactoring, or test-only PRs, and then add the feature or behavior change in a later PR.
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The principle is right but the guidance doesn't have an easy to follow definition. Let's give contributors a concrete threshold. For example (numbers are taken from this study https://www.propelcode.ai/blog/pr-size-impact-code-review-quality-data-study):

As a rough guideline, if a pull request is over 300–400 lines of code (excluding docs and tests), consider whether it can be split. Smaller PRs are reviewed and merged faster, and the savings in review time usually outweigh the overhead of splitting


Avoid packing unrelated cleanup, infrastructure changes, test rewrites, and the feature itself into one large pull request. Smaller PRs help maintainers verify intent, reduce review time, and lower the risk of regressions.
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a new feature, implement the smallest change that solves the problem. Polish, additional configuration, optimization, and extensions can be addressed in the follow-up PRs


For significant changes, use the review and design guidance in:

- `docs/ai-review.md`
- `docs/changes_checklist.md`
- `docs/highlevel-changes-checklist.md`
- `docs/features.md` for changes that touch `client-v2` or `jdbc-v2`

### Testing Expectations

Tests should verify correct behavior, not only reproduce a specific issue. An issue may describe a symptom, but it does not always define the complete correct behavior. A good test should make the expected success or failure path clear to future maintainers.

Please include negative tests when a change fixes invalid input, error handling, validation, parsing, serialization, or compatibility-sensitive behavior. The test should prove that the failure path produces the correct result or exception, not only that the original bug no longer happens.

Test code is as important as production code. Keep tests compact, readable, and focused on the scenario being verified. Avoid repeating existing coverage unless the new test adds a distinct scenario, edge case, module, type, format, or failure mode. Reduce duplication in test setup and assertions so reviewers can quickly see what behavior is being tested.

We measure test coverage, but coverage percentage is not the only goal. We value tests that cover meaningful scenarios and edge cases. For example, a test that reads a random large integer is useful, but a stronger test also checks boundary values, rounding behavior, invalid values, and the interaction with nullable or nested types when relevant.

### Restricted Changes

Changes to CI workflows are currently restricted. Please discuss CI workflow changes with maintainers before opening a pull request.

## Pull Request Checklist
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a couple of things to set clearer expectations on both sides:

  • CI must be runnable on the PR. For external contributors, the initial CI run requires approval from a maintainer (as of today, probably should be revisited). Let’s explicitly mention this and commit to triggering it promptly to avoid contributors being stuck waiting. For internal PRs, the CI suite should actually run against the proposed change before the review process begins. If tests are scoped down, skipped, or disabled, the PR should clearly indicate this.

  • Reviews occur when CI is green. Clearly state that the team will review the code once CI passes. Additionally, contributors should either fix PRs with failing CI or move them to the draft. This approach is fair in both directions: contributors have a clear understanding of what means ready for review, and reviewers avoid spending cycles on changes that are not ready


Please make pull requests review-ready before requesting review. A PR may still take time to review and prepare for merge, especially when it changes public behavior or touches multiple modules.

Every pull request should include:

- a clear description of what changed and why
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- a clear description of what changed and why
- a clear description of what changed, why and why now

- a link to the related issue, when one exists
- an explanation of the user-visible behavior and compatibility impact
- tests for the changed behavior, including integration tests when the change interacts with ClickHouse server behavior
- the local test commands that were run
Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need that level of noize in every PR. I'd just set expectations: you run test before opening a PR

Copy link
Copy Markdown
Member

@mshustov mshustov May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • run the project's code review skill against your diff before requesting human review

this and run tests locally can be moved to before you open a PR

- a `CHANGELOG.md` update for user-visible changes
- a `docs/features.md` update when a `client-v2` or `jdbc-v2` feature is added, removed, or intentionally changed
- documentation or examples when the change affects user-facing behavior

Use `docs/review-template.md` as a reference for what reviewers will look for when assessing important changes.

TBD: add a dockerized development environment section that documents a standard way to run the full local verification suite.

## Technical Verification

Before submitting a pull request, verify the affected modules locally. Prefer targeted Maven commands over full-repository runs when the change is localized.

### 1. Create a Fork and Clone It

### Create a fork of the repository and clone it
```bash
git clone https://github.com/[YOUR_USERNAME]/clickhouse-java
cd clickhouse-java
```

### Set up environment
You have installed:
- JDK 8 or JDK 17+
- To build a multi-release jar use JDK 17+ with `~/.m2/toolchains.xml`
```xml
<?xml version="1.0" encoding="UTF8"?>
<toolchains>
<toolchain>
<type>jdk</type>
<provides>
<version>17</version>
</provides>
<configuration>
<jdkHome>/usr/lib/jvm/java-17-openjdk</jdkHome>
</configuration>
</toolchain>
</toolchains>
```

### Build modules
- JDK 8 Use `mvn -Dj8 -DskipITs clean verify` to compile and generate packages.
- JDK 17+ Use create a multi-release jar (see [JEP-238](https://openjdk.java.net/jeps/238)) please verify that you added `~/.m2/toolchains.xml` and run `mvn -DskipITs clean verify`


To create a native binary of JDBC driver for evaluation and testing:
### 2. Set Up the Environment

Install JDK 8 or JDK 17+.

To build a multi-release jar with JDK 17+, configure `~/.m2/toolchains.xml`:

```xml
<?xml version="1.0" encoding="UTF8"?>
<toolchains>
<toolchain>
<type>jdk</type>
<provides>
<version>17</version>
</provides>
<configuration>
<jdkHome>/usr/lib/jvm/java-17-openjdk</jdkHome>
</configuration>
</toolchain>
</toolchains>
```

### 3. Build and Compile

Use the command that matches your environment:

```bash
# JDK 8
mvn -Dj8 -DskipITs clean verify

# JDK 17+ with toolchains.xml configured
mvn -DskipITs clean verify
```

For targeted module verification, use:

```bash
mvn -pl <module> test
mvn -pl <module> -am test
```

Compile examples or packaging modules when your change affects examples, packaging, public APIs, or user-facing behavior.

### 4. Run Unit and Integration Tests

Unit tests do not require a running ClickHouse server. Relevant unit tests should pass locally before a PR is submitted.

Integration tests usually require [Docker](https://docs.docker.com/engine/install/). The Docker image defaults to `clickhouse/clickhouse-server`, and containers are created automatically by [testcontainers](https://www.testcontainers.org/). To test against a specific ClickHouse version, pass a Maven parameter such as:

```bash
mvn -pl <module> test -DclickhouseVersion=23.3
```

If you do not want to use Docker, or you prefer to test against an existing server:

- make sure the server can be accessed with the default account, user `default` and no password, with both DDL and DML privileges
- add the test server configuration files and expose all default ports:
- [ports.xml](clickhouse-client/src/test/resources/containers/clickhouse-server/config.d/ports.xml)
- [users.xml](clickhouse-client/src/test/resources/containers/clickhouse-server/users.d/users.xml)
- make sure the ClickHouse binary, usually `/usr/bin/clickhouse`, is available in `PATH` for `clickhouse-cli-client` tests
- put `test.properties` under either `~/.clickhouse` or the module's `src/test/resources`

Example `test.properties`:

```properties
# ClickHouse server for integration tests
clickhouseServer=x.x.x.x

# Custom HTTP proxy for integration tests
proxyAddress=<host>:<port>

# Properties below are only useful for testcontainers
#clickhouseVersion=latest
#clickhouseTimezone=UTC
#clickhouseImage=clickhouse/clickhouse-server
#additionalPackages=
#proxyImage=ghcr.io/shopify/toxiproxy:2.5.0
```

TBD: document a dockerized development environment for running the standard local test suite.

### 5. Optional Native Binary Check

To create a native binary of the JDBC driver for evaluation and testing:

- [install GraalVM](https://www.graalvm.org/latest/docs/getting-started/) and optionally [upx](https://upx.github.io/)
- make sure [native-image](https://www.graalvm.org/latest/docs/getting-started/#native-image) is installed
- build and run the native binary

- make sure you have [native-image](https://www.graalvm.org/latest/docs/getting-started/#native-image) installed, and then build the native binary

```bash
cd clickhouse-java
mvn -DskipTests clean install
cd clickhouse-jdbc
mvn -DskipTests -Pnative clean package
# only if you have upx installed
upx -7 -k target/clickhouse-jdbc-bin
```

- run native binary

```bash
# print usage
./target/clickhouse-jdbc-bin
Usage: clickhouse-jdbc-bin [PROPERTIES] <URL> [QUERY] [FILE]
...

# test database connection using JDBC driver
./target/clickhouse-jdbc-bin -Dverbose=true 'jdbc:ch:http://localhost'
Arguments:
- url=jdbc:ch:http://localhost
- query=select 500000000
- file=jdbc.out

Options: action=read, batch=1000, samples=500000000, serde=true, type=, verbose=true
Processed 1 rows in 85.56 ms (11.69 rows/s)

# test query performance using Java client
./target/clickhouse-jdbc-bin -Dverbose=true -Dtype=uint64 'http://localhost'
...
Processed 500,000,000 rows in 52,491.38 ms (9,525,373.89 rows/s)

# test same query again using JVM for comparison - don't have GraalVM EE so JIT wins in my case
java -Dverbose=true -Dtype=uint64 -jar target/clickhouse-jdbc-*-http.jar 'http://localhost'
...
Processed 500,000,000 rows in 25,267.89 ms (19,787,963.94 rows/s)
```

## Testing

By default, [docker](https://docs.docker.com/engine/install/) is required to run integration test. Docker image(defaults to `clickhouse/clickhouse-server`) will be pulled from Internet, and containers will be created automatically by [testcontainers](https://www.testcontainers.org/) before testing. To test against specific version of ClickHouse, you can pass parameter like `-DclickhouseVersion=23.3` to Maven.

In the case you don't want to use docker and/or prefer to test against an existing server, please follow instructions below:

- make sure the server can be accessed using default account(user `default` and no password), which has both DDL and DML privileges
- add below two configuration files to the existing server and expose all defaults ports for external access
- [ports.xml](../../blob/main/clickhouse-client/src/test/resources/containers/clickhouse-server/config.d/ports.xml) - enable all ports
- and [users.xml](../../blob/main/clickhouse-client/src/test/resources/containers/clickhouse-server/users.d/users.xml) - accounts used for integration test
- make sure ClickHouse binary(usually `/usr/bin/clickhouse`) is available in PATH, as it's required to test `clickhouse-cli-client`
- put `test.properties` under either `~/.clickhouse` or `src/test/resources` of your project, with content like below:
```properties
# ClickHouse server for integration test
clickhouseServer=x.x.x.x
# custom HTTP proxy for integration test
proxyAddress=<host>:<port>

# below properties are only useful for test containers
#clickhouseVersion=latest
#clickhouseTimezone=UTC
#clickhouseImage=clickhouse/clickhouse-server
#additionalPackages=
#proxyImage=ghcr.io/shopify/toxiproxy:2.5.0
```

### Tooling
We use [TestNG](https://testng.org/) as testing framework and for running ClickHouse Local instance [testcontainers](https://www.testcontainers.org/modules/databases/clickhouse/).

### Running unit tests

Does not require a running ClickHouse server.
Running the maven commands above will trigger the test.

## Benchmark

To benchmark JDBC drivers:
```bash
cd clickhouse-java
mvn -DskipTests clean install
cd clickhouse-jdbc
mvn -DskipTests -Pnative clean package

# print usage
./target/clickhouse-jdbc-bin
```

### 6. Optional Benchmark Check

Run benchmarks when the change may affect performance:

```bash
cd clickhouse-benchmark
mvn clean package

# single thread mode
java -DdbHost=localhost -jar target/benchmarks.jar -t 1 \
-p client=clickhouse-jdbc -p connection=reuse \
-p statement=prepared -p type=default Query.selectInt8
```

It's time consuming to run all benchmarks against all drivers using different parameters for comparison. If you just need some numbers to understand performance, please refer to [this](https://github.com/ClickHouse/clickhouse-java/issues/768) and watch [this](https://github.com/ClickHouse/clickhouse-java/issues/928) for more information and updates.