Adding ParsedCardinality #23973
Adding ParsedCardinality #23973
Conversation
There was a problem hiding this comment.
I changed this (and the way the class interacts with the internal field) because I wanted to get close to what InternalAggregation allows for metadata. I think it can be null there, in which case the method returns null and we don't write anything to xContent.
There was a problem hiding this comment.
as I said above, that makes sense, I adjusted it on our branch that way.
There was a problem hiding this comment.
okay, will rebase this then
There was a problem hiding this comment.
I pushed a very similar change to our branch. There was a discrepancy between empty and null handling in this class compared to InternalAggregation, I aligned it to InternalAggregation. this should not be needed anymore here.
There was a problem hiding this comment.
okay, will rebase this then
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a few comments, LGTM otherwise
There was a problem hiding this comment.
why protected? subclasses can use the getter to retrieve it? but it only gets set while parsing? was this change required?
There was a problem hiding this comment.
I think you forgot to put back the randomization here
There was a problem hiding this comment.
do we want to shuffle these fields?
There was a problem hiding this comment.
Maybe not always, but that would be nice indeed
There was a problem hiding this comment.
what is Double.MIN_VALUE doing here?
There was a problem hiding this comment.
Thats just the expected delta between expeted/actual value for double comparissons in assertEquals()
There was a problem hiding this comment.
This is weird as this value is not returned with the response. But we can hardcode the formatter because this agg doesn't support format? I am a bit confused. Ideally we wouldn't have to implement this at all.
There was a problem hiding this comment.
Yes, InternalCardinality doesn't print "value_as_string", but you can get a formated value using the java API using valueAsString defined in InternalNumericMetricsAggregation. That one uses the default formater, which is DocValueFormat.RAW. I don't think this can be reset in InternalCardinality, thats why I use it here to get the same behaviour as InternalCardinality.
There was a problem hiding this comment.
ok can you expand your comment on why we do this and why it's ok? I also wish one day we get rid of it. I thought about throwing UnsupportedOperationException for a sec, but let's keep it for now as-is.
There was a problem hiding this comment.
Maybe not always, but that would be nice indeed
cbuescher
force-pushed
the
internalCardinality-parsing
branch
from
8363f74 to
0fe3f5a
Compare
cbuescher
force-pushed
the
internalCardinality-parsing
branch
from
0fe3f5a to
9cd83b2
Compare
| } | ||
|
|
||
| protected void setName(String name) { | ||
| this.name = name; |
There was a problem hiding this comment.
I think that with this protected setter, you can now make the instance member private?
| return cardinalityValue; | ||
| } | ||
|
|
||
| private void setValue(long cardinalityValue) { |
There was a problem hiding this comment.
It is a bit pointless to have a private setter for a private field, but we need it for object parser I guess?
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
LGTM thanks @cbuescher . I wonder if we want to remove ParsedAggregationTests at this point, not as part of this PR though.
Adding parsing of InternalCardinality xContent output. Parsing method will return a new implementation of the Cardinality interface, ParsedCardinality.
3 participants
First stab at adding parsing one of the single value aggregations from xContent.
This is WIP against our current feature branch.