[client-v2,jdbc-v2] Fix Writing Date/Time values #2710
Open
[client-v2,jdbc-v2] Fix Writing Date/Time values #2710
Conversation
chernser
changed the title
chernser
changed the title
…d write back with different timezone
Client V2 CoverageCoverage Report
Class Coverage
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
jdbc-v2/src/main/java/com/clickhouse/jdbc/WriterStatementImpl.java
Outdated
Show resolved
Hide resolved
| extCal3.clear(); | ||
| extCal3.set(localDateFromExternal.getYear(), localDateFromExternal.getMonthValue() - 1, localDateFromExternal.getDayOfMonth(), 0, 0, 0); | ||
| System.out.println("converted> " + extCal3.toInstant()); // wrong date!! | ||
| } |
There was a problem hiding this comment.
Test has debug output and no assertions
Low Severity
testDifferentDateConversions contains multiple System.out.println statements, a // wrong date!! comment, unused local variables (utcTz, applicationLocalTz), and no assertions. This looks like exploratory debug code that was accidentally included in the commit rather than a proper unit test.
jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java
Outdated
Show resolved
Hide resolved
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
| public void setDate(int parameterIndex, Date x, Calendar cal) throws SQLException { | ||
| ensureOpen(); | ||
| writer.setValue(parameterIndex, sqlDateToInstant(x, cal)); | ||
| writer.setValue(parameterIndex, DataTypeUtils.toLocalDate(x, cal.getTimeZone())); |
There was a problem hiding this comment.
Null Calendar causes NPE in WriterStatementImpl set methods
High Severity
setDate, setTime, and setTimestamp in WriterStatementImpl directly call cal.getTimeZone() without a null check, causing a NullPointerException when cal is null. The sibling class PreparedStatementImpl correctly guards against this with (cal == null ? defaultCalendar : cal).getTimeZone(). These are public JDBC API methods where null Calendar is a valid argument.
Additional Locations (2)
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
1 participant
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.
Summary
Closes #2701
Closes #2065
Closes #2496
Closes #1220
Closes #1117
Closes #1048
Closes #2381
Checklist
Delete items not relevant to your PR:
Note
Medium Risk
Touches core JDBC serialization/deserialization for
Date/Time/Timestamp(timezone and nanos handling), which can subtly change behavior across timezones and existing applications despite added test coverage.Overview
Fixes JDBC v2 date/time parameter and result conversions to consistently honor the
Calendar/connection default timezone, addressing “day shift” and timestamp misinterpretation issues when reading/writingDate,Time, andTimestamp.Centralizes timezone-aware SQL↔Java conversions in
DataTypeUtils(new helpers forjava.sql.*andLocal*/ZonedDateTime, plus safe Unix-nanos string generation), updatesPreparedStatementImpl/WriterStatementImpl/ResultSetImplto use them, and adjusts encoding to usefromUnixTimestamp64Nano(...)forLocalDateTime/Instantand supportDurationasTime64strings. Binary readers now treatLocalDateTime-backed time values asDuration/TemporalAmountvialocalDateTimeToDuration.Adds extensive unit/integration tests for timezone round-trips and day-shift edge cases, and disables prior “implicit cast” tests that relied on server-side coercion.
Written by Cursor Bugbot for commit 5c582f7. This will update automatically on new commits. Configure here.