SQL: Fix milliseconds handling in intervals #51675
Conversation
This commit fixes: - the parsing of milliseconds in intervals: a value such as .23 is no longer converted to 23 millis, but to 230; - the printing of a trailing .0, in case the interval lacks the fractional part.
|
Pinging @elastic/es-search (:Search/SQL) |
bpintea
changed the title
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
Nice catch. The .0 was bothersome in the INTERVALs - we didn't support millis yet they got displayed.
matriv
left a comment
matriv
left a comment
There was a problem hiding this comment.
LGTM. Left some comments which are just suggestions, maybe your approach is better :-)
| long millis = TimeUnit.NANOSECONDS.toMillis(d.getNano()); | ||
| if (millis > 0) { | ||
| sb.append("."); | ||
| while (millis % 10 == 0) { |
| long millis = TimeUnit.NANOSECONDS.toMillis(d.getNano()); | ||
| if (millis > 0) { | ||
| sb.append("."); | ||
| while (millis % 10 == 0) { |
There was a problem hiding this comment.
Alternatively you could just iterate on the string millis from end->start and remove all zeroes.
Don't know if performance wise makes sense...
There was a problem hiding this comment.
I've also fluttered a bit between the two approaches. I went with this approach since generally arithmetic operations should be faster. But anyways, micro-optimisations.
| } | ||
| if (units.get(unitIndex) == TimeUnit.MILLISECOND && number.length() < 3) { | ||
| // normalize the number past DOT to millis | ||
| v *= number.length() < 2 ? 100 : 10; |
There was a problem hiding this comment.
Same concept here, you could avoid the multiplication by creating a string with right zero padding and then read is as number.
There was a problem hiding this comment.
This might be changed later if we ever bring the nanos to intervals and a multiplication might be more succinct. But yes, also here debatable.
|
Should this be targeting (Commenting because I'm curating release notes and this one caught my eye being still open) |
Sure, can wait for the next patch, not critical. |
This fixes: - the parsing of milliseconds in intervals: everything past the . used to be converted as-is to milliseconds, with no normalisation of the unit; thus, a value of .23 ended up as 23 millis in the interval, instead of 230. - the printing of a trailing .0, in case the interval lacks the fractional part; - tests generating a random millisecond value used to simply print it in the string about to be evaluated without a necessary front-filling of 0[s], where the amount was below 100/10. (The combination of first and last issues above, plus statistical "luck" made the incorrect handling pass the tests.) (cherry picked from commit 4de8c64)
@polyfractal I didn't merge this into 7.6 yet (as per the conversation above), so it won't be in 7.6.1. Will do that as soon as 7.6.1 is released. Sorry for the confusion. |
|
Sorry, this was my fault :) The release script just automatically re-labels things, and I forgot to go look through them to see if any were incorrectly re-labeled. I'll reset the label and set a backport-pending on it Apologies for the hassle! |
This fixes: - the parsing of milliseconds in intervals: everything past the . used to be converted as-is to milliseconds, with no normalisation of the unit; thus, a value of .23 ended up as 23 millis in the interval, instead of 230. - the printing of a trailing .0, in case the interval lacks the fractional part; - tests generating a random millisecond value used to simply print it in the string about to be evaluated without a necessary front-filling of 0[s], where the amount was below 100/10. (The combination of first and last issues above, plus statistical "luck" made the incorrect handling pass the tests.) (cherry picked from commit 4de8c64)
7 participants
This PR fixes:
.used to be converted as-is to milliseconds, with no normalisation of the unit; thus, a value of.23ended up as 23 millis in the interval, instead of 230.0[s], where the amount was below 100/10.(The combination of first and last issues above, plus statistical "luck" made the incorrect handling pass the tests.)
Closes #41635.