Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant BytesQueryBuilder in favour of using WrapperQueryBuilder internally #10919

Conversation

javanna
Copy link
Member

@javanna javanna commented May 1, 2015

BytesQueryBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperQUeryBuilder could be used instead. Added WrapperQueryBuilder constructor that accepts a BytesReference as argument.

One other reason why this query builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only query builder that allows to build a query through java api without having a corresponding query parser.

@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch 3 times, most recently from c2e28dc to b0c3649 Compare May 1, 2015 12:41
@javanna
Copy link
Member Author

javanna commented May 1, 2015

Heya @areek can you please double check that I am not missing anything here? :)

@areek
Copy link
Contributor

areek commented May 1, 2015

Hey @javanna, this looks good! Thanks for fixing this :)

@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch from b0c3649 to 6f380bf Compare May 1, 2015 17:30
javanna added a commit to javanna/elasticsearch that referenced this pull request May 1, 2015
…perFilterBuilder internally

BytesFilterBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperFilterBuilder could be used instead. Added WrapperFilterBuilder constructor that accepts a BytesReference as argument.

One other reason why this filter builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only filter builder that allows to build a filter through java api without having a respective filter parser.

Closes elastic#10919
@areek
Copy link
Contributor

areek commented May 1, 2015

Hey @javanna, I have added a test that breaks with this change. The test along with the fix can be found here https://github.com/elastic/elasticsearch/compare/elastic:master...areek:pr/10919?expand=1

@javanna
Copy link
Member Author

javanna commented May 4, 2015

Hey @areek I did some more testing and compared the two constant score query outputs:

Before:

{
  "constant_score" : {
    "filter" : {
      "query" : {
        "match_phrase" : {
          "title" : "united states houses of representatives elections in washington 2006"
        }
      }
    }
  }
}

After:

{
  "constant_score" : {
    "filter" : {
      "wrapper" : {
        "filter" : "eyJxdWVyeSI6eyJtYXRjaF9waHJhc2UiOnsidGl0bGUiOiJ1bml0ZWQgc3RhdGVzIGhvdXNlcyBvZiByZXByZXNlbnRhdGl2ZXMgZWxlY3Rpb25zIGluIHdhc2hpbmd0b24gMjAwNiJ9fX0="
      }
    }
  }
}

where the binary portion contains the following:

{"query":{"match_phrase":{"title":"united states houses of representatives elections in washington 2006"}}}

There is an additional layer of wrapping, which is expected since we use the wrapper filter, but this looks good to me, I don't see any problem. I pulled in your additional test but didn't seem to fail for me. Let me know what you think.

@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch from 6f380bf to 0310539 Compare May 4, 2015 08:34
javanna added a commit to javanna/elasticsearch that referenced this pull request May 4, 2015
…perFilterBuilder internally

BytesFilterBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperFilterBuilder could be used instead. Added WrapperFilterBuilder constructor that accepts a BytesReference as argument.

One other reason why this filter builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only filter builder that allows to build a filter through java api without having a respective filter parser.

Closes elastic#10919
@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch from 0310539 to 597e80f Compare May 8, 2015 08:52
javanna added a commit to javanna/elasticsearch that referenced this pull request May 8, 2015
…erQueryBuilder internally

BytesQueryBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperQueryBuilder could be used instead. Added WrapperQueryBuilder constructor that accepts a BytesReference as argument.

One other reason why this filter builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only query builder that allows to build a query through java api without having a respective query parser.

Closes elastic#10919
@javanna javanna changed the title Java api: remove redundant BytesFilterBuilder in favour of using WrapperFilterBuilder internally Java api: remove redundant BytesQueryBuilder in favour of using WrapperQueryBuilder internally May 8, 2015
@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch from 597e80f to 56ef4e5 Compare May 8, 2015 08:53
javanna added a commit to javanna/elasticsearch that referenced this pull request May 8, 2015
…erQueryBuilder internally

BytesQueryBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperQueryBuilder could be used instead. Added WrapperQueryBuilder constructor that accepts a BytesReference as argument.

One other reason why this filter builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only query builder that allows to build a query through java api without having a respective query parser.

Closes elastic#10919
…erQueryBuilder internally

BytesQueryBuilder was introduced to be used internally by the phrase suggester and its collate feature. It ended up being exposed via Java api but the existing WrapperQueryBuilder could be used instead. Added WrapperQueryBuilder constructor that accepts a BytesReference as argument.

One other reason why this filter builder should be removed is that it gets on the way of the query parsers refactoring, given that it's the only query builder that allows to build a query through java api without having a respective query parser.

Closes elastic#10919
@javanna javanna force-pushed the enhancement/remove_bytesref_filter_builder branch from 56ef4e5 to 1a70a16 Compare May 8, 2015 08:56
@javanna
Copy link
Member Author

javanna commented May 8, 2015

I just rebased this given that #10985 went in and BytesFilterBuilder became BytesQueryBuilder. Change looks similar though, we still have to wrap it into a constant score query. @jpountz can you please have a look too?

@jpountz
Copy link
Contributor

jpountz commented May 8, 2015

LGTM

@areek
Copy link
Contributor

areek commented May 8, 2015

LGTM!

@javanna javanna closed this in 9e01ded May 9, 2015
@kevinkluge kevinkluge removed the review label May 9, 2015
javanna added a commit to javanna/elasticsearch that referenced this pull request May 12, 2015
…lder

BytesFilterBuilder got removed from master and is now deprecated in 1.x. WrapperFilterBuilder should be used instead.

Relates to elastic#10919
javanna added a commit to javanna/elasticsearch that referenced this pull request May 12, 2015
…lder

BytesFilterBuilder got removed from master and is now deprecated in 1.x. WrapperFilterBuilder should be used instead.

Relates to elastic#10919
Closes elastic#11112
javanna added a commit that referenced this pull request May 12, 2015
BytesFilterBuilder got removed from master and is now deprecated in 1.x. WrapperFilterBuilder should be used instead.

Relates to #10919
Closes #11112
@clintongormley clintongormley changed the title Java api: remove redundant BytesQueryBuilder in favour of using WrapperQueryBuilder internally Remove redundant BytesQueryBuilder in favour of using WrapperQueryBuilder internally Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants