[CALCITE-6703] RelJson cannot handle timestamps prior to 1970-01-25 2… #4060
Conversation
tanclary
force-pushed
the
6703-reljson
branch
from
1a6d87a
to
aea53d9
Compare
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertThatReadExpressionResult(timestampRepresentedAsInt, is("1970-01-25 15:30:00")); |
There was a problem hiding this comment.
is this supposed to work for dates before the epoch or the Gregorian calendar cutoff?
maybe you can add tests for these too - if they are not supposed to work, the errors can be tested.
There was a problem hiding this comment.
So the mechanism going from millis->timestamp String actually lives in DateTimeUtils in Avatica and is pretty well-tested in DateTimeUtilsTest (let me know if you need links.) The real bug here is that JSON doesn't distinguish the numeric types so we have to ensure it's a Long (or convert) it. So that's the scope I'm trying to test here.
There was a problem hiding this comment.
I have approved, I leave to you this decision.
| + " }\n" | ||
| + "}\n"; | ||
| final String timestampRepresentedAsLong = "{\n" | ||
| + " \"literal\": 3129400000,\n" |
There was a problem hiding this comment.
not easy to check that this value corresponds to the date below.
There was a problem hiding this comment.
Yeah I also wasn't sure about this.. the point I was trying to illustrate is that one case is below Integer.MAX_VALUE and the other is above, any suggestions?
There was a problem hiding this comment.
putting this in a comment will help a bit - the purpose of this choice is clearer
but if you have validated this using other external means, like another DB, you can also mention it in a comment.
There was a problem hiding this comment.
Ok i've added some comments
tanclary
force-pushed
the
6703-reljson
branch
3 times, most recently
from
6a9be62
to
9a43fe6
Compare
tanclary
force-pushed
the
6703-reljson
branch
from
9a43fe6
to
765ba72
Compare
|
|
@mihaibudiu have you seen the Windows failures like the ones on this PR before? They look unrelated but thought I'd ask? |
|
I have seen similar errors in windows builds, but not this many... they usually go away if you rerun the failed jobs. |
…0:31:23.648