ObjectParser: add method to parse inner objects with names matching a given predicate #24020

cbuescher · 2017-04-10T14:10:24Z

When parsing aggregations and sub-aggregations we encounter cases where the
aggregation name is written to the response as key for a new nested aggregation
object on the same level that we want to parse other fields. Currently we cannot
use ObjectParser in this situations because it requires some ParseField for
matching the field name.

This adds a new declareUnknownFieldParser() method to ObjectParser that can be
used to declare one specific ContextParser that gets called if no other
FieldParsers that have been declared match.

tlrx

Thanks @cbuescher, I left some comments. Since this is really related to aggregation parsing, I guess it could have been opened against the feature branch?

tlrx · 2017-04-10T14:44:15Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

I'm bad at naming things but I'd call this fallbackFieldParser

tlrx · 2017-04-10T14:47:11Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

I find it confusing to have a fallback/unknown field parser and still be able to ignore unknown fields. I'd make these two mutually exclusive, ie not possible to define an unknown field parser if ignoreUnknownFields is set to true.

I will add a check for this.

but we do have to do both right? the logic should be: check whether the agg is a registered one by type, where the type is part of the name (type#name). If not, ignore the field (for forward compatibility).

nik9000 · 2017-04-10T14:49:45Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

I'm not super comfortable making this. I think maybe instead we should add the match skipping at the FieldParser level. Maybe some kind of subclass that skips or something. Not sure.

I added an update that subclasses FieldParser but I found the previous solution more convincing tbh. I'd like to change it back if you don't have other ideas but added a commit to discuss it. Maybe I did it in a bad way.

javanna

I am not sure that this solution addresses our parsing problems. We would rather need a way to match based on a prefix I guess, and still be able to ignore other fields that don't match anything else. With this solution I think we could declare a match all parser that calls parser.namedObject, but then we should be able to ignore unknown fields if there isn't a named object with such a name. It could also be that I don't see how we would use this, in that case please educate me :)

I was rather thinking that we'd need a name to declare a named object among other fields, while at the moment we only allow it under a specific object container which can only contain named objects of a specific type. Hope this makes sense.

javanna · 2017-04-11T14:58:11Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

but we do have to do both right? the logic should be: check whether the agg is a registered one by type, where the type is part of the name (type#name). If not, ignore the field (for forward compatibility).

javanna · 2017-04-11T15:06:59Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

this example is not that close to reality. we cannot really parse such an agg as we need its type in the name (type#total_number_of_ratings). That is what makes the field not unknown, which is why I am not convinced that the mechanism should be a match all.

I updated the example. The test should now cover a more complex case with inner objects, although I didn't use any special aggregation parsers in the test to keep them more simple.

When parsing aggregations and sun-aggregations we encounter cases where the aggregation name is written to the response as key for a new nested aggregation object on the same level that we want to parse other fields. Currently we cannot use ObjectParser in this situations because it requires some ParseField for matching the field name. This adds a new `declareUnknownFieldParser()` method to ObjectParser that can be used to declare one specific ContextParser that gets called if no other FieldParsers that have been declared match.

cbuescher

I updated the PR to address some previous concerns. I renamed the new method to declareMatchFieldParser and it requires an additional Predicate argument now that needs to match the field name we are currently parsing to apply the provided ContextParser. Otherwise we fall back to the current ignoreUnknownFields logic. I also changed the test to reflect those cases.

javanna

I left a few comments, I would love if @nik9000 can have a look and see if this is generic enough to go to ObjectParser and be useful in other cases than aggregations parsing.

javanna · 2017-05-09T10:48:06Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

     */
    private final boolean ignoreUnknownFields;

+    /**


s/precidate/predicate

javanna · 2017-05-09T10:49:20Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+    /**
+     * A special purpose field parser that gets used when the provided matchFieldPrecidate accepts it
+     */
+    private FieldParser matchFieldParser;


I like the idea of the predicate. It could for instance be something like registry.supports(name, Aggregation.class). This way we could also support ignoreUnknownFields for forward compatibility

I am trying to think if these can be a single field rather than two that are so tightly coupled. I couldn't come up with a good idea but maybe you will...

I can combine these into an inner private struct-like class. I think they should be separate arguments in the declareMatchFieldParser method though, that way it's easier to use and we don't need to expose another class for this.

It could for instance be something like registry.supports(name, Aggregation.class).

Upon further thought, with the current signature (Predicate<String>) this would be difficult since the only information the function "sees" is the field name, and the registry is not visible when declaring the parser in the static initialization context. But I think we can postpone this extensions for later, this is useful as is, wdyt?

BiPredicate<String, NamedXContentRegistry> ? is that ugly? :)

javanna · 2017-05-09T10:49:41Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+    /**
+     * Declares a parser for fields where the exact field name is not known when
+     * creating the ObjectParser. In order for this field name to match, it has
+     * to be accepted by a provided predicate As an example, in the aggregation


add a punctuation mark after predicate?

javanna · 2017-05-09T10:51:14Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+     *            the accepted values for this field
+     */
+    public <T> void declareMatchFieldParser(BiConsumer<Value, T> consumer, ContextParser<Context, T> parser,
+            Predicate<String> fieldNameMatcher, ValueType type) {


rename fieldNameMatcher to fieldNamePredicate?

javanna · 2017-05-09T10:52:11Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+        this.matchFieldParser = new MatchAllFieldParser((p, v, c) -> consumer.accept(v, parser.parse(p, c)), type);
+    }
+
+    private class MatchAllFieldParser extends FieldParser {


should it still be called MatchAll although it parses only what matches a predicate?

No, will think about a better name.

Not very original, but I renamed this to MatchfieldParser, similar to the method that it uses. Its not a public class so I think thats is okay.

javanna · 2017-05-09T10:52:45Z

core/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

+            return new Tuple<>(name, value);
+        }, s -> s.contains("#"), ObjectParser.ValueType.STRING);
+
+        if (ignoreUnknown == true) {


do we need == true ? :)

No, I might have added that for readablility but I don't like it now that I see it.

cbuescher · 2017-05-09T12:04:07Z

For an example of how this would be used in the context of aggregation parsing see https://github.com/elastic/elasticsearch/pull/24386/files#diff-76a2fb6a5f584d4c41b8f7046955bf6bR66

nik9000

I like the idea of this. I imagine there are other contexts where this might be useful in addition to aggregation parsing.

nik9000 · 2017-05-09T17:50:58Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+        Objects.requireNonNull(parser, "[parser] is required");
+        Objects.requireNonNull(fieldNameMatcher, "[fieldNameMatcher] is required");
+        Objects.requireNonNull(type, "[type] is required");
+        this.matchFieldPredicate = fieldNameMatcher;


This should also require that this.matchFieldPredicate is not already set. For now, let's limit ourselves to only supporting one of these on each parser.

nik9000 · 2017-05-09T17:52:45Z

core/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

    }

+    /**
+     * test parsing fields with a random name


I think it'd be more right to say that this tests support for parsing unknown fields.

javanna

LGTM I am good with getting this in once the few minors you have are addressed

javanna

I left a couple of comments/ideas, but LGTM already.

javanna · 2017-05-10T09:20:46Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java

+        }
+    }
+
+    private class MatchFieldParser extends FieldParser {


I wonder why we need this class at this point. I guess it is because we don't provide a ParseField and the parseField.match part in assertSupports would not work. Would it be an idea to change parse field to always work with a predicate internally, and allow to pass the predicate in through a new constructor?

javanna · 2017-05-10T09:21:17Z

core/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java


+    /**
+     * A special purpose field parser that gets used when the provided matchFieldPredicate accepts it
+     */


this could be final?

javanna · 2017-05-10T10:21:19Z

After discussing with @tlrx and @cbuescher we decided not to get this in for now. It seems like it would add some complexity to ObjectParser, for something that is not that complicated to do when manually parsing aggregations. We may revisit this decision in the future, especially if more users of this pop up.

javanna · 2017-06-09T08:17:41Z

given that we completed the aggs parsing for the high level client and we didn't feel the need to get this PR in, shall we close this PR at this point? WDYT @cbuescher ?

cbuescher · 2017-06-09T08:21:03Z

@javanna sure, I still have the code around if needed for something else later on

cbuescher added the :Query DSL label Apr 10, 2017

tlrx reviewed Apr 10, 2017

View reviewed changes

nik9000 reviewed Apr 10, 2017

View reviewed changes

cbuescher added review v5.4.0 v6.0.0-alpha1 labels Apr 10, 2017

javanna reviewed Apr 11, 2017

View reviewed changes

cbuescher force-pushed the addUnknownFieldParsing-ObjectParser branch from e05a94b to 2a91a5e Compare April 26, 2017 17:00

clintongormley added v5.4.1 v6.0.0 and removed v5.4.0 v6.0.0-alpha1 labels Apr 28, 2017

cbuescher mentioned this pull request May 8, 2017

WIP: Add parsing single bucket #24386

Closed

cbuescher force-pushed the addUnknownFieldParsing-ObjectParser branch 5 times, most recently from 8b21a61 to 6a1169a Compare May 9, 2017 09:58

Adding predicate to check matching of unknown field name

59118cd

cbuescher force-pushed the addUnknownFieldParsing-ObjectParser branch from 6a1169a to 59118cd Compare May 9, 2017 10:14

cbuescher commented May 9, 2017

View reviewed changes

javanna reviewed May 9, 2017

View reviewed changes

nik9000 reviewed May 9, 2017

View reviewed changes

javanna approved these changes May 9, 2017

View reviewed changes

javanna added v5.5.0 and removed v5.4.1 labels May 9, 2017

Adressing review comments

a8d225a

javanna approved these changes May 10, 2017

View reviewed changes

javanna added :Internal and removed :Query DSL labels May 10, 2017

javanna changed the title ~~Add method to parse inner objects with unknown field names to ObjectParser~~ ObjectParser: add method to parse inner objects with names matching a given predicate May 10, 2017

javanna added stalled and removed review v5.5.0 v6.0.0 labels May 10, 2017

cbuescher closed this Jun 9, 2017

cbuescher mentioned this pull request Jul 3, 2017

Construct the significant terms aggregation builder's parser statically #25519

Closed

nik9000 mentioned this pull request Jan 2, 2020

x-content: Support collapsed named objects #50564

Merged

ObjectParser: add method to parse inner objects with names matching a given predicate #24020

ObjectParser: add method to parse inner objects with names matching a given predicate #24020

Uh oh!

Conversation

cbuescher commented Apr 10, 2017 • edited by nik9000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented May 9, 2017

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Apr 10, 2017 •

edited by nik9000

Loading

cbuescher May 10, 2017 •

edited

Loading