Add parsing for InternalSimpleValue and InternalDerivative #24162
Conversation
There was a problem hiding this comment.
This flag might seem a bit weird but I think we need it (or something similar) to distinguish cases where the original InternalDerivative.normalizationFactor was 0. Its an edge case, but in this case no xContent output is rendered at all. I couldn't think of another way to cover this edge case without somehow tracking whether this parser encountered any "normalized_value" or "normalized_value_as_string" fields in the parsed xContent.
There was a problem hiding this comment.
I think this is the way to go. Maybe we could document the purpose of this flag and maybe rename it to hasNormalizationFactor or hasValue or isValuePresent? Ok, you know I'm bad at naming things :)
There was a problem hiding this comment.
++ on hasNormalizationFactor . I would probably associate the flag to the parsing of normalized_value only. the other field is only optionally printed out.
tlrx
left a comment
tlrx
left a comment
There was a problem hiding this comment.
LGTM
I left some comments but they are minor. I really like how it becomes straightforward to add new types of aggregations to parse!
There was a problem hiding this comment.
min can be named simple or aggregation
There was a problem hiding this comment.
can you place everything on the same line?
There was a problem hiding this comment.
Can we rename min to something else?
There was a problem hiding this comment.
In which case value can be infinite?
There was a problem hiding this comment.
Also, we could put this check in a protected static method and reuse it in ParsedDerivative
There was a problem hiding this comment.
In which case value can be infinite?
Good question, this is just the same condition as in InternalSimpleValue, which I just assumed makes sense. I guess any pipeline agg that reduces to InternalSimpleValue could potentially set it to some infinite value.
Maybe we should share things like this across InternalSimpleValue/ParsedSimpleValue via the SimpleValue interface? Any opinions?
There was a problem hiding this comment.
but I thought we never set the parsed value to infinite?
There was a problem hiding this comment.
Good point, both infinite and NaN values are printed as null and parsed back as NaN so you are right, this shouldn't be possible in ParsedSimpleValue. I just copied the condition from the internal implementation to be on the safe side I guess. I can remove this here, or move to a shared condition in the interface like suggested above (and then have one "unnecessary check" in there...). Wdyt?
There was a problem hiding this comment.
I am slightly more in favour of having precise checks where needed rather than sharing code with unnecessary checks.
There was a problem hiding this comment.
Okay, I changed the condition and will merge this PR then. We can always change this later if we think its necessary.
There was a problem hiding this comment.
I think this is the way to go. Maybe we could document the purpose of this flag and maybe rename it to hasNormalizationFactor or hasValue or isValuePresent? Ok, you know I'm bad at naming things :)
There was a problem hiding this comment.
This is similar to the internal implementation, nice
There was a problem hiding this comment.
I added a randomNumericDocValueFormat() method in core which seems appropriate for this.
There was a problem hiding this comment.
I think Luca asked this question already but I didn't find the response... why Double.MIN_VALUE?
There was a problem hiding this comment.
it's a delta, mandatory argument in comparisons between double values.
There was a problem hiding this comment.
The version without a delta is deprecated, so I think better to add one. @tlrx yes, I can use 0 here, but I usually prefer to leave a very small value to allow minimal rounding errors. But I will change the delta to 0 here, hope to remember that when I see it in other place, please remind me if I don't ;-)
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a few minors, LGTM otherwise
There was a problem hiding this comment.
++ on hasNormalizationFactor . I would probably associate the flag to the parsing of normalized_value only. the other field is only optionally printed out.
There was a problem hiding this comment.
it's a delta, mandatory argument in comparisons between double values.
cbuescher
force-pushed
the
addParsing-SimpleValue-Derivative
branch
from
7ea2c6e to
1edc368
Compare
cbuescher
force-pushed
the
addParsing-SimpleValue-Derivative
branch
from
1edc368 to
0e3db29
Compare
3 participants
Similar to #24085 this adds parsing for InternalSimpleValue and InternalDerivative. This will be needed for
the high level rest client. PR is against the current feature branch for aggregation parsing.