Remove ValueFetcher depedendency from MapperService#64524
Remove ValueFetcher depedendency from MapperService#64524javanna merged 2 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-search (:Search/Mapping) |
There was a problem hiding this comment.
Why not using QueryShardContext ? We could also remove the SearchLookup argument.
There was a problem hiding this comment.
Why not using
QueryShardContext? We could also remove theSearchLookupargument.
I'd prefer not to use QueryShardContext everywhere. It really is "too big" as it is.
For what it is worth, we could totally add SearchLookup to ValueFetcher.Context if we wanted.
But I've made three attempts at this change so far and we didn't like them so I'm kind of done having opinions here. Do whatever makes folks happy.
There was a problem hiding this comment.
QueryShardContext is a huge object that I don't think we want to spread around more than it already is. While we remove the dependency from MapperService, I think that we should try and be careful with encapsulation and who gets access to what. sourcePath is currently not exposed anywhere other than MapperService, and we will have to find a way to expose it so we can progress with removing direct MapperService usages, but we want to be careful with adding new methods to all of the existing contexts, that already have many methods.
There was a problem hiding this comment.
I updated and added QueryShardContext as an argument. I am not sure about SearchLookup because it needs to retrieved specifically through QueryShardContext#newFetchLookup and reused. Looks like we should improve this as its confusing that you can get the lookup also from the context, though it would be the wrong one. I will leave this for another time though.
There was a problem hiding this comment.
+1 to solve this as a follow up.
There was a problem hiding this comment.
I'm not sure that this is better than my last attempt. I think it's up to @jtibshirani to make the call on this one.
There was a problem hiding this comment.
I think that the difference is subtle, but I believe this should address the concerns around tracking usages. At the same time we are introducing a lightweight abstraction that clearly encapsulates what is needed to fetch values, which is much harder to read when functions are carried around. I would love for Julie to have a look and let us know what she thinks though.
|
I think it's important to be consistent here. IMO the |
The signature of MappedFieldType#valueFetcher requires MapperService as an argument which is unfortunate as that is one of the reasons why FetchContext exposes the whole MapperService. Such use of MapperService can be replaced with exposing the QueryShardContext which encapsulates the MapperService.
362aceb to
e2462f5
Compare
|
|
||
| @Override | ||
| public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) { | ||
| public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchLookup, String format) { |
There was a problem hiding this comment.
If we're passing QueryShardContext I think that means we can avoid passing SearchLookup here?
There was a problem hiding this comment.
see my comment above, I don't think so, because we need to pass through the fetch lookup that is created in FetchContext?
romseygeek
left a comment
There was a problem hiding this comment.
One nit, but LGTM otherwise.
| return mapperService.isMetadataField(field); | ||
| } | ||
|
|
||
| public Set<String> sourcePath(String fullName) { |
There was a problem hiding this comment.
One nit: maybe this should be sourcePaths, given that it can return more than one?
There was a problem hiding this comment.
yes, in my iterations I did both, I went for singular for consistency with the name of the method in MapperService. I don't know what is more important between using the same name or using the correct name here. Maybe we should rename both as a followup?
The signature of MappedFieldType#valueFetcher requires MapperService as an argument which is unfortunate as that is one of the reasons why FetchContext exposes the whole MapperService. Such use of MapperService can be replaced with exposing the QueryShardContext which encapsulates the MapperService.
The signature of MappedFieldType#valueFetcher requires MapperService as an argument which is unfortunate as that is one of the reasons why FetchContext exposes the whole MapperService. Such use of MapperService can be replaced with exposing the QueryShardContext which encapsulates the MapperService.
jtibshirani
left a comment
There was a problem hiding this comment.
Sorry for the slow review, I like the approach taken in the PR to pass in QueryShardContext. +1 to think about a follow-up refactor where we avoid passing around QueryShardContext and SearchLookup separately.
ValueFetcher needs only one method from MapperService: sourcePaths. The signature of MappedFieldType#valueFetcher requires MapperService as an argument which is unfortunate as that is one of the reasons why FetchContext exposes the whole MapperService.
This can be easily removed by having a specific lightweight object to carry around instead of the MapperService, that exposes only the needed method.
This change is marked
breaking-javabecause it changes the signature of theMappedFieldType#valueFetchermethod to takeQueryShardContextas argument instead ofMapperService.MapperPlugins that plug in a custom field type implementation will have to adapt their code to make it compile with Elasticsearch 7.11 and following versions.