Skip to content
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

Add parsing for InternalScriptedMetric aggregation #24738

Conversation

Projects
None yet
4 participants
@cbuescher
Copy link
Member

commented May 17, 2017

This adds parsing of the scripted_metric aggregation that we need for the high level java rest client.
While the parsing code itself is straight forward, it wasn't completely clear which kind of values we
should support to parse back from xContent. The documentation mentions: primitive types, String, Map, Array (containing elements of only the types listed here) so that's what is parsed and tested now. It might be possible that other kinds of values also currently work when using the transport client (e.g. StreamOutput#writeGenericValue() also supports GeoPoint, so that might work via transport but it gets rendered as a json object). I included one test to cover these cases with the example of GeoPoint as well.

@javanna
Copy link
Member

left a comment

left a few minors

@Override
public Object aggregation() {
if (aggregation.size() != 1) {
throw new IllegalStateException("aggregation was not reduced");

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

is this error up-to-date?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

I think so, at least in InternalScriptedMetric. I think in the parsed version we can ommit it.

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

yea that was my point, it doesn't apply to the parsed version of it.

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

I left an assert for tests and a comment though, otherwise this might look like a mistake later on.

if (token == XContentParser.Token.VALUE_NULL) {
return null;
} else if (token.isValue()) {
return XContentParserUtils.parseStoredFieldsValue(parser);

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

I think that it feels weird to use this method from here, given its name, as it was really meant to parse stored fields values based on assumptions made around the possible types we have there. I wouldn't want changes made there to be reflected here in the future. Shall we copy what it does over here?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

Sure, thats what I had before. I agree that a little copy/paste is better than to introduce unneeded dependencies in this case. Glad to change back.

@@ -19,7 +19,10 @@

package org.elasticsearch.search.aggregations.metrics.scripted;

import com.google.common.base.Supplier;

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

is this what you meant to use?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

No, good catch. Seems to do the same thing as java.util.function.Supplier apparently though.

() -> new ArrayList<>() };



This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

can you remove some of these empty lines?



@Override
@Before

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

I don't think you need @before here, the parent method already has it.

}

@SuppressWarnings("unchecked")
private static Object randomValue(Supplier<Object>[] valueTypes, int level) {

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

I wonder if it's worth having this in RandomObjects instead. But given that it is only used here I don't have a strong opinion on that.

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

it is also very specialized given that before dance that creates the suppliers, maybe wise to leave it here for now.

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

+1

assertValues(aggregation.aggregation(), parsed.aggregation());
}

private void assertValues(Object expected, Object actual) {

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

can this be static?

@@ -412,7 +412,8 @@ public String toString() {
OBJECT_OR_STRING(START_OBJECT, VALUE_STRING),
OBJECT_ARRAY_BOOLEAN_OR_STRING(START_OBJECT, START_ARRAY, VALUE_BOOLEAN, VALUE_STRING),
OBJECT_ARRAY_OR_STRING(START_OBJECT, START_ARRAY, VALUE_STRING),
VALUE(VALUE_BOOLEAN, VALUE_NULL, VALUE_EMBEDDED_OBJECT, VALUE_NUMBER, VALUE_STRING);
VALUE(VALUE_BOOLEAN, VALUE_NULL, VALUE_EMBEDDED_OBJECT, VALUE_NUMBER, VALUE_STRING),
VALUE_OBJECT_ARRAY(VALUE_BOOLEAN, VALUE_NULL, VALUE_EMBEDDED_OBJECT, VALUE_NUMBER, VALUE_STRING, START_OBJECT, START_ARRAY);

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

given that we add this to ObjectParser we should add tests for it as part of the object parser tests too. Maybe even commit it upstream from a separate PR?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

I don't see why this is necessary, those enums are merely combinations of token types and the parsing test added in this PR would fail if this wasn't working (indeed it did before I added this new constant includinf ARRAY_START and OBJECT_START). We also don't use any of the enums other than STRING and FLOAT in ObjectParserTests currently.
As for adding this as a separate PR upstream, that PR would only add an enum thats not used? I think we can add it together with merging the feature branch.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

@javanna thanks for the review, I pushed a commit that should address most of your comments and left some response for the others.

@tlrx

tlrx approved these changes May 17, 2017

Copy link
Member

left a comment

LGTM, I left some comments but feel free to implement them or not.

@SuppressWarnings("unchecked")
private static Object randomValue(Supplier<Object>[] valueTypes, int level) {
Supplier<Object> sup = valueTypes[level];
Object value = sup.get();

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

You can save a line and go Object value = valueTypes[level].get(); directly

Supplier<Object> sup = valueTypes[level];
Object value = sup.get();
if (value instanceof Map) {
int elements = randomIntBetween(1,5);

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

space after comma

if (value instanceof Map) {
int elements = randomIntBetween(1,5);
for (int i = 0; i < elements; i++) {
((Map<String, Object>) value).put(randomAlphaOfLength(5), randomValue(valueTypes, level + 1));

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

Could you cast once before the for loop?

} else if (value instanceof List) {
int elements = randomIntBetween(1,5);
for (int i = 0; i < elements; i++) {
((List<Object>) value).add(randomValue(valueTypes, level + 1));

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

Same here, cast before then use in loop?

GeoPoint point = (GeoPoint) expected;
Map<String, Object> pointMap = (Map<String, Object>) actual;
assertEquals(point.getLat(), pointMap.get("lat"));
assertEquals(point.getLon(), pointMap.get("lon"));

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

So here we have a difference between transport client and our rest client. Would it make sense to add an extra post-parsing treatment for maps that in case of a map of 2 keys lat and lon converts it back to a GeoPoint?

This comment has been minimized.

Copy link
@cbuescher

cbuescher May 17, 2017

Author Member

I don't know, this won't be the only case but one of many. I don't think we should be forced to treat each of those as a special case in the rest client, the documentation says we don't support them anyway. Maybe @colings86 has an opinion about this though...

This comment has been minimized.

Copy link
@javanna

javanna May 17, 2017

Member

I think we should draw a line here. Users need to understand that we are going through REST and some things have to change as the protocol is different. I am not sure that patching this to make things work like with the transport client will work, there will be edge cases that we don't handle anyways. After all it should be easy for people to migrate, and a few users may be hit by this right?

This comment has been minimized.

Copy link
@colings86

colings86 May 17, 2017

Member

I agree, really all we can do is provide a best effort for compatibility between the two and there will inevitably be differences. I think this is a case where the extra effort to make the two the same is not worth it

This comment has been minimized.

Copy link
@tlrx

tlrx May 17, 2017

Member

Good for me, I didn't have a strong feeling about it.

@javanna
Copy link
Member

left a comment

LGTM

@cbuescher cbuescher merged commit 9fc9db2 into elastic:feature/client_aggs_parsing May 17, 2017

1 check passed

CLA Commit author is a member of Elasticsearch
Details

javanna added a commit to javanna/elasticsearch that referenced this pull request May 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.