Modifying metadata checking in document parser to use mapperRegistry #24473
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
Thanks for the PR. However, as I noted in my comment on #24422, we need the existing meta field methods on MapperService modifed, and META_FIELDS removed. This PR adds a new method and leaves other uses broken. We don't need a new method, we just need the existing method implementations fixed. |
|
Hi @rjernst I have actually modified my modifications, as I finally understood your cue, I suppose injecting the metadatafields from the mapperRegistry into the META_FIELDS at construction time is the easiest way to get these fields. One question though, now that I have 2 different commits, should I squash them together, or create a different PR? Many thanks for the time you take to review my work. |
| @@ -153,6 +153,8 @@ public MapperService(IndexSettings indexSettings, IndexAnalyzers indexAnalyzers, | |||
| this.searchQuoteAnalyzer = new MapperAnalyzerWrapper(indexAnalyzers.getDefaultSearchQuoteAnalyzer(), p -> p.searchQuoteAnalyzer()); | |||
| this.mapperRegistry = mapperRegistry; | |||
|
|
|||
| META_FIELDS = ObjectHashSet.from(mapperRegistry.getMetadataMapperParsers().keySet().toArray(new String[mapperRegistry.getMetadataMapperParsers().keySet().size()])); | |||
There was a problem hiding this comment.
I don't think there is any need to keep META_FIELDS, and this is incorrect do both because META_FIELDS is static right now (so should not be initialized in the ctor) and because it leaves the incorrect values there now. META_FIELDS should be removed completely, and the methods that use it should instead pull the keyset from mapperRegistry.
Making additional commits here is fine. When we have finished iterating and it is ready to merge, we will squash on merge. |
|
@asettouf Are you still interested working on this? |
|
Thanks @asettouf, I will close this PR then. Please reopen in the future if you want to get back to it. |
Fix for issue #24422
Note that I modified a test in
DocumentParserTestsas it was using the field "_ttl" to check the rejection of metadata fields which is still inMETA_FIELDS, however not inmapperRegistry.getMetadataMapperParserswhich seems correct to me given this linkOne last thing is I did not remove the
META_FIELDSusage as for instance it used in GetField and I am not sure how to grab correctly a reference to aMapperService.