-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add ParsedAggregation as base Aggregation impl for high level client #23965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations; | ||
|
|
||
| import org.elasticsearch.common.xcontent.ObjectParser; | ||
| import org.elasticsearch.common.xcontent.ToXContent; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * 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 { | ||
|
|
||
| //TODO move CommonFields out of InternalAggregation | ||
| protected static void declareCommonFields(ObjectParser<? extends ParsedAggregation, Void> objectParser) { | ||
| objectParser.declareObject((parsedAgg, metadata) -> parsedAgg.metadata.putAll(metadata), | ||
| (parser, context) -> parser.map(), InternalAggregation.CommonFields.META); | ||
| } | ||
|
|
||
| String name; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. protected? |
||
| final Map<String, Object> metadata = new HashMap<>(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. protected?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| @Override | ||
| public final String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| @Override | ||
| public final Map<String, Object> getMetaData() { | ||
| return Collections.unmodifiableMap(metadata); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a string representing the type of the aggregation. This type is added to | ||
| * the aggregation name in the response, so that it can later be used by REST clients | ||
| * 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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point I will leave the TODO for now, I don't know either yet |
||
|
|
||
| //TODO the only way to avoid duplicating this method is making Aggregation extend ToXContent | ||
| //and declare toXContent as a default method in it. Doesn't sound like the right thing to do. | ||
| 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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
| if (metadata.isEmpty() == false) { | ||
| builder.field(InternalAggregation.CommonFields.META.getPreferredName()); | ||
| builder.map(metadata); | ||
| } | ||
| doXContentBody(builder, params); | ||
| builder.endObject(); | ||
| return builder; | ||
| } | ||
|
|
||
| protected abstract XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations; | ||
|
|
||
| import org.elasticsearch.common.ParseField; | ||
| import org.elasticsearch.common.bytes.BytesReference; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.common.xcontent.NamedXContentRegistry; | ||
| import org.elasticsearch.common.xcontent.ObjectParser; | ||
| import org.elasticsearch.common.xcontent.ToXContent; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.common.xcontent.XContentHelper; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
| import org.elasticsearch.common.xcontent.XContentType; | ||
| import org.elasticsearch.test.ESTestCase; | ||
| import org.elasticsearch.test.rest.FakeRestRequest; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; | ||
| import static org.hamcrest.CoreMatchers.instanceOf; | ||
|
|
||
| public class ParsedAggregationTests extends ESTestCase { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the idea to have this as a base test class or is this just here to check the new abstract class?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| //TODO maybe this test will no longer be needed once we have real tests for ParsedAggregation subclasses | ||
| public void testParse() throws IOException { | ||
| String name = randomAlphaOfLengthBetween(5, 10); | ||
| int numMetas = randomIntBetween(0, 5); | ||
| Map<String, Object> meta = new HashMap<>(numMetas); | ||
| for (int i = 0; i < numMetas; i++) { | ||
| meta.put(randomAlphaOfLengthBetween(3, 10), randomAlphaOfLengthBetween(3, 10)); | ||
| } | ||
| TestInternalAggregation testAgg = new TestInternalAggregation(name, meta); | ||
| XContentType xContentType = randomFrom(XContentType.values()); | ||
| FakeRestRequest params = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) | ||
| .withParams(Collections.singletonMap("typed_keys", "true")).build(); | ||
| BytesReference bytesAgg = XContentHelper.toXContent(testAgg, xContentType, params, randomBoolean()); | ||
| try (XContentParser parser = createParser(xContentType.xContent(), bytesAgg)) { | ||
| parser.nextToken(); | ||
| assert parser.currentToken() == XContentParser.Token.START_OBJECT; | ||
| parser.nextToken(); | ||
| assert parser.currentToken() == XContentParser.Token.FIELD_NAME; | ||
| String currentName = parser.currentName(); | ||
| int i = currentName.indexOf(InternalAggregation.TYPED_KEYS_DELIMITER); | ||
| String aggType = currentName.substring(0, i); | ||
| String aggName = currentName.substring(i + 1); | ||
| Aggregation parsedAgg = parser.namedObject(Aggregation.class, aggType, aggName); | ||
| assertThat(parsedAgg, instanceOf(TestParsedAggregation.class)); | ||
| assertEquals(testAgg.getName(), parsedAgg.getName()); | ||
| assertEquals(testAgg.getMetaData(), parsedAgg.getMetaData()); | ||
| BytesReference finalAgg = XContentHelper.toXContent((ToXContent) parsedAgg, xContentType, randomBoolean()); | ||
| assertToXContentEquivalent(bytesAgg, finalAgg, xContentType); | ||
| } | ||
| } | ||
|
|
||
| @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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| NamedXContentRegistry.Entry entry = new NamedXContentRegistry.Entry(Aggregation.class, new ParseField("type"), | ||
| (parser, name) -> TestParsedAggregation.fromXContent(parser, (String)name)); | ||
| return new NamedXContentRegistry(Collections.singletonList(entry)); | ||
| } | ||
|
|
||
| private static class TestParsedAggregation extends ParsedAggregation { | ||
| private static ObjectParser<TestParsedAggregation, Void> PARSER = new ObjectParser<>("testAggParser", TestParsedAggregation::new); | ||
|
|
||
| static { | ||
| ParsedAggregation.declareCommonFields(PARSER); | ||
| } | ||
|
|
||
| @Override | ||
| protected String getType() { | ||
| return "type"; | ||
| } | ||
|
|
||
| @Override | ||
| protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { | ||
| return builder; | ||
| } | ||
|
|
||
| public static TestParsedAggregation fromXContent(XContentParser parser, String name) throws IOException { | ||
| TestParsedAggregation parsedAgg = PARSER.parse(parser, null); | ||
| parsedAgg.name = name; | ||
| return parsedAgg; | ||
| } | ||
| } | ||
|
|
||
| private static class TestInternalAggregation extends InternalAggregation { | ||
|
|
||
| private TestInternalAggregation(String name, Map<String, Object> metaData) { | ||
| super(name, Collections.emptyList(), metaData); | ||
| } | ||
|
|
||
| @Override | ||
| public String getWriteableName() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| protected String getType() { | ||
| return "type"; | ||
| } | ||
|
|
||
| @Override | ||
| protected void doWriteTo(StreamOutput out) throws IOException { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public InternalAggregation doReduce(List<InternalAggregation> aggregations, ReduceContext reduceContext) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public Object getProperty(List<String> path) { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { | ||
| return builder; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like "ExternalAggregation" but I'm OK with ParsedAggregation too.