Select query failing if miliseconds used as time for indexing#6937
Select query failing if miliseconds used as time for indexing#6937jon-wei merged 6 commits intoapache:masterfrom
Conversation
| return new DateTime(instant, ISOChronology.getInstanceUTC()); | ||
| } | ||
| catch (IllegalArgumentException ex) { | ||
| return new DateTime(Long.valueOf(instant), ISOChronology.getInstanceUTC()); |
There was a problem hiding this comment.
This will obscure the original exception, by reporting something like "java.lang.NumberFormatException: For input string" rather than the original DateTime String parsing exception. How about doing this instead:
try {
return new DateTime(instant, ISOChronology.getInstanceUTC());
}
catch (IllegalArgumentException ex) {
try {
return new DateTime(Long.valueOf(instant), ISOChronology.getInstanceUTC());
} catch (IllegalArgumentException ex2) {
// Throw the original exception, since it is more likely to be meaningful to the user.
throw ex;
}
}There was a problem hiding this comment.
An oversight on my part. I'll make the change. Thanks
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Production code LGTM, but please adjust the test.
| { | ||
| Long milis = System.currentTimeMillis(); | ||
| DateTime dt1 = DateTimes.of(milis.toString()); | ||
| Assert.assertTrue(DateTimes.COMMON_DATE_TIME_PATTERN.matcher(dt1.toString()).matches()); |
There was a problem hiding this comment.
Could you please edit this test to check that the parsed values are actually what we expect (not just that it matches COMMON_DATE_TIME_PATTERN). It would probably help to use a constant instead of System.currentTimeMillis().
|
@mirkojotic - are you able to adjust the test? If so we can get this into Druid 0.14.0. |
|
@gianm Yes. Will be working on it tomorrow. |
- Try converting to Integer and then multiply by 1000L to achieve milis. - If not successful try converting to Long or rethrow original exception.
- in addition to seconds and milisecs, this method currently supports even a date string.
| { | ||
| return new DateTime(instant, ISOChronology.getInstanceUTC()); | ||
| try { | ||
| Integer seconds = Integer.valueOf(instant); |
There was a problem hiding this comment.
This code won't work for timestamps that are near the epoch. I think it's a bit too clever and we should stick to only working on milliseconds timestamps and ISO8601 strings.
There was a problem hiding this comment.
As I wrote it last night I realized it's doing too much.
I'll work on it today.
|
@gianm let me know if it needs more work |
…#6937) * [apache#1332] Fix - select failing if milis used for idx. * Formating correction. * Address comment: throw original exception. * Using constant values in tests - Try converting to Integer and then multiply by 1000L to achieve milis. - If not successful try converting to Long or rethrow original exception. * DateTime#of has to support "2011-01-01T00:00:00" - in addition to seconds and milisecs, this method currently supports even a date string. * Handle only milisec timestamps and ISO8601 strings
…#7140) * [#1332] Fix - select failing if milis used for idx. * Formating correction. * Address comment: throw original exception. * Using constant values in tests - Try converting to Integer and then multiply by 1000L to achieve milis. - If not successful try converting to Long or rethrow original exception. * DateTime#of has to support "2011-01-01T00:00:00" - in addition to seconds and milisecs, this method currently supports even a date string. * Handle only milisec timestamps and ISO8601 strings
4 participants
If milliseconds were passed to DateTime#of as string
jodawould throw an Illegal Argument Exception because it expects milliseconds to be in form of a Long type. I've added a couple of tests to make sure it is working for seconds and milliseconds.Let me know if I need to change anything.
Solves #1332