Skip to content

Instant / DateTime64 via parameter #2501

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

Jump to bottom
Merged
chernser merged 5 commits into from
Jul 23, 2025
Merged

Instant / DateTime64 via parameter #2501

chernser merged 5 commits into from
Jul 23, 2025

Conversation

@enqueue
Copy link
Contributor

@enqueue enqueue commented Jul 22, 2025 •
edited
Loading

Summary

This PR adds support for supplying a java.time.Instant as query parameter to a query. The driver will take care to translate the Java Instant into a format ClickHouse server will understand. In some use cases, user will have to perform the mapping themself.

This is only the start into a great journey. As outlined in #2456 , clients will also expect this to work with Array, probably Tuple etc. as well. Nice candidates with time zone madness are also LocalDateTime and friends.

I decided against exposing individual mapping methods via API in DataTypeUtils, because they are subject to change. Instead, clients should be able to rely on default behavior.

This feels very similar to requirements from JDBC, where Java objects have to be serialized somehow (see PreparedStatementImpl.encodeObject(). We have to be cautious not to have different ways of serializing stuff for ClickHouse queries. N.b.: That method uses a dirty hack to dynamically invoke fromUnixTimestamp64Nano on server for all modern Java time data types which are time-zone based.

I did not know where to place the integration tests, really. I first started adding a method to QueryTests, but I reckoned that this class' primary purpose was to ensure the same behavior using different configuration options (compression etc.) via sub classes. I also considered DataTypeTests, but this class mainly deals with some complex, nested stuff. So I created a new test, com.clickhouse.client.e2e.ParameterizedQueryTest. Please let me know if you see a better place for this integration test.

The format methods in DataTypeUtils create an empty String for null values. This might not always be the best default value for every ClickHouse data type. There are different options:

  1. Throw NPE or IAE if object is null
  2. Provide default String values per data type, either directly in ClickHouseDataType or here, in the DataTypeUils class.

Please suggest the most practical way you would expect the API to work.

Some more general remarks:

  1. There are many Java warnings which are correct, e.g. about unused imports, unused fields etc. It would really be great if we could improve code quality a little bit (related Contributing Guidelines / Code Style #304).
  2. I was struggling with Java 8 target. There are so many API improvements, language features, bug fixes we are missing out on. Do maintainers have a sound strategy for Java version targets?
  3. Then, on the other hand, there is a dependency on lombok library. This is not very practical because you have to set up your IDE to pre-compile this stuff etc. Either stick to "old-school" Java, let IDE generate getters/setters for you or go current Java standard, records.
  4. Some tests are mixing JUnit assertions with TestNG stuff. This should not happen for sake of consistency.

Closes #2456

Checklist

Delete items not relevant to your PR:

@enqueue enqueue marked this pull request as ready for review July 22, 2025 16:30
@enqueue enqueue marked this pull request as draft July 22, 2025 17:11
@enqueue
Copy link
Contributor Author

enqueue commented Jul 22, 2025 •
edited
Loading

I added a test for unusual String parameters which fails for multi-line Strings and quoted Strings.

I had to disable the tests because I was not able to make this work.

I think this is related to query parameter parsing downstream, see issues ClickHouse/ClickHouse#70240 and ClickHouse/ClickHouse#69656

@enqueue enqueue marked this pull request as ready for review July 22, 2025 17:41
@chernser
Copy link
Contributor

chernser commented Jul 23, 2025

Good day, @enqueue !
Thank you very much for the contribution - I will try review it asap

Here are some answer to the general remarks :

  • Yes, we will do cleanup of warning. We could use spotless plugin to clean some of them or at list fail build.
  • Java 8 problem roots from some old library that are using our client. We will phase out java 8 soon. Meanwhile we may be start providing multi release builds for components where using newer java make more performance and development benefits.
  • Lombok is used in tests only and only in one place. We do not plan using such things in production code. Sorry for inconvenience - we will think about removing it later.

The format methods in DataTypeUtils create an empty String for null values. This might not always be the best default value for every ClickHouse data type.
If there are two many uncertainties then it is safe to be protective and throw NPE. We should not guess.

As for integration tests - we mostly test against real DB and just mark tests with integration group. I suggest to more com.clickhouse.client.e2e.ParameterizedQueryTest to com.clickhouse.client package.

chernser
chernser reviewed Jul 23, 2025
View reviewed changes
@chernser chernser requested review from Copilot and mzitnik July 23, 2025 04:11
@chernser
Copy link
Contributor

chernser commented Jul 23, 2025

@enqueue please see my comments.
Thanks!

Copilot AI reviewed Jul 23, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for using java.time.Instant as query parameters in ClickHouse queries. The driver automatically converts Java Instant objects into appropriate ClickHouse DateTime/DateTime64 formats, with configurable time zone handling for Date types.

  • Implements DataTypeUtils.format() methods to convert Instant objects to ClickHouse-compatible string formats
  • Adds query parameter formatting in the Client class to handle object-to-string conversion
  • Includes comprehensive integration and unit tests for datetime parameter handling

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
ParameterizedQueryTest.java New integration test class for parameterized queries with datetime types
BinaryStreamReaderTests.java Added datetime serialization/deserialization tests with timezone handling
ClickHouseBinaryFormatReaderTest.java Updated test assertions to remove redundant type casting
DataTypeUtilsTests.java Added comprehensive unit tests for Instant formatting methods
HttpAPIClientHelper.java Refactored query parameter handling with constant for statement params key
SerializerUtils.java Added support for OffsetDateTime serialization and cleaned up imports
BinaryStreamReader.java Simplified datetime reading logic and removed redundant type casting
RowBinaryFormatSerializer.java Fixed timezone handling in date serialization
DataTypeUtils.java Core implementation of Instant formatting for various ClickHouse data types
Client.java Added query parameter formatting and deprecated method annotation
Comments suppressed due to low confidence (1)

client-v2/src/test/java/com/clickhouse/client/api/DataTypeUtilsTests.java:16

  • Test class should be declared as 'public' to follow standard testing conventions and ensure proper test discovery
class DataTypeUtilsTests {

@chernser chernser merged commit e1d46ff into ClickHouse:main Jul 23, 2025
15 of 23 checks passed
@enqueue enqueue deleted the datetime64 branch July 23, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@chernser chernser chernser approved these changes

@mzitnik mzitnik Awaiting requested review from mzitnik

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Passing an Instant as a query parameter in client-v2 fails during parsing

2 participants

@enqueue @chernser