Draft
Conversation
slabko
force-pushed
the
time-time64-support
branch
7 times, most recently
from
d44abd4 to
cc78927
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class support for ClickHouse Time / Time64 types in the C++ client library, including type parsing, column implementations, ItemView validation/printing, and unit/integration tests to verify roundtrip + client behavior.
Changes:
- Introduce
Type::Time/Type::Time64(incl.Time64Type) and extend the type parser to recognize both. - Add new columns
ColumnTimeandColumnTime64, wire them into the column factory, and update ItemView validation + test utilities output formatting. - Expand UT coverage (ItemView, column behavior, parsing, client roundtrip) and adjust CI workflows (ClickHouse image/secrets).
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ut/value_generators.h |
Adds MakeTime / MakeTime64 generators for new time-based tests. |
ut/value_generators.cpp |
Implements MakeTime() value set including positive/negative cases. |
ut/utils_ut.cpp |
Extends ItemView ostream serialization tests for Time/Time64. |
ut/utils.cpp |
Adds Time/Time64 formatting cases to ItemView ostream output. |
ut/type_parser_ut.cpp |
Adds parser tests for Time and Time64(precision). |
ut/roundtrip_column.cpp |
Enables server setting needed to roundtrip Time/Time64. |
ut/itemview_ut.cpp |
Adds ItemView type-size tests for Time and Time64. |
ut/columns_ut.cpp |
Adds column unit tests for appending/constructing ColumnTime/ColumnTime64. |
ut/client_ut.cpp |
Adds integration tests verifying insertion/selection and string rendering for Time/Time64. |
ut/Column_ut.cpp |
Adds generic column roundtrip test cases for Time/Time64 at multiple precisions. |
clickhouse/types/types.h |
Extends Type::Code and introduces Time64Type + factory methods. |
clickhouse/types/types.cpp |
Implements Time/Time64 naming and Time64Type validation + naming. |
clickhouse/types/type_parser.cpp |
Registers Time/Time64 keywords in the type-code map. |
clickhouse/columns/time.h |
New ColumnTime and ColumnTime64 public column APIs. |
clickhouse/columns/time.cpp |
Implements storage, IO, slicing, swapping, and ItemView exposure for time columns. |
clickhouse/columns/itemview.cpp |
Extends ItemView size validation for Time (4 bytes) and Time64 (8 bytes). |
clickhouse/columns/factory.cpp |
Wires Time/Time64 into CreateColumnByType terminal factory. |
clickhouse/client.h |
Exposes time columns via the public client header includes. |
clickhouse/CMakeLists.txt |
Adds columns/time.cpp to the library build sources. |
.github/workflows/windows_msvc.yml |
Updates CI secrets/host wiring for Windows MSVC runs. |
.github/workflows/windows_mingw.yml |
Updates CI secrets/host wiring for Windows MinGW runs. |
.github/workflows/macos.yml |
Updates CI secrets/host wiring for macOS runs. |
.github/workflows/linux.yml |
Updates ClickHouse server image and container startup options for Linux CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+111
to
+121
| template <size_t Precision> | ||
| inline std::vector<int64_t> MakeTime64() { | ||
| std::vector<int64_t> ret{}; | ||
| auto time32 = MakeTime(); | ||
| int64_t exp = 1; | ||
| for (size_t i = 0; i < Precision; ++i) { | ||
| exp *= 10; | ||
| } | ||
| std::transform(time32.cbegin(), time32.cend(), std::back_inserter(ret), [exp](int32_t x) { | ||
| return x * exp; | ||
| }); |
Comment on lines
21
to
25
| columns/numeric.cpp | ||
| columns/map.cpp | ||
| columns/string.cpp | ||
| columns/time.cpp | ||
| columns/tuple.cpp |
clickhouse/columns/factory.cpp
Outdated
Comment on lines
+116
to
+119
| throw AssertionError("Time64 type without precision"); | ||
| } else { | ||
| return std::make_shared<ColumnTime64>(GetASTChildElement(ast, 0).value); | ||
| } |
ut/columns_ut.cpp
Outdated
Comment on lines
+182
to
+187
| TEST(ColumnsCase, Time_UInt32_construct_from_rvalue_data) { | ||
| auto const expected = MakeNumbers<int32_t>(); | ||
|
|
||
| auto data = expected; | ||
| auto col = std::make_shared<ColumnTime>(std::move(data)); | ||
|
|
ut/columns_ut.cpp
Outdated
Comment on lines
+203
to
+208
| TEST(ColumnsCase, Time64_UInt32_construct_from_rvalue_data) { | ||
| auto const expected = MakeNumbers<int64_t>(); | ||
|
|
||
| auto data = expected; | ||
| auto col1 = std::make_shared<ColumnTime64>(3, std::move(data)); | ||
|
|
slabko
force-pushed
the
time-time64-support
branch
from
cc78927 to
45fcc5e
Compare
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.
No description provided.