-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add null-field checks to shape field mappers #71999
Conversation
Pinging @elastic/es-search (Team:Search) |
Marking as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
@@ -691,4 +691,16 @@ public final void testIndexTimeStoredFieldsAccess() throws IOException { | |||
.fielddataBuilder("test", lookupSource) | |||
.build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService()); | |||
} | |||
|
|||
public final void testNullInput() throws Exception { | |||
assumeTrue("Mapper does not permit null values", allowsNullValues()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we actually check that when the mapper does not allow null values, an exception is thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good idea. I think I'm going to revert the changes to rank_features and dense_vectors as well and add them as TODOs because I want to get this into 7.13 and they should get some more discussion.
@javanna I updated the check to account for exception-throwing cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
elastic#71696 introduced a regression to the various shape field mappers, where they would no longer handle null values. This commit fixes that regression and adds a testNullValues method to MapperTestCase to ensure that all field mappers correctly handle nulls. Fixes elastic#71874
#71696 introduced a regression to the various shape field mappers,
where they would no longer handle null values. This commit fixes
that regression and adds a
testNullValues
method to MapperTestCaseto ensure that all field mappers correctly handle nulls.
Fixes #71874