-
Notifications
You must be signed in to change notification settings - Fork 24.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. The .0
was bothersome in the INTERVAL
s - we didn't support millis yet they got displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
long millis = TimeUnit.NANOSECONDS.toMillis(d.getNano()); | ||
if (millis > 0) { | ||
sb.append("."); | ||
while (millis % 10 == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -341,6 +341,10 @@ TemporalAmount parse(Source source, String string) { | |||
+ ": negative value [{}] not allowed (negate the entire interval instead)", | |||
v); | |||
} | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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)
This PR fixes:
.
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.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.