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

Adds `ignore_unmapped` option to nested and P/C queries #17748

Merged
merged 1 commit into from Apr 14, 2016

Conversation

Projects
None yet
4 participants
@colings86
Copy link
Member

commented Apr 14, 2016

The change adds a new option to the nested, has_parent, has_children and parent_id queries: ignore_unmapped. If this option is set to false, the toQuery method on the QueryBuilder will throw an exception if the type/path specified in the query is unmapped. If the option is set to true, the toQuery method on the QueryBuilder will return a MatchNoDocsQuery. The default value is falseso the queries work how they do today (throwing an exception on unmapped paths/types)

@jpountz

View changes

core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java Outdated
fail("expected has_children query on unmapped field to fail when ignore_unmapped is false");
} catch (QueryShardException e) {
assertThat(e.getMessage(), containsString("[" + HasChildQueryBuilder.NAME + "] no mapping found for type [unmapped]"));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

Can you use expectThrows instead? I think we should use it rather than this try:fail/catch:check_msg pattern for new code.

@jpountz

View changes

core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java Outdated
} catch (QueryShardException e) {
assertThat(e.getMessage(),
containsString("[" + HasParentQueryBuilder.NAME + "] query configured 'parent_type' [unmapped] is not a valid type"));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

can you use expectThrows?

@jpountz

View changes

core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java Outdated
} catch (IllegalStateException e) {
assertThat(e.getMessage(),
containsString("[" + NestedQueryBuilder.NAME + "] failed to find nested object under path [unmapped]"));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

can you use expectThrows?

@jpountz

View changes

core/src/test/java/org/elasticsearch/index/query/ParentIdQueryBuilderTests.java Outdated
fail("expected parent_id query on unmapped field to fail when ignore_unmapped is false");
} catch (QueryShardException e) {
assertThat(e.getMessage(), containsString("[" + ParentIdQueryBuilder.NAME + "] no mapping found for type [unmapped]"));
}

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

can you use expectThrows?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2016

A minor concern I have is that ignore_unmapped: false is a double negation, which could get confusing. Otherwise the change looks good and well tested.

@martijnvg

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

maybe we should name this setting fail_unmapped? (and then default to true)

@colings86

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2016

I'm ok with changing it to fail_unmapped although we seem to use ignore_* elsewhere in the api including ignore_malformed which is a bit of a double negation too. What do you think @jpountz and @clintongormley ?

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

ignore_unmapped defaults to false, so the only time the user will use this will be to set ignore_unmapped: true, which I think is OK.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

Docs LGTM

@jpountz

View changes

core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java Outdated
@@ -464,12 +502,12 @@ protected boolean doEquals(HasChildQueryBuilder that) {
&& Objects.equals(scoreMode, that.scoreMode)
&& Objects.equals(minChildren, that.minChildren)
&& Objects.equals(maxChildren, that.maxChildren)
&& Objects.equals(innerHitBuilder, that.innerHitBuilder);
&& Objects.equals(innerHitBuilder, that.innerHitBuilder) && Objects.equals(ignoreUnmapped, that.ignoreUnmapped);

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

let's put it on the next line to be consistent?

@jpountz

View changes

core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java Outdated
@@ -286,12 +323,12 @@ protected boolean doEquals(HasParentQueryBuilder that) {
return Objects.equals(query, that.query)
&& Objects.equals(type, that.type)
&& Objects.equals(score, that.score)
&& Objects.equals(innerHit, that.innerHit);
&& Objects.equals(innerHit, that.innerHit) && Objects.equals(ignoreUnmapped, that.ignoreUnmapped);

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

next line?

@jpountz

View changes

core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java Outdated
@@ -208,19 +243,23 @@ protected boolean doEquals(NestedQueryBuilder that) {
return Objects.equals(query, that.query)
&& Objects.equals(path, that.path)
&& Objects.equals(scoreMode, that.scoreMode)
&& Objects.equals(innerHitBuilder, that.innerHitBuilder);
&& Objects.equals(innerHitBuilder, that.innerHitBuilder) && Objects.equals(ignoreUnmapped, that.ignoreUnmapped);

This comment has been minimized.

Copy link
@jpountz

jpountz Apr 14, 2016

Contributor

next line?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2016

LGTM

Adds ignore_unmapped option to nested and P/C queries
The change adds a new option to the `nested`, `has_parent`, `has_children` and `parent_id` queries: `ignore_unmapped`. If this option is set to false, the `toQuery` method on the QueryBuilder will throw an exception if the type/path specified in the query is unmapped. If the option is set to true, the `toQuery` method on the QueryBuilder will return a MatchNoDocsQuery. The default value is `false`so the queries work how they do today (throwing an exception on unmapped paths/types)

@colings86 colings86 force-pushed the colings86:enhance/ignoreUnmappedNestedPC branch to 686aff1 Apr 14, 2016

@colings86 colings86 merged commit 686aff1 into elastic:master Apr 14, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@colings86 colings86 deleted the colings86:enhance/ignoreUnmappedNestedPC branch Apr 14, 2016

This was referenced Apr 14, 2016

@clintongormley clintongormley changed the title Adds ignore_unmapped option to nested and P/C queries Adds `ignore_unmapped` option to nested and P/C queries May 2, 2016

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 14, 2016

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.