Skip to content

Conversation

@javanna
Copy link
Member

@javanna javanna commented Sep 2, 2015

Refactoring of the query_string query: adds serialization bits, validate, equals, hashcode and tests for QueryStringQueryBuilder. Lucene query generation moved to toQuery method. Used QueryParserSettings as intermediate object, created on the data node, to provide arguments to MapperQueryParser for the lucene query parsing.

Relates to #10217

@javanna
Copy link
Member Author

javanna commented Sep 2, 2015

This PR contains a little breakage for the java api, as the QueryStringQueryBuilder#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 QueryStringQueryBuilder#field(String, float) and do field("field", 2);

It also moves to parsing the locale using Locale#forLanguageTag which may cause backwards compatibility issue (see #13229).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these boolean option be boolean rather than Boolean and define a default? or does null mean something for these values?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only few stayed Boolean, I moved everything I could to primitive type. Exceptions are analyzeWildcard and allowLeadingWildcard. null there has a meaning (see toQuery method), as the value needs to be taken from the context, which we have only in toQuery, so we can't use it as a default value instead...

@colings86
Copy link
Contributor

@javanna I left a few comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use isEmpty()?

@s1monw
Copy link
Contributor

s1monw commented Sep 8, 2015

I added some comments

@javanna javanna force-pushed the refactor/query_string branch from a219114 to 5f32534 Compare September 9, 2015 13:15
@javanna
Copy link
Member Author

javanna commented Sep 9, 2015

I have addressed some comments, replied to others, also added a TODO on unifying settings and query options for later on. It should be ready.

@s1monw
Copy link
Contributor

s1monw commented Sep 9, 2015

LGTM

@javanna
Copy link
Member Author

javanna commented Sep 10, 2015

Merged 484fcd4

@javanna javanna closed this Sep 10, 2015
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm a big fan of this. We should at least discuss it further?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a java api change only, you can always specify fields as "field1^5 field2^3" on the rest layer, but when using the java api you now have to do .field("field1", 5).field("field2", 3) instead of being able to do .field("field1^5").field("field2", 3) which makes no sense from a java point of view. There is a better method that accepts two arguments, which is the way to go. I don't think we need to keep the old method that requires us to parse a string to get out the actual field name and the boost value as part of the java api builder, that's something that should happen only as part of query parsing. I think this clarifies the change and addresses your concerns.

MaineC pushed a commit that referenced this pull request Jan 19, 2016
Fix for MatchQueryBuilderTests.testToQuery random fuzziness generation caused test failure.

Relates to #13284
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>breaking :Search/Search Search-related issues that do not fall into other categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants