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

Fail shards early when we can detect a type missmatch #79869

Merged
merged 12 commits into from
Nov 29, 2021
Merged

Fail shards early when we can detect a type missmatch #79869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
47119d8
Lots of tests!
not-napoleon Oct 26, 2021
7e374a3
slightly cleaner script compile injection
not-napoleon Oct 27, 2021
887bdc1
more VSC resolve tests
not-napoleon Oct 29, 2021
64d82ea
javadoc
not-napoleon Oct 29, 2021
4a0d8e6
Move value type/field conflict detection earlier in the process
not-napoleon Oct 29, 2021
77d621d
formatting fix
not-napoleon Oct 29, 2021
af886a1
Merge branch 'master' into 72276-fail-shard-on-valuetype-missmatch
not-napoleon Oct 29, 2021
04fcb13
formatting fix
not-napoleon Oct 29, 2021
179fcbe
Merge branch 'master' into 72276-fail-shard-on-valuetype-missmatch
not-napoleon Nov 5, 2021
3739806
fix skip version
not-napoleon Nov 5, 2021
08f61ac
Merge branch 'master' into 72276-fail-shard-on-valuetype-missmatch
not-napoleon Nov 29, 2021
345b24a
add another test, this time with docs
not-napoleon Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/20_terms.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,3 +1368,143 @@ huge size:
- match: { aggregations.str_terms.buckets.1.doc_count: 2 }
- match: { aggregations.str_terms.buckets.2.key: c }
- match: { aggregations.str_terms.buckets.2.doc_count: 3 }

---
Value type mismatch fails shard:
- skip:
version: " - 8.0.99"
reason: "Fixed in 8.1"

- do:
indices.create:
index: valuetype_test_1
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
ip:
type: keyword
- do:
indices.create:
index: valuetype_test_2
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
ip:
type: ip

- do:
bulk:
index: valuetype_test_1
refresh: true
body: |
{ "index": {} }
{ "ip": "192.168.7.1" }
{ "index": {} }
{ "ip": "192.168.7.2" }
{ "index": {} }
{ "ip": "192.168.7.3" }

- do:
bulk:
index: valuetype_test_2
refresh: true
body: |
{ "index": {} }
{ "ip": "127.0.0.1" }
{ "index": {} }
{ "ip": "192.168.0.1" }
{ "index": {} }
{ "ip": "192.168.0.2" }
{ "index": {} }
{ "ip": "192.168.0.3" }
- do:
search:
index: valuetype_test_1,valuetype_test_2
body:
size: 0
aggs:
str_terms:
terms:
field: ip
value_type: ip

- match: { _shards.failed: 1 }
- length: { aggregations.str_terms.buckets: 4 }
- match: { aggregations.str_terms.buckets.0.key: "127.0.0.1" }
- match: { aggregations.str_terms.buckets.0.doc_count: 1 }
- match: { aggregations.str_terms.buckets.1.key: "192.168.0.1" }
- match: { aggregations.str_terms.buckets.1.doc_count: 1 }
- match: { aggregations.str_terms.buckets.2.key: "192.168.0.2" }
- match: { aggregations.str_terms.buckets.2.doc_count: 1 }
- match: { aggregations.str_terms.buckets.3.key: "192.168.0.3" }
- match: { aggregations.str_terms.buckets.3.doc_count: 1 }

---
Value type mismatch fails shard with no docs:
- skip:
version: " - 8.0.99"
reason: "Fixed in 8.1"

- do:
indices.create:
index: valuetype_test_1
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
ip:
type: keyword
- do:
indices.create:
index: valuetype_test_2
body:
settings:
number_of_shards: 1
number_of_replicas: 0
mappings:
properties:
ip:
type: ip

- do:
bulk:
index: valuetype_test_2
refresh: true
body: |
{ "index": {} }
{ "ip": "127.0.0.1" }
{ "index": {} }
{ "ip": "192.168.0.1" }
{ "index": {} }
{ "ip": "192.168.0.2" }
{ "index": {} }
{ "ip": "192.168.0.3" }
- do:
search:
index: valuetype_test_1,valuetype_test_2
body:
size: 0
aggs:
str_terms:
terms:
field: ip
value_type: ip

- match: { _shards.failed: 1 }
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 is a special case of the problem, because valuetype_test_1 contains no documents. Wouldn't it be ok to silently skip its shards instead of failing them?

Also, would it make sense to add a test where the index may contain documents?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, there are two problems with silently skipping shards that don't have docs that I can think of. First, we don't know that until later in the process, and by that time we don't have an obvious way to check if we have a conflict. So we'd need to carry some "has type conflict" flag and fail if that's true and we have docs. Seems clunky. It also adds some leniency and unpredictability. If the query changes to include docs on those shards, now the aggregation starts failing. Seems confusing.

Furthermore, if there are no matching docs on the shard, failing it doesn't change the results. It just lets the user know there's a potential problem. More information is better.

And, finally, yes, it makes sense to add a test with docs in both indices. Will push one up shortly. Thanks!

- length: { aggregations.str_terms.buckets: 4 }
- match: { aggregations.str_terms.buckets.0.key: "127.0.0.1" }
- match: { aggregations.str_terms.buckets.0.doc_count: 1 }
- match: { aggregations.str_terms.buckets.1.key: "192.168.0.1" }
- match: { aggregations.str_terms.buckets.1.doc_count: 1 }
- match: { aggregations.str_terms.buckets.2.key: "192.168.0.2" }
- match: { aggregations.str_terms.buckets.2.doc_count: 1 }
- match: { aggregations.str_terms.buckets.3.key: "192.168.0.3" }
- match: { aggregations.str_terms.buckets.3.doc_count: 1 }
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/search/aggregations/support/FieldContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,8 @@ public MappedFieldType fieldType() {
return fieldType;
}

public String getTypeName() {
return fieldType.typeName();
}

}
21 changes: 20 additions & 1 deletion server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,26 @@ private static ValuesSourceConfig internalResolve(
if (valuesSourceType == null) {
// We have a field, and the user didn't specify a type, so get the type from the field
valuesSourceType = fieldResolver.getValuesSourceType(fieldContext, userValueTypeHint, defaultValueSourceType);
}
} else if (valuesSourceType != fieldResolver.getValuesSourceType(fieldContext, userValueTypeHint, defaultValueSourceType)
&& script == null) {
/*
* This is the case where the user has specified the type they expect, but we found a field of a different type.
* Usually this happens because of a mapping error, e.g. an older index mapped an IP address as a keyword. If
* the aggregation proceeds, it will usually break during reduction and return no results. So instead, we fail the
* shard with the conflict at this point, allowing the correctly mapped shards to return results with a partial
* failure.
*
* Note that if a script is specified, the assumption is that the script adapts the field into the specified type,
* and we allow the aggregation to continue.
*/
throw new IllegalArgumentException(
"Field type ["
+ fieldContext.getTypeName()
+ "] is incompatible with specified value_type ["
+ userValueTypeHint
+ "]"
);
}
}
}
if (valuesSourceType == null) {
Expand Down
2 changes: 1 addition & 1 deletion ...t/java/org/elasticsearch/search/aggregations/bucket/terms/BinaryTermsAggregatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void testBadUserValueTypeHint() throws IOException {
ValueType.NUMERIC // numeric type hint
)
);
assertThat(e.getMessage(), equalTo("Expected numeric type on field [binary], but got [binary]"));
assertThat(e.getMessage(), equalTo("Field type [binary] is incompatible with specified value_type [numeric]"));
}

private void testSearchCase(
Expand Down
Loading