Bugfix: Timestamps sometimes converted to Dates in legacy engine #3613
Conversation
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
Signed-off-by: Simeon Widdis <[email protected]>
| @@ -3,6 +3,6 @@ | |||
| {"index":{"_id":"2"}} | |||
| {"login_time":"2015-01-01T12:10:30Z"} | |||
| {"index":{"_id":"3"}} | |||
| {"login_time":"1585882955"} | |||
| {"login_time":"1585882955000"} | |||
There was a problem hiding this comment.
I found while writing the reproducers that this was getting parsed as milliseconds instead of seconds, despite the magnitude. That gets sent to January 19, 1970 instead of (probably intended) April 3, 2020. I decided to update the row.
Signed-off-by: Simeon Widdis <[email protected]>
Swiddis
requested review from
ps48,
kavithacm,
derek-ho,
joshuali925,
dai-chen,
YANG-DB,
mengweieric,
penghuo,
seankao-az,
MaxKsyunz,
Yury-Fridlyand,
anirudha,
forestmvey,
acarbonetto,
GumpacG,
ykmr1224,
LantaoJin,
noCharger and
qianheng-aws
as code owners
Swiddis
added
legacy
| // breakage for consumers like JDBC. Until Calcite's done and we can delete legacy, just reset | ||
| // the type. | ||
| String t = column.getType(); | ||
| if (t.equals("date")) { |
There was a problem hiding this comment.
Is it always true that we need to convert date to timestamp?
There was a problem hiding this comment.
Probably not, but I haven't figured out the smart way to do this. It's better to overconvert to timestamp than underconvert since date has less info according to JDBC.
There was a problem hiding this comment.
Passes all the tests, so if there's a case where overconverting causes issues, it isn't tracked
There was a problem hiding this comment.
So the hack is to replace the column type name only, but the value is not touched?
There was a problem hiding this comment.
You can see in the bug reports that even when the column was date, the value still holds time information, so it's sufficient to just rename the column.
| @@ -0,0 +1,8 @@ | |||
| {"index":{"_id":"1"}} | |||
There was a problem hiding this comment.
leverage YamlRestTest in future, avoid unncessary mapping and data files.
| // TODO currently out of scope due to V1/V2 engine feature mismatch. Should be fixed with Calcite. | ||
| @Test | ||
| @Ignore | ||
| public void joinTimestampComparison() throws IOException { |
There was a problem hiding this comment.
have u try enable calcite, issue still exist?
Description
Another Legacy-V2 mismatch issue, something in Legacy sometimes converts
timestamps todates, causing prod breakage for some JDBC users. I couldn't find the root cause of the inconsistent conversion, so I just threw a hack onProtocolinstead. (By my understanding, Protocol is only called in Legacy.)We should be able to remove all this when Calcite happens.
I invite reviewers to find a better way to do this.
Related Issues
Resolves #1545
Resolves #3159
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.