[Transform] add support for unsigned_long data type #63940
Merged
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
writing out integer results properly, because coerce is not supported for unsigned_long fixes elastic#63871
|
Pinging @elastic/ml-core (:ml/Transform) |
benwtrent
reviewed
Oct 20, 2020
| * @param value the value as double (aggs return double for everything) | ||
| * @return value if its floating point, a integer | ||
| */ | ||
| public static Object convertToIntegerTypeIfNeeded(String type, double value) { |
There was a problem hiding this comment.
Suggested change
| public static Object convertToIntegerTypeIfNeeded(String type, double value) { | |
| public static Object convertToDiscreteNumeralTypeIfNeeded(String type, double value) { |
Or some better name as this is is not an integer but instead some possibly large discrete numeral (big int or a long).
There was a problem hiding this comment.
its referring to the mathematical definition of integer, that's why integer type, not integer. I can try to find a better name, maybe dropFloatingPointComponentIfNeeded, but lets not over-complicate it, the doc string defines what it does.
Comment on lines
+68
to
+75
| if (NUMERIC_FIELD_MAPPER_TYPES.getOrDefault(type, true) == false) { | ||
| assert value % 1 == 0; | ||
| if (value < Long.MAX_VALUE) { | ||
| return (long) value; | ||
| } | ||
|
|
||
| // special case for unsigned long | ||
| return BigDecimal.valueOf(value).toBigInteger(); |
There was a problem hiding this comment.
Its weird to me that there is not a blanket map doc value -> indexable value :(
This works in a pinch, I wonder if there is anyway to use DocValueFormat.UNSIGNED_LONG_SHIFTED.
But looking at it, there isn't a clean cut solution :/
added 2 commits
October 20, 2020 16:46
benwtrent
approved these changes
Oct 20, 2020
hendrikmuhs
pushed a commit
to hendrikmuhs/elasticsearch
that referenced
this pull request
Oct 20, 2020
add support for unsigned_long, which required a change in writing out integer results properly, because coerce is not supported for unsigned_long
This was referenced Oct 20, 2020
hendrikmuhs
pushed a commit
that referenced
this pull request
Oct 20, 2020
hendrikmuhs
pushed a commit
that referenced
this pull request
Oct 20, 2020
pugnascotia
pushed a commit
to pugnascotia/elasticsearch
that referenced
this pull request
Oct 21, 2020
add support for unsigned_long, which required a change in writing out integer results properly, because coerce is not supported for unsigned_long fixes elastic#63871
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
add support for unsigned_long, which required a change in
writing out integer results properly, because coerce is not
supported for unsigned_long
fixes #63871