Refactors SimpleQueryStringBuilder/Parser #11274

MaineC · 2015-05-21T07:23:03Z

Posting as early preview - in the current state makes SimpleQueryStringBuilder streamable, adds hashCode and equals. Next step adds Lucene query checks.

Note: Switched to using toLanguageTag/forLanguageTag when parsing Locales. Using LocaleUtils from either Elasticsearch or Apache commons resulted in Locales not passing the roundtrip test. For more info see https://issues.apache.org/jira/browse/LUCENE-4021 and the respective Java Locale.toString() documentation.

Relates to #10217

This PR goes against the feature/query-refactoring feature branch.

javanna · 2015-05-26T11:47:28Z

src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java

+    @Override
+    public Query toQuery(QueryParseContext parseContext) {
+        // Query text is required
+        if (queryText == null) {


this check should be moved to the validate method I think

javanna · 2015-05-26T12:39:49Z

done first round of review, left some comments.

MaineC · 2015-05-28T07:56:38Z

I briefly chatted with @dakrone - here's his take on adding the Boostable interface:

I think the SimpleQueryString stuff could implement Boostable, the
reason it didn't previously I think was that it allowed specifying
fields in the ["field1", "field2"^2.0] syntax. We should add support for
the "boost" field that boosts the entire query to the syntax as well.

javanna · 2015-06-04T16:05:24Z

src/main/java/org/elasticsearch/index/query/SimpleQueryStringBuilder.java

+     * always sorted in same order for generated Lucene query for easier
+     * testing.
+     */
+    private final Map<String, Float> fieldsAndWeights = new TreeMap<>();


feels like having to use a treemap for better testing might not be a good idea, unless we really need it besides testing. I guess this has to do with comparing expected lucene query with the output of toQuery? Does the lucene equals rely on ordering?

Yes, that is for comparing expected to generated lucene query. From reading the equals method of boolean queries the clauses end up in an ArrayList which is sensitive to ordering.

seems like a bug in lucene: https://issues.apache.org/jira/browse/LUCENE-6305 can you put a comment so that we know why and we get back to it once we fix the bug in lucene please?

Thanks for the pointer. Sure - will add a comment.

javanna · 2015-06-22T13:34:32Z

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

+        return Objects.hash(fieldsAndWeights, analyzer, defaultOperator, queryText, queryName, minimumShouldMatch, settings);
+    }
+
+    /** {@inheritDoc} */


I think it's not needed

javanna · 2015-06-22T13:38:40Z

I did a last round of review, this is very close.

MaineC · 2015-06-22T18:26:10Z

Will make the code changes tomorrow morning when awake - thanks for your comments. Will ping you when done.

javanna · 2015-06-23T08:57:40Z

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

+        /** Specifies whether lenient query parsing should be used. */
+        private Boolean lenient = SimpleQueryStringBuilder.DEFAULT_LENIENT;
+        /** Specifies whether wildcards should be analyzed. */
+        private Boolean analyzeWildcard = SimpleQueryStringBuilder.DEFAULT_ANALYZE_WILDCARD;


I think these Booleans can stay boolean primitive types? we now have the public constants for defaults, I think thats good enough

Than I need to re-add the null-check there:

#11274 (diff)

sorry for the confusion. we shouldn't need null checks if we use boolean everywhere for these members? Just initialize them in the parser with their default values?

Ah - that's what you meant. Sure, makes sense.

javanna · 2015-06-23T10:04:32Z

LGTM squash and push? ;)

This commit makes SimpleQueryStringBuilder streamable, add hashCode and equals. Adds a dedicated builder/parser unit test, fixes formatting, adds JavaDoc where needed, adjust the handling of default values according to https://github.com/elastic/dev/blob/master/design/queries/general-guidelines.md Switched to using toLanguageTag/forLanguageTag when parsing Locales. Using LocaleUtils from either Elasticsearch or Apache commons resulted in Locales not passing the roundtrip test. For more info see https://issues.apache.org/jira/browse/LUCENE-4021 Relates to elastic#10217

…actoring Refactors SimpleQueryStringBuilder/Parser

Forgot to commit the very last change yesterday which led to analyzeWildcard to remain at the default value always. Relates to #11274

…ing-refactoring Refactors SimpleQueryStringBuilder/Parser

Forgot to commit the very last change yesterday which led to analyzeWildcard to remain at the default value always. Relates to elastic#11274

Brings Lucene query assertions to QB test. Theser assertions were originally added as part of the SimpleQueryStringQueryBuilder refactoring but removed later as there were more extensive tests in place already. This commit brings them back in as the other tests have been removed. This relates to #10217 and #11274

javanna · 2015-09-01T11:48:03Z

Marked this breaking. This PR contains a little breakage for the java api, as the SimpleQueryStringQueryBuilder#field(String) method doesn't allow to specify the boost in form field("field^2"). If you want to specify a boost you need to switch to SimpleQueryStringQueryBuilder#field(String, float) and do field("field", 2);
It breaks how we parse the locale on the REST layer, as we now do Locale#forLanguageTag and we output using Locale#toLanguageTag. This is more correct but not a backwards compatible change. Locales that were previously properly parsed might not get parsed anymore. See #13229.

MaineC self-assigned this May 21, 2015

MaineC force-pushed the feature/simple-query-string-refactoring branch 3 times, most recently from 437b01b to 3b575fd Compare May 22, 2015 08:52

javanna added the :Query Refactoring label May 26, 2015

javanna reviewed May 26, 2015
View reviewed changes

MaineC force-pushed the feature/simple-query-string-refactoring branch 4 times, most recently from 89b3058 to b472e1e Compare June 3, 2015 08:03

javanna reviewed Jun 4, 2015
View reviewed changes

javanna reviewed Jun 22, 2015
View reviewed changes

MaineC force-pushed the feature/simple-query-string-refactoring branch 2 times, most recently from 02a253c to 5246a59 Compare June 23, 2015 08:46

javanna reviewed Jun 23, 2015
View reviewed changes

MaineC force-pushed the feature/simple-query-string-refactoring branch from cc8df97 to e170c8e Compare June 23, 2015 19:26

MaineC pushed a commit that referenced this pull request Jun 23, 2015

Merge pull request #11274 from MaineC/feature/simple-query-string-ref…

7afa37c

…actoring Refactors SimpleQueryStringBuilder/Parser

MaineC merged commit 7afa37c into elastic:feature/query-refactoring Jun 23, 2015

MaineC removed the review label Jun 23, 2015

MaineC pushed a commit that referenced this pull request Jun 24, 2015

Fix SimpleQueryStringBuilder wildcard handling

b166259

Forgot to commit the very last change yesterday which led to analyzeWildcard to remain at the default value always. Relates to #11274

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Merge pull request elastic#11274 from MaineC/feature/simple-query-str…

9a1ddf9

…ing-refactoring Refactors SimpleQueryStringBuilder/Parser

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Fix SimpleQueryStringBuilder wildcard handling

850a4a4

Forgot to commit the very last change yesterday which led to analyzeWildcard to remain at the default value always. Relates to elastic#11274

javanna added the >breaking label Sep 1, 2015

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors SimpleQueryStringBuilder/Parser #11274

Refactors SimpleQueryStringBuilder/Parser #11274

MaineC commented May 21, 2015

javanna May 26, 2015

javanna commented May 26, 2015

MaineC commented May 28, 2015

javanna Jun 4, 2015

MaineC Jun 9, 2015

javanna Jun 9, 2015

MaineC Jun 10, 2015

javanna Jun 22, 2015

javanna commented Jun 22, 2015

MaineC commented Jun 22, 2015

javanna Jun 23, 2015

MaineC Jun 23, 2015

javanna Jun 23, 2015

MaineC Jun 23, 2015

javanna commented Jun 23, 2015

javanna commented Sep 1, 2015

Refactors SimpleQueryStringBuilder/Parser #11274

Refactors SimpleQueryStringBuilder/Parser #11274

Conversation

MaineC commented May 21, 2015

Choose a reason for hiding this comment

javanna commented May 26, 2015

MaineC commented May 28, 2015

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

javanna commented Jun 22, 2015

MaineC commented Jun 22, 2015

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

javanna commented Jun 23, 2015

javanna commented Sep 1, 2015