Always support ignore_malformed in the same way#90565
Merged
nik9000 merged 4 commits intoelastic:mainfrom Oct 3, 2022
Merged
Conversation
Collaborator
|
Pinging @elastic/es-search (Team:Search) |
Member
Author
|
My ulterior motive here is that I can reuse this infrastructure when we support |
This makes sure that all field types that support `ignore_malfored` do
so in the same way.
Production changes:
* All mapper has an `ignoreMalformed` method that must return `true` if
the field accepts the `ignore_malformed` mapping parameter was
configured. It defaults to `false` because many fields either don't
have a concept of "malformed" value or don't have the ability to
ignore malformed values.
* Fix the `scaled_float` field to store it's field name in `_ignored` if
it ignores any malfored values. This is how all other field mappers
work.
Test changes:
* `MapperTestCase` forces subclasses to declare if their
`supportIgnoreMalformed` or not.
* If `MapperTestCase` subclasses `supportIgnoreMalfored` they must
define some `exampleMalformedValues`.
* `MapperTestCase` always grows three new tests:
* One that creates a field without setting `ignore_malformed` and
verifies that all `exampleMalformedValues` throw expected errors
* On that explicitly configured `ignore_malformed` to false and, if
`supportIgnoreMalformed` it verifies the errors again. If not
`supportIgnoreMalformed` it verifies that the parameter is unknown.
* On that explicitly configured `ignore_malformed` to true and, if
`supportIgnoreMalformed` it verifies that parsing doesn't produce
errors and correctly produces `_ignored`. If not
`supportIgnoreMalformed` it verifies that the parameter is unknown.
* Moved some subclasesses of `MapperTestCase` from
`internalClusterTests` to `tests`. This isn't strictly required but
that's the right place for them.
romseygeek
approved these changes
Sep 30, 2022
Contributor
romseygeek
left a comment
There was a problem hiding this comment.
This is great, thanks @nik9000
I left one question around how aggregate metric double records ignored values, but we can handle that in a follow-up if it's not one with an obvious answer.
|
|
||
| if (BuildParams.isSnapshotBuild() == false) { | ||
| tasks.named("internalClusterTest").configure { | ||
| tasks.named("test").configure { |
Contributor
There was a problem hiding this comment.
This has been annoying me for ages, thanks for fixing
test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| protected String[] ignoredFields() { | ||
| return new String[] { "field.value_count", "field.min", "field.max" }; |
Contributor
There was a problem hiding this comment.
This is interesting - the _ignored field is really for user inspection, so I'm not sure if this mapper should be recording all its subfields in it as they are implementation details.
Member
Author
There was a problem hiding this comment.
Yeah! I think that's worth discussing somewhere. I can file an issue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes sure that all field types that support
ignore_malforeddo so in the same way.Production changes:
ignoreMalformedmethod that must returntrueif the field accepts theignore_malformedmapping parameter was configured. It defaults tofalsebecause many fields either don't have a concept of "malformed" value or don't have the ability to ignore malformed values.scaled_floatfield to store it's field name in_ignoredif it ignores any malfored values. This is how all other field mappers work.Test changes:
MapperTestCaseforces subclasses to declare if theirsupportIgnoreMalformedor not.MapperTestCasesubclassessupportIgnoreMalforedthey must define someexampleMalformedValues.MapperTestCasealways grows three new tests:ignore_malformedand verifies that allexampleMalformedValuesthrow expected errorsignore_malformedto false and, ifsupportIgnoreMalformedit verifies the errors again. If notsupportIgnoreMalformedit verifies that the parameter is unknown.ignore_malformedto true and, ifsupportIgnoreMalformedit verifies that parsing doesn't produce errors and correctly produces_ignored. If notsupportIgnoreMalformedit verifies that the parameter is unknown.MapperTestCasefrominternalClusterTeststotests. This isn't strictly required but that's the right place for them.exampleMalformedValues