Skip to content

Query refactoring: query_string query#13284

Closed
javanna wants to merge 5 commits intoelastic:feature/query-refactoringfrom
javanna:refactor/query_string
Closed

Query refactoring: query_string query#13284
javanna wants to merge 5 commits intoelastic:feature/query-refactoringfrom
javanna:refactor/query_string

Conversation

@javanna
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

@javanna I left a few comments

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

s1monw commented Sep 9, 2015

LGTM

@javanna
Copy link
Copy Markdown
Contributor Author

javanna commented Sep 10, 2015

Merged 484fcd4

@javanna javanna closed this Sep 10, 2015
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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.

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