Add CH_NON_OPTIONAL_CURRENT_ENDPOINT and CH_USE_3X_API build options - #563
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The non-optional GetCurrentEndpoint() implementation currently dereferences a std::optional in a way that can be undefined behavior if the invariant is ever violated, and the public signature returns const Endpoint by value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces build-time API compatibility switches to ease migration toward a 3.x API surface, including an option to make Client::GetCurrentEndpoint() non-optional when configured.
Changes:
- Add
CH_NON_OPTIONAL_CURRENT_ENDPOINTto optionally exposeClient::GetCurrentEndpoint()as returningEndpoint(non-optional). - Add umbrella option
CH_USE_3X_APIin CMake to enable multiple 3.x-API-affecting flags together and fail on conflicting per-flag overrides. - Update tests/examples/docs and Bazel defines to align with the new option behavior.
File summaries
| File | Description |
|---|---|
ut/client_ut.cpp |
Normalizes GetCurrentEndpoint() usage in unit tests across optional vs non-optional builds. |
tests/simple/main.cpp |
Adjusts example output to compile under both return-type variants. |
README.md |
Documents CH_USE_3X_API and the implied per-flag settings. |
CMakeLists.txt |
Introduces CH_USE_3X_API, sets implied defaults, and enforces no-conflict configuration. |
clickhouse/CMakeLists.txt |
Propagates CH_NON_OPTIONAL_CURRENT_ENDPOINT as a public compile definition (0/1). |
clickhouse/client.h |
Conditionally changes Client::GetCurrentEndpoint() signature based on the macro. |
clickhouse/client.cpp |
Implements the conditional non-optional GetCurrentEndpoint() variant. |
BUILD.bazel |
Unconditionally enables CH_NON_OPTIONAL_CURRENT_ENDPOINT=1 for Bazel builds. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1416
to
+1418
| const Endpoint Client::GetCurrentEndpoint() const { | ||
| return *impl_->GetCurrentEndpoint(); | ||
| } |
Comment on lines
341
to
+347
| /// Get current endpoint, i.e. the last successfully connected endpoint. | ||
| /// It remains optional for backward compatibility, but now always contains a value. | ||
| #if CH_NON_OPTIONAL_CURRENT_ENDPOINT | ||
| const Endpoint GetCurrentEndpoint() const; | ||
| #else | ||
| const std::optional<Endpoint>& GetCurrentEndpoint() const; | ||
| #endif |
slabko
force-pushed
the
build-options-3x-api
branch
2 times, most recently
from
September 4, 2026 17:25
e14d4ce to
5188222
Compare
When enabled, Client::GetCurrentEndpoint() returns Endpoint by value instead of the legacy std::optional<Endpoint>. Defaults to OFF for backward compatibility. - Declare the CMake option and propagate it as a PUBLIC compile definition, mirroring CH_MAP_BOOL_TO_UINT8 / CH_USE_ABSEIL_FOR_BIGNUM - Enable the option unconditionally in BUILD.bazel (no 2.x compatibility promised there) and document the option in README - Fix missing Client:: qualifier on the by-value overload - Make unit tests and tests/simple compile under both modes
CH_USE_3X_API=ON selects the 3.x API in one step by defaulting CH_MAP_BOOL_TO_UINT8=OFF, CH_USE_ABSEIL_FOR_BIGNUM=OFF and CH_NON_OPTIONAL_CURRENT_ENDPOINT=ON. Setting any of these to a conflicting value alongside CH_USE_3X_API=ON is a configuration error. Update README to recommend CH_USE_3X_API and document the mapping between the umbrella option and the individual ones.
slabko
force-pushed
the
build-options-3x-api
branch
from
September 4, 2026 17:28
5188222 to
df3c321
Compare
chernser
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After #558,
Client::GetCurrentEndpoint()still returns an optional value, even though it is never set tonullopt. This PR changesClient::GetCurrentEndpoint()to return a non-optional value when theCH_NON_OPTIONAL_CURRENT_ENDPOINTconfiguration option is enabled.Additionally, since there are already a few flags that need to be enabled to use the new API, a new umbrella option,
CH_USE_3X_API, is added to enable all of these flags, as well as any future flags, at once.Note that
BUILD.bazeldefinesCH_NON_OPTIONAL_CURRENT_ENDPOINT=1unconditionally; the Bazel build does not promise 2.x compatibility.