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.

Sign up for GitHub

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

Jump to bottom

Remove types from percolate query API #46985

Merged
merged 7 commits into from
Sep 30, 2019
Merged

Remove types from percolate query API #46985

merged 7 commits into from
Sep 30, 2019

Conversation

romseygeek
Copy link
Contributor

Relates to #41059

@romseygeek romseygeek added :Search Relevance/Percolator Reverse search: find queries that match a document >refactoring v8.0.0 labels Sep 23, 2019
@romseygeek romseygeek self-assigned this Sep 23, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@romseygeek
Copy link
Contributor Author

@elasticsearch update branch

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

@romseygeek
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@romseygeek
Copy link
Contributor Author

This ILM test keeps failing, but it seems to be completely unrelated and doesn't reproduce locally for me at all:

./gradlew ':x-pack:plugin:ilm:qa:rest:restTestRunner' --tests "org.elasticsearch.xpack.ilm.IndexLifecycleRestIT.test {yaml=ilm/40_explain_lifecycle/Test All Indexes Lifecycle Explain}" -Dtests.seed=A02B76EAD235AF79

@elasticmachine run elasticsearch-ci/2

jtibshirani
jtibshirani reviewed Sep 25, 2019
View reviewed changes
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java Outdated
this(field, Collections.singletonList(document), documentXContentType);
}

private PercolateQueryBuilder(String field, BytesReference document) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this constructor is unused.

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java Outdated
@@ -354,7 +332,7 @@ public void testSettingNameWhileRewritingWhenDocumentSupplierAndSourceNotNull()
Supplier<BytesReference> supplier = () -> new BytesArray("{\"test\": \"test\"}");
String testName = "name1";
QueryShardContext shardContext = createShardContext();
PercolateQueryBuilder percolateQueryBuilder = new PercolateQueryBuilder(queryField, null, supplier);
PercolateQueryBuilder percolateQueryBuilder = new PercolateQueryBuilder(queryField, supplier::get);
Copy link
Contributor

Choose a reason for hiding this comment

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

Super small comment, could this just be new PercolateQueryBuilder(queryField, supplier)?

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
/**
* Read from a stream.
*/
PercolateQueryBuilder(StreamInput in) throws IOException {
super(in);
field = in.readString();
name = in.readOptionalString();
documentType = in.readOptionalString();
if (in.getVersion().before(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this query type, a 7.x 'typeless' query is represented by a null documentType and indexedDocumentType. So I think that here and in doWriteTo, we should always assume these parameters are null instead of _doc.

@jpountz jpountz mentioned this pull request Sep 26, 2019
Closed
66 tasks
jtibshirani
jtibshirani reviewed Sep 26, 2019
View reviewed changes
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java Outdated
@@ -281,9 +245,13 @@ protected void doWriteTo(StreamOutput out) throws IOException {
}
out.writeString(field);
out.writeOptionalString(name);
out.writeOptionalString(documentType);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to my comment on the previous revision, I think we should write null here. In 7.x, a 'typeless' percolate query is represented by a null documentType and indexedDocumentType.

@romseygeek romseygeek merged commit 47bf259 into elastic:master Sep 30, 2019
@romseygeek romseygeek deleted the types-removal/percolate-query branch September 30, 2019 07:49
@pgomulka pgomulka mentioned this pull request Mar 25, 2020
Closed
66 tasks
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jun 29, 2021
@pgomulka
[Rest Api Compatibility] Types for Percolate Query Api
92afc0e 
Previously removed in elastic#46985. The yaml test is included in this PR, but
will be removed once elastic#74689 is merged.
@pgomulka pgomulka mentioned this pull request Jun 29, 2021
Merged
pgomulka added a commit that referenced this pull request Jun 30, 2021
@pgomulka
[Rest Api Compatibility] Types for Percolate Query Api (#74698)
603ebf7 
Previously removed in #46985. The yaml test is included in this PR, but
will be removed once #74689 is merged.

relates #54160
relates main meta issue #51816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtibshirani jtibshirani jtibshirani approved these changes

@jpountz jpountz Awaiting requested review from jpountz

Assignees

@romseygeek romseygeek

Labels
>refactoring :Search Relevance/Percolator Reverse search: find queries that match a document v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@romseygeek @elasticmachine @jtibshirani @jakelandis