-
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
Simplify the return type of FieldMapper#parse. #32654
Conversation
Pinging @elastic/es-search-aggs |
b7035ff
to
19f0a06
Compare
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.
@jtibshirani makes sense to me, I took a quick look and only left one comment regarding changing of a javadoc comment. LGTM otherwise.
@@ -268,7 +268,7 @@ public CopyTo copyTo() { | |||
* update if dynamic mappings modified the mappings, or {@code null} if | |||
* mappings were not modified. | |||
*/ | |||
public Mapper parse(ParseContext context) throws IOException { | |||
public void parse(ParseContext context) throws IOException { |
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.
I think it makes sense to also update the method comment according to the change.
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.
Oops, thanks! Will update.
19f0a06
to
970dcfa
Compare
…e-default-distribution * elastic/master: (213 commits) ML: Fix build after HLRC change Fix inner hits retrieval when stored fields are disabled (_none_) (elastic#33018) SQL: Show/desc commands now support table ids (elastic#33363) Mute testValidateFollowingIndexSettings HLRC: Add delete by query API (elastic#32782) [ML] The sort field on get records should default to the record_score (elastic#33358) [ML] Minor improvements to categorization Grok pattern creation (elastic#33353) [DOCS] fix a couple of typos (elastic#33356) Disable assemble task instead of removing it (elastic#33348) Simplify the return type of FieldMapper#parse. (elastic#32654) [ML] Delete forecast API (elastic#31134) (elastic#33218) Introduce private settings (elastic#33327) [Docs] Add search timeout caveats (elastic#33354) TESTS: Fix Race Condition in Temp Path Creation (elastic#33352) Fix from_range in search_after in changes snapshot (elastic#33335) TESTS+DISTR.: Fix testIndexCheckOnStartup Flake (elastic#33349) Null completion field should not throw IAE (elastic#33268) Adds code to help with IndicesRequestCacheIT failures (elastic#33313) Prevent NPE parsing the stop datafeed request. (elastic#33347) HLRC: Add ML get overall buckets API (elastic#33297) ...
No implementation of this method returned a non-null value. It looks like the dynamic creation of field mappers is fully handled by
DocumentParser#parseDynamicValue
.