diff --git a/.github/workflows/analysis.yml b/.github/workflows/analysis.yml index a25262228..0112a5ef7 100644 --- a/.github/workflows/analysis.yml +++ b/.github/workflows/analysis.yml @@ -1,4 +1,4 @@ -name: Analysis +name: Coverage on: push: @@ -94,13 +94,13 @@ jobs: find . -type f -name "simplelogger.*" -exec rm -fv '{}' \; mvn -q --no-transfer-progress --batch-mode -DclickhouseVersion=$PREFERRED_LTS_VERSION \ -DskipTests install - - name: Analyze + - name: Generate coverage report env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Needed to get PR information, if any SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} run: | mvn -fn --no-transfer-progress --batch-mode -DclickhouseVersion=$PREFERRED_LTS_VERSION \ - -Panalysis verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar -Dsonar.projectKey=ClickHouse_clickhouse-java + -Pcoverage verify org.sonarsource.scanner.maven:sonar-maven-plugin:sonar -Dsonar.projectKey=ClickHouse_clickhouse-java continue-on-error: true - name: Generate and post coverage report env: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index da2d9ef53..ab53f1928 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,125 +1,226 @@ -## 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. +# Contribution Guide + +## Preface + +Libraries in this repository are used in many production systems. Users expect certain level of quality and active support (adding new features, fixing issues). +It is important to make all changes safe and swiftly. There we ask all contributors for tests and clear description of changes done in a PR. This expedites merge and +quadruples value of your contribution. + +This document covers main important things related to the contributing. Here is very brief overview of them: +- (Contribution Process)[#contribution-process] - describes how contribution is handled and what steps we expect. Specially pay attention if you want to implement +any feature or do big change. +- (Testing Expectations)[#testing-expectations] - test is requirement for mostly any change and tests have their quality measure, too. We value failure scenario tests +and when tests cover veriaty of data. Here is where help mostly needed because every use-case or domain specific data examples significantly increase test quality. +- (Pull Request Checklist)[#pull-request-checklist] - helps to prepare PR for review and make whole process faster. It is very important to run tests locally - we skip +PRs if CI is failing and will leave according comment. +- (Restricted Changes)[#restricted-changes] - currently we restrict only changes in CI workflows and everything related to it. +- (Technical Verification)[#technical-verification] - all kinds of instractions to run tests, examples and verifications. + + +## 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 + +Please review our (AI Policy)[AI_POLICY.MD] if you are using AI tools. Special attention should be paid to tests because they are the main guardrails for code changes. + +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 + +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. + +We all know that large PRs (400+ lines of code) are hard to review and it is more likely to miss a bug. Small changes are appreciated and take less time to verify them. If many changes are needed anyway (feature implementation, refactoring), please split them: something can be done as code preparation, cleanup or smaller improvement. It should take less time at sum as this repository is actively maintained. We will ask to split changes of 800+ LOC anyway. + +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 self and AI review use following guids: +- `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 + +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. + +Steps: +- Describe all changes in PR briefly so every human can read. There can be additional description appended to the main description. + - Include link to an issue. If issue is missing please create one. + - Describe any user-visible behavior especially if it was changed + - Include compatibility impact +- Run self-review of the code (personally or by AI). This reduces PR time. +- Run tests locally. IMPORTANT: We skip PR that has failed CI and ask to fix it. +- Update `CHANGELOG.md` shortly with what was the problem and how fixed. Add link to the issue. +- Update `docs/features.md` when `client-v2` or `jdbc-v2` feature was added, removed, or intentionally behaviour change. This file helps to review code. + +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 - - - - jdk - - 17 - - - /usr/lib/jvm/java-17-openjdk - - - - ``` - -### 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 + + + + jdk + + 17 + + + /usr/lib/jvm/java-17-openjdk + + + +``` + +### 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 test +mvn -pl -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 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=: + +# 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. Run Coverage Verification + +To run tests and generate a code coverage report, use the `coverage` Maven profile. This will execute tests and produce JaCoCo coverage reports. + +```bash +mvn clean verify -P coverage +``` + +For targeted module verification: + +```bash +mvn -pl -am clean verify -P coverage +``` + +The coverage reports will be generated in the `target/site/jacoco-aggregate` or module-specific `target/site/jacoco-ut` directories. You can view the HTML version of the coverage report by opening the `index.html` file in your browser: + +- Full repository report: `target/site/jacoco-aggregate/index.html` +- Module-specific report: `/target/site/jacoco-ut/index.html` + +### 6. 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] [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=: - - # 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 +``` + +### 7. 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. diff --git a/pom.xml b/pom.xml index 4a0b030b4..69f35d6c5 100644 --- a/pom.xml +++ b/pom.xml @@ -626,7 +626,7 @@ - analysis + coverage