Fix/5.x whole time units #2976
Merged
Fix/5.x whole time units #2976
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit changes the implementation of Time within the client for better consistency with Elasticsearch's TimeValue. The Time type in NEST 5.x is used to represent both Elasticsearch's TimeValue (which allows from nanos up to days as unit), and as time modifiers within DateMath expressions, for which Elasticsearch accepts units of y (years) M (months), w (weeks), d (days), h/H (hours), m (minutes) and s (seconds). - When converting from a TimeSpan to Time, the serialized value is a whole number with the largest unit of days. This fixes the implementation to be in line with Elasticsearch 5.0+ with elastic/elasticsearch#19102 where weeks was removed as a supported unit for TimeValue. - When constructing a Time using the constructor that takes a double of milliseconds, make the implementation consistent with the implicit conversion from double implementation; in the case of -1, both should be Time.MinusOne and the case of 0, both should be Time.Zero. If needing to express -1ms or 0ms, the constructor taking a factor and unit, or the constructor taking a string value with unit should be used. - When constructing a Time from double or TimeSpan, try to reduce the value to a whole number factor and largest represented unit (up to unit of days). For values smaller than 1 nanosecond, retain the factor as a fraction of nanoseconds. - When converting a Time to a TimeSpan, round the value to the nearest TimeSpan tick for microseconds and nanoseconds, and milliseconds for all other units. - When converting to a Time within a DateMath expression using Time.ToFirstUnitYieldingInteger(), when the value is fractional nanoseconds, round to the nearest nanosecond. Although this method is only used within the client for DateMath expressions and can return times with units smaller than a second (the smallest unit that DateMath within Elasticsearch accepts), the method is public so could be used by consumers for purposes other than DateMath. Because of this, don't break the implementation for units smaller than one second. - Allow the implicit conversion from string and the constructor that takes a string to continue to accept fractional units, which can be returned from Elasticsearch as per TimeValue's toString implementation https://github.com/elastic/elasticsearch/blob/f2501130399f6e3788e110d74dd62b3aad66804a/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java#L253-L290 - Update documentation Closes #2818
Contributor
Author
|
The implementation of
|
russcam
commented
Dec 21, 2017
| if (Math.Abs(milliseconds) < FLOAT_TOLERANCE) return Time.Zero; | ||
| if (Math.Abs(milliseconds - (-1)) < FLOAT_TOLERANCE) return MinusOne; | ||
| if (Math.Abs(milliseconds) < FLOAT_TOLERANCE) return Zero; | ||
| return new Time(milliseconds); |
Contributor
Author
There was a problem hiding this comment.
Can now simply return new Time(milliseconds);
Mpdreamz
approved these changes
Dec 21, 2017
| if (Math.Abs(milliseconds) < FLOAT_TOLERANCE) return Time.Zero; | ||
| if (Math.Abs(milliseconds - (-1)) < FLOAT_TOLERANCE) return MinusOne; | ||
| if (Math.Abs(milliseconds) < FLOAT_TOLERANCE) return Zero; | ||
| return new Time(milliseconds); |
Member
|
Awesome work @russcam
I remember going to some effort to build it as it was since the constructor mentions passing milliseconds but the way this PR normalizes it makes a lot more sense |
Add XML documentation to Time methods to describe what ToTimeSpan() and ToFirstUnitYieldingInteger() will do Simplify multiplication by reciprocals
ToFirstUnitYieldingInteger() is used in NEST only within DateMath expressions. Support whole values in months and weeks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit changes the implementation of Time within the client for better consistency with Elasticsearch's TimeValue. The Time type in NEST 5.x is used to represent both Elasticsearch's TimeValue (which allows from nanos up to days as unit), and as time modifiers within DateMath expressions, for which Elasticsearch accepts units of y (years) M (months), w (weeks), d (days), h/H (hours), m (minutes) and s (seconds).
When converting from a TimeSpan to Time, the serialized value is a whole number with the largest unit of days. This fixes the implementation to be in line with Elasticsearch 5.0+ with Keep input time unit when parsing TimeValues elasticsearch#19102 where weeks was removed as a supported unit for TimeValue.
When constructing a Time using the constructor that takes a double of milliseconds, make the implementation consistent with the implicit conversion from double implementation; in the case of -1, both should be Time.MinusOne and the case of 0, both should be Time.Zero. If needing to express -1ms or 0ms, the constructor taking a factor and unit, or the constructor taking a string value with unit should be used.
When constructing a Time from double or TimeSpan, try to reduce the value to a whole number factor and largest represented unit (up to unit of days). For values smaller than 1 nanosecond, retain the factor as a fraction of nanoseconds.
When converting a Time to a TimeSpan, round the value to the nearest TimeSpan tick for microseconds and nanoseconds, and milliseconds for all other units.
When converting to a Time within a DateMath expression using Time.ToFirstUnitYieldingInteger(), when the value is fractional nanoseconds, round to the nearest nanosecond. Although this method is only used within the client for DateMath expressions and can return times with units smaller than a second (the smallest unit that DateMath within Elasticsearch accepts), the method is public so could be used by consumers for purposes other than DateMath. Because of this, don't break the implementation for units smaller than one second.
Allow the implicit conversion from string and the constructor that takes a string to continue to accept fractional units, which can be returned from Elasticsearch as per TimeValue's toString implementation https://github.com/elastic/elasticsearch/blob/f2501130399f6e3788e110d74dd62b3aad66804a/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java#L253-L290
Update documentation
Closes #2818