Remove QueryParseContext #25486

cbuescher · 2017-06-30T08:45:56Z

QueryParseContext is currently only used as a wrapper for an XContentParser, so
this change removes it entirely and changes the appropriate APIs that use it so
far to only accept a parser instead.

nik9000 · 2017-06-30T19:43:11Z

core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java

@@ -53,7 +53,7 @@
    public static final ParseField IGNORE_UNMAPPED = new ParseField("ignore_unmapped");
    public static final QueryBuilder DEFAULT_INNER_HIT_QUERY = new MatchAllQueryBuilder();

-    private static final ObjectParser<InnerHitBuilder, QueryParseContext> PARSER = new ObjectParser<>("inner_hits", InnerHitBuilder::new);


nik9000 · 2017-06-30T19:43:59Z

core/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java

+     * Internal: Registers sub-factories with this factory. The sub-factory will
+     * be responsible for the creation of sub-aggregators under the aggregator
+     * created by this factory. This is only for use by
+     * {@link AggregatorFactories#parseAggregators(org.elasticsearch.common.xcontent.XContentParser)}.


I think it is ok to import XContentParser to make the javadoc shorter if you like.

nik9000 · 2017-06-30T19:48:07Z

...rg/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilder.java

@@ -57,15 +57,15 @@
    private String separator = DEFAULT_SEPARATOR;

    public static Aggregator.Parser getParser() {


Can this be

public static final Aggregator.Parser PARSER; static { ObjectParser objectParser = new ObjectParser(); // build the parser PARSER = (name, parser) -> { // Stuff }; }

I think that'd be closer to how we parse other things. Not the same, but closer.

to be honest, I don't know the reason why this is constructed the way it is. Will see if I can change this.

I changed this to a more "standard" approach of constructing the parser.

nik9000 · 2017-06-30T19:49:57Z

...elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java

                IncludeExclude::parseInclude, IncludeExclude.INCLUDE_FIELD, ObjectParser.ValueType.OBJECT_ARRAY_OR_STRING);

-        parser.declareField((b, v) -> b.includeExclude(IncludeExclude.merge(b.includeExclude(), v)),


Same question as for the adjacency matrix one.

This is more tricky to untangle because of the significanceHeuristicParserRegistry that needs to be passed in. No idea why, but I tried changing this but it gets a bit to involved to do it in this PR I think.

Fair enough. I wouldn't change the capitalization though.

I had a quick look and opened #25519 with what I imagine the strategy is. It certainly looks big enough to be worth doing in its own PR.

I wouldn't change the capitalization though

Thats done because of naming conflicts I otherwise get in https://github.com/elastic/elasticsearch/pull/25486/files#diff-e5532441d96af10726cd20f6b653e3e9R101
I could rename the XContentparser there I guess, but we user capital case for ObjectParser quiet often.

I wouldn't name it in capital case because it isn't a constant. Otherwise I'm fine with whatever rename you like.

That makes sense, sorry it took me so long to understand what you mean, but I'm so used to see these parsers as constants (because they usually are) that I didn't see this one isn't.

nik9000 · 2017-06-30T19:53:39Z

Yes!

javanna

LGTM thanks @cbuescher for taking care of this

javanna · 2017-07-03T08:53:16Z

core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java

@@ -276,6 +275,9 @@ public final QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws
        return rewritten;
    }

+    /**
+     * @throws IOException exception on rewrite


nit: if we add javadocs maybe we should also say what the method is for?

I removed this docs, they serve no real purpose

QueryParseContext is currently only used as a wrapper for an XContentParser, so this change removes it entirely and changes the appropriate APIs that use it so far to only accept a parser instead.

nik9000 · 2017-07-03T13:44:05Z

...elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java

                IncludeExclude::parseInclude, IncludeExclude.INCLUDE_FIELD, ObjectParser.ValueType.OBJECT_ARRAY_OR_STRING);

-        parser.declareField((b, v) -> b.includeExclude(IncludeExclude.merge(b.includeExclude(), v)),


I had a quick look and opened #25519 with what I imagine the strategy is. It certainly looks big enough to be worth doing in its own PR.

nik9000 · 2017-07-03T13:44:45Z

...rg/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilder.java

-        };
+    private static final ObjectParser<AdjacencyMatrixAggregationBuilder, Void> PARSER = new ObjectParser<>(
+            AdjacencyMatrixAggregationBuilder.NAME);
+    static {


👍 Much more like the others. nice. And less lines!

* master: (52 commits) Include shared/attributes.asciidoc from docs master Fixed page breaks for ICU Collation Keyword Fields Remove QueryParseContext (elastic#25486) [Test] Use a common testing class for all XContent filtering tests (elastic#25491) Tests fix - Significant terms/text aggs (elastic#25499) [DOCS] add docs for REST high level client index method (elastic#25501) Tests: Add Debian 9 (Stretch) to the packaging tests test: Run flush before upgrade and refresh after upgrade. Fix third party audit for repository-hdfs [TEST] Expect nodes getting disconnected quickly testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow Cleanup network / transport related settings (elastic#25489) Fix repository-hdfs plugin packaging test Remove allocation id from replica replication response (elastic#25488) Adjust BWC version on bad allocation request test Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497) Adjust status on bad allocation explain requests Preliminary support for ARM Add doc note regarding explicit publish host Fix typo in name of test ...

cbuescher added :Query DSL >breaking-java v6.0.0 labels Jun 30, 2017

cbuescher requested review from nik9000 and javanna June 30, 2017 08:45

nik9000 approved these changes Jun 30, 2017

View reviewed changes

javanna approved these changes Jul 3, 2017

View reviewed changes

cbuescher force-pushed the remove-QueryParseContext-p2 branch 3 times, most recently from dd496c7 to 7d6ffcc Compare July 3, 2017 12:42

Remove QueryParseContext

d1f7a44

QueryParseContext is currently only used as a wrapper for an XContentParser, so this change removes it entirely and changes the appropriate APIs that use it so far to only accept a parser instead.

cbuescher force-pushed the remove-QueryParseContext-p2 branch from 7d6ffcc to d1f7a44 Compare July 3, 2017 13:18

nik9000 reviewed Jul 3, 2017

View reviewed changes

nik9000 approved these changes Jul 3, 2017

View reviewed changes

Rename parser

9548523

cbuescher merged commit f576c98 into elastic:master Jul 3, 2017

colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove QueryParseContext #25486

Remove QueryParseContext #25486

cbuescher commented Jun 30, 2017

nik9000 Jun 30, 2017

nik9000 Jun 30, 2017

nik9000 Jun 30, 2017

cbuescher Jul 3, 2017

cbuescher Jul 3, 2017

nik9000 Jun 30, 2017

cbuescher Jul 3, 2017

nik9000 Jul 3, 2017

nik9000 Jul 3, 2017

cbuescher Jul 3, 2017

nik9000 Jul 3, 2017

cbuescher Jul 3, 2017

nik9000 commented Jun 30, 2017

javanna left a comment

javanna Jul 3, 2017

cbuescher Jul 3, 2017

nik9000 Jul 3, 2017

nik9000 Jul 3, 2017

		@@ -57,15 +57,15 @@
		private String separator = DEFAULT_SEPARATOR;

		public static Aggregator.Parser getParser() {

		IncludeExclude::parseInclude, IncludeExclude.INCLUDE_FIELD, ObjectParser.ValueType.OBJECT_ARRAY_OR_STRING);

		parser.declareField((b, v) -> b.includeExclude(IncludeExclude.merge(b.includeExclude(), v)),

Remove QueryParseContext #25486

Remove QueryParseContext #25486

Conversation

cbuescher commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Jun 30, 2017

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment