Add ParsedAggregation as base Aggregation impl for high level client #23965
Conversation
ParsedAggregation is the base Aggregation implementation for the high level client, which parses aggs responses into java objects.
tlrx
left a comment
tlrx
left a comment
There was a problem hiding this comment.
This should be in a dedicated branch. I made multiple comments, but this is mostly loud thinking so please to be bother by them. I'd like to keep the typed_keys logic out of the parsed aggregation as I think it should be handled by the NameXContentRegistry and that I suspect that we won't have a 1-1 correspondance between parsed agg and internal aggs.
Anyway, I'm fine to push this as it is as long as it is in a branch.
| * An implementation of {@link Aggregation} that is parsed from a REST response. | ||
| * Serves as a base class for all aggregation implementations that are parsed from REST. | ||
| */ | ||
| public abstract class ParsedAggregation implements Aggregation, ToXContent { |
There was a problem hiding this comment.
I also like "ExternalAggregation" but I'm OK with ParsedAggregation too.
| } | ||
|
|
||
| String name; | ||
| final Map<String, Object> metadata = new HashMap<>(); |
There was a problem hiding this comment.
not sure if protected is needed here, maybe it will. I'd rather make the change when we need it. The idea is that these fields can only be parsed, so they are only written through objectparser and package private is enough with that approach.
| (parser, context) -> parser.map(), InternalAggregation.CommonFields.META); | ||
| } | ||
|
|
||
| String name; |
| * to determine the internal type of the aggregation. | ||
| */ | ||
| //TODO it may make sense to move getType to the Aggregation interface given that we are duplicating it in both implementations | ||
| protected abstract String getType(); |
There was a problem hiding this comment.
I suspect that some aggregations could be grouped under the same parsed aggregation implementation, so we won't really have a 1-1 relationship between the internal agg and the parsed agg. Like an aggregation of type "sum" (ie InternalSum) and "min" (ie InternalMin) can be parsed back using a same LongSingleValueParsedAggregation. In definitive I'm not sure we should add the getType() here or also in the Aggregation interface.
There was a problem hiding this comment.
good point I will leave the TODO for now, I don't know either yet
| public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { | ||
| //TODO move TYPED_KEYS_DELIMITER constant out of InternalAggregation | ||
| // Concatenates the type and the name of the aggregation (ex: top_hits#foo) | ||
| builder.startObject(String.join(InternalAggregation.TYPED_KEYS_DELIMITER, getType(), name)); |
There was a problem hiding this comment.
Do we really need to also duplicate the typed_keys logic here? Can't we just print out the name of the parsed aggregation (it can be initialized with type#name)
There was a problem hiding this comment.
I think it would be nice that we can parse back what we print out? that is why I didn't make it conditional here, I rather always print out the type.
There was a problem hiding this comment.
oh I didn't realize that we would be able to parse it if we just called it type#name. but then getName would have to split the string anyway? Either way we have to split the string right?
There was a problem hiding this comment.
I understand. But this requires to grab back the type and delimiter where initializing the parsed aggregation directly with the name "type#name" would allow to parse back the result too. Also, in a client side point of view, the name is "type#name". But I'm nitpicking, we can change this later if we want.
There was a problem hiding this comment.
I think that users expect to get back the name with getName not type#name. This concatenation should be hidden to users, then they get back the same name as with the transport client.
|
|
||
| @Override | ||
| protected NamedXContentRegistry xContentRegistry() { | ||
| //TODO we may want to un-deprecate this Entry constructor if we are going to use it extensively, which I think we are |
|
Agreed, I pushed this to the https://github.com/elastic/elasticsearch/tree/feature/client_aggs_parsing feature branch. |
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; | ||
| import static org.hamcrest.CoreMatchers.instanceOf; | ||
|
|
||
| public class ParsedAggregationTests extends ESTestCase { |
There was a problem hiding this comment.
Is the idea to have this as a base test class or is this just here to check the new abstract class?
There was a problem hiding this comment.
I am not sure yet, for now it was useful to check out that the base class is right and the common metadata field gets parsed ok. we may want to remove it later once we have some real implementors. There's a TODO on that below.
Now the Java High Level Rest Client has tests to parse all aggregations, this test is not needed anymore. We have better tests like AggregationsTests and sub classes of InternalAggregationTestCase. Related to elastic#23965
Now the Java High Level Rest Client has tests to parse all aggregations, this test is not needed anymore. We have better tests like AggregationsTests and sub classes of InternalAggregationTestCase. Related to #23965
Now the Java High Level Rest Client has tests to parse all aggregations, this test is not needed anymore. We have better tests like AggregationsTests and sub classes of InternalAggregationTestCase. Related to elastic#23965
3 participants
ParsedAggregation is the base Aggregation implementation for the high level client, which parses aggs responses into java objects.