Uh oh!
There was an error while loading. Please reload this page.
ARROW-9973: [Java] JDBC DateConsumer does not allow dates before epoch - #8167
ARROW-9973: [Java] JDBC DateConsumer does not allow dates before epoch#8167pwoody wants to merge 5 commits into
Conversation
| int day = (int) TimeUnit.MILLISECONDS.toDays(date.getTime()); | ||
| if (day < 0 || day > MAX_DAY) { | ||
| long day = TimeUnit.MILLISECONDS.toDays(date.getTime()); | ||
| if (day < Integer.MIN_VALUE || day > MAX_DAY) { |
There was a problem hiding this comment.
IMO, the lower bound should not be Integer.MIN_VALUE. Instead, it should be the date corresponding to '-9999-12-31'?
There was a problem hiding this comment.
Is there context as to why the date range of -9999-12-31 to 9999-12-31? I hadn't seen it in the format spec.
There was a problem hiding this comment.
It seems different databases have different date ranges. 9999-12-31 is one frequently used (in MySQL & SQL Server).
https://stackoverflow.com/questions/37138076/get-max-and-min-date-of-java-sql-date
There was a problem hiding this comment.
Interesting, I'm mostly inclined to just open the bounds to whatever epoch days fit within an integer range given that this is simply a reader and underlying systems have different ranges. Thoughts?
e.g. postgres has a max year of 5874897
There was a problem hiding this comment.
Sounds reasonable. However, Integer.MIN_VALUE seems overly conservative, as it forces us to use big int comparisons.
There was a problem hiding this comment.
Yeah, but otherwise we can end up with particularly bad data. I've changed the condition to just be something similar to Math.toExactInt where we see if the casted value is the same.
Without doing the big int comparison I think we run the risk of wrapping around into a totally valid positive range (e.g. 2*Integer.MAX_VALUE + x)
| if (day < 0 || day > MAX_DAY) { | ||
| throw new IllegalArgumentException("Day overflow: " + day); | ||
| long dayLong = TimeUnit.MILLISECONDS.toDays(date.getTime()); | ||
| int day = (int) dayLong; |
There was a problem hiding this comment.
Maybe you can use LargeMemoryUtil#checkedCastToInt?
There was a problem hiding this comment.
I've made it toIntExact to be more explicit about what we are going for @liyafan82 .
LargeMemoryUtil#checkedCastToInt seems to be guarded by an unrelated property (ARROW_ENABLE_UNSAFE_MEMORY_ACCESS) and also just seems to be implemented incorrectly:length <= Integer.MAX_VALUE || length >= Integer.MIN_VALUE is always true? Think we want that to be an && (or we simply delegate to the toIntExact like above).
There was a problem hiding this comment.
Nice catch. Maybe you can open an issue for it if you are interested.
| long dayLong = TimeUnit.MILLISECONDS.toDays(date.getTime()); | ||
| int day = (int) dayLong; | ||
| if (day != dayLong) { | ||
| throw new IllegalArgumentException("Day overflow: " + dayLong); |
There was a problem hiding this comment.
Maybe we should also use Math.toIntExact here?
liyafan82
commented
Sep 15, 2020
@pwoody A small problem, and I think it would be ready for merge. |
liyafan82
commented
Sep 15, 2020
Merging this. Thanks @pwoody |
Reported here: https://issues.apache.org/jira/browse/ARROW-9973 Looks like a pretty straightforward fix. Let me know if you have any comments - thanks! Closesapache#8167 from pwoody/pw/ARROW-9973 Authored-by: Patrick Woody <pwoody@branta.io> Signed-off-by: liyafan82 <fan_li_ya@foxmail.com>
Reported here: https://issues.apache.org/jira/browse/ARROW-9973
Looks like a pretty straightforward fix. Let me know if you have any comments - thanks!