Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6486 Phoenix uses inconsistent chronologies internally, breaking pre-Gregorian date handling - #1284
PHOENIX-6486 Phoenix uses inconsistent chronologies internally, breaking pre-Gregorian date handling#1284richardantal wants to merge 1 commit into
Conversation
stoty
commented
Aug 5, 2021
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
The solution of adjusting the time read from DB seems like a kludge to me.
I think that we should do is switch over everything in DateUtil to use the Joda Time, with ISO chronology.
That should take care of keeping the input and output string representations consistent at least.
This is the notes I took:
//TODO
//Switch everything over to Joda Formatter/Parser objects
//Parsing ALWAYS uses the default format string ( why ?)
//Formatting format string is configurable / settable
//Cache formatter objects by Format String / Timezone ?
richardantal
commented
Aug 6, 2021
I can see the point that it is not the most sophisticated solution to the problem. |
joshelser
left a comment
There was a problem hiding this comment.
When I checked the different type of dates we use in the code I concluded that at the end we return a java.sql.Date in Phoenix resultSet from a long value. The java.sql.Date has the GJChronology so even if until that point we consistently use the ISO chronology it will be changed.
If we're still falling back to GJ in what we return back to the user, is this change simply helping us be consistent in what gets written into Phoenix? If this is a less-impactful change than using Jodatime with ISO chronology everywhere, I'm not sure what we are getting/missing with this easier change.
| stmt.execute("UPSERT INTO " + tableName + " VALUES(1, TO_DATE('" + date1 | ||
| + "'), TO_TIME('" + date1 + "'), TO_TIMESTAMP('" + date1 + "'))"); | ||
| stmt.execute("UPSERT INTO " + tableName + " VALUES(2, TO_DATE('" + date2 | ||
| + "'), TO_TIME('" + date2 + "'), TO_TIMESTAMP('" + date2 + "'))"); | ||
| String f = " GMT', 'yyyy-MM-dd HH:mm:ss.SSS z', 'UTC"; | ||
| stmt.execute("UPSERT INTO " + tableName + " VALUES(4, TO_DATE('" + date2 + f | ||
| + "'), TO_TIME('" + date2 + f + "'), TO_TIMESTAMP('" + date2 + f + "'))"); |
There was a problem hiding this comment.
nit: could these be PreparedStatements too?
There was a problem hiding this comment.
I'd like to have test for both PreparedStatements and execute statements as well.
| assertions++; | ||
| } | ||
| } | ||
| assertEquals(assertions, 28); |
There was a problem hiding this comment.
nit: make this dates.size() *7? Not sure if there's a lot of value directly in this assertion either way.
There was a problem hiding this comment.
It can be deleted, I agree it does not add any value anymore. In development I used it to validate if I do the check on every column of every row.
| for (int i = 0; i < dates.size(); i++){ | ||
| assertTrue(rs.next()); | ||
| for (int j = 0; j < 6; j++){ | ||
| int expected = Integer.parseInt(dateLists.get(i).get(j)); |
There was a problem hiding this comment.
nit: IMO, would be more readable to add List<String> dateList = dateList.get(i) outside the inner-loop and make this dateList.get(j).
| Date d2 = parseDate(dt2.toString()); | ||
| long l1 = d1.getTime(); | ||
| long l2 = d2.getTime(); | ||
| return (ts + (l1 - l2)); |
There was a problem hiding this comment.
Any chance you could include a description as to how/why this math works?
There was a problem hiding this comment.
I made it a bit simpler and added comments in the latest change.
| Time d2 = parseTime(dt2.toString()); | ||
| long l1 = d1.getTime(); | ||
| long l2 = d2.getTime(); | ||
| return new Time(ts - (l1 - l2)); |
There was a problem hiding this comment.
An explanation for this one too would be great.
…ing pre-Gregorian date handling Change-Id: Ie176cc267db7a30585729660271e1636d5a503d6
6742bb9 to
ef72920Comparerichardantal
commented
Aug 16, 2021
Roughly yes, I changed the SimpleDateFormatParser to parse the date in ISO chronology to be more consistent with the ISODateFormatParser (as ISODateFormatParser is used more often). The major change is on the read part where I I simply adjust the long values to be accurate. |
stoty
commented
Aug 16, 2021
💔 -1 overall
This message was automatically generated. |
richardantal
commented
Aug 19, 2021
Closing this PR as this is not a good solution to the problem. |
No description provided.