Validate out of range DateTime value in clickhouse connector #26703
Conversation
krvikash
requested review from
ssheikin,
ebyhr,
chenjian2664,
vlad-lyutenko,
marcinsbd and
anusudarsan
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
krvikash
force-pushed
the
krvikash/clickhouse-datetime-validation
branch
from
c75b5a9 to
4e46e02
Compare
|
Thanks @chenjian2664, Addressed your comments. |
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Show resolved
Hide resolved
| return (resultSet, columnIndex) -> { | ||
| ZonedDateTime zonedDateTime = resultSet.getObject(columnIndex, ZonedDateTime.class); | ||
| return packDateTimeWithZone(zonedDateTime.toInstant().toEpochMilli(), zonedDateTime.getZone().getId()); | ||
| }; |
There was a problem hiding this comment.
I think we should validate the reading as well
There was a problem hiding this comment.
When an out-of-range value is inserted into a ClickHouse table, ClickHouse stores it incorrectly by mapping it to a value within the supported range.
Example:
1912-01-01 00:00:00 → stored as 1970-01-01 00:00:00
3102-01-01 00:00:00 → stored as 2027-10-17 11:03:28
In Trino, the value that gets read appears to be within the supported range, making it impossible to tell whether 1970-01-01 00:00:00 was originally 1912-01-01 00:00:00 or actually 1970-01-01 00:00:00.
There was a problem hiding this comment.
3102-01-01 00:00:00→ stored as2027-10-17 11:03:28
Just curious: If the given date above MAX it's stored not as MAX (2106-02-07 06:28:15) but as 2027-10-17 11:03:28 ?
There was a problem hiding this comment.
Yes, I have checked with some different values as well and it is inserting different values for those cases.
krvikash
force-pushed
the
krvikash/clickhouse-datetime-validation
branch
from
4e46e02 to
c04a715
Compare
|
Thanks @anusudarsan @chenjian2664, AC. |
krvikash
force-pushed
the
krvikash/clickhouse-datetime-validation
branch
from
c04a715 to
3c5d35e
Compare
krvikash
force-pushed
the
krvikash/clickhouse-datetime-validation
branch
from
3c5d35e to
efc1e3a
Compare
|
(rebased with master) |
| import static io.trino.plugin.clickhouse.TestingClickHouseServer.ALTINITY_DEFAULT_IMAGE; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class TestAltinityClickHouseTypeMapping |
There was a problem hiding this comment.
Test classes should be defined as package-private and final.
Test methods should be defined as package-private.
There was a problem hiding this comment.
Since testUnsupportedPoint is declared public in the base class, it cannot be made package-private in this subclass.
Need refactoring of Base class. I think that can be a follow up.
krvikash
force-pushed
the
krvikash/clickhouse-datetime-validation
branch
from
efc1e3a to
c41a9f1
Compare
| return (resultSet, columnIndex) -> { | ||
| ZonedDateTime zonedDateTime = resultSet.getObject(columnIndex, ZonedDateTime.class); | ||
| return packDateTimeWithZone(zonedDateTime.toInstant().toEpochMilli(), zonedDateTime.getZone().getId()); | ||
| }; |
There was a problem hiding this comment.
3102-01-01 00:00:00→ stored as2027-10-17 11:03:28
Just curious: If the given date above MAX it's stored not as MAX (2106-02-07 06:28:15) but as 2027-10-17 11:03:28 ?
Description
Validate out of range DateTime value in clickhouse connector
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text: