Remove ValueFetcher depedendency from MapperService #64524

javanna · 2020-11-03T12:24:50Z

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-java because it changes the signature of the MappedFieldType#valueFetcher method to take QueryShardContext as argument instead of MapperService. 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.

elasticmachine · 2020-11-03T12:24:52Z

Pinging @elastic/es-search (:Search/Mapping)

romseygeek

LGTM, very nice.

jimczi

I left one comment

jimczi · 2020-11-03T12:43:21Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

Why not using QueryShardContext ? We could also remove the SearchLookup argument.

Why not using QueryShardContext ? We could also remove the SearchLookup argument.

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.

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.

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.

+1 to solve this as a follow up.

nik9000 · 2020-11-03T12:46:46Z

server/src/main/java/org/elasticsearch/index/mapper/ValueFetcher.java

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.

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.

jimczi · 2020-11-03T16:22:36Z

I think it's important to be consistent here. IMO the QueryShardContext is the lightweight object that we use when parsing/building/fetching in the query or the fetch phase so it's logic to expose it entirely. Look at the existing query methods in MappedFieldType, they already expose the QueryShardContext. We could argue that they don't need the entire context but that would make the API more complex to use.
My point here is that the QSC is the right abstraction to expose for consistency. We can ensure that nothing unsafe leaks in a central point.

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.

romseygeek · 2020-11-03T17:37:51Z

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java


        @Override
-        public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
+        public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchLookup, String format) {


If we're passing QueryShardContext I think that means we can avoid passing SearchLookup here?

see my comment above, I don't think so, because we need to pass through the fetch lookup that is created in FetchContext?

romseygeek

One nit, but LGTM otherwise.

romseygeek · 2020-11-04T10:05:29Z

server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java

+        return mapperService.isMetadataField(field);
+    }
+
+    public Set<String> sourcePath(String fullName) {


One nit: maybe this should be sourcePaths, given that it can return more than one?

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?

+1 for a followup

Relates to #64524

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

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.

javanna added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.11.0 labels Nov 3, 2020

javanna requested review from nik9000 and romseygeek November 3, 2020 12:24

elasticmachine added the Team:Search Meta label for search team label Nov 3, 2020

romseygeek approved these changes Nov 3, 2020

View reviewed changes

javanna requested a review from jtibshirani November 3, 2020 12:33

jimczi reviewed Nov 3, 2020

View reviewed changes

nik9000 reviewed Nov 3, 2020

View reviewed changes

javanna force-pushed the refactoring/value_fetcher_mapper_service branch from 362aceb to e2462f5 Compare November 3, 2020 16:29

fix test

db473a5

romseygeek reviewed Nov 3, 2020

View reviewed changes

javanna requested a review from romseygeek November 4, 2020 09:57

romseygeek approved these changes Nov 4, 2020

View reviewed changes

javanna merged commit 344ad33 into elastic:master Nov 4, 2020

javanna added a commit that referenced this pull request Nov 4, 2020

Adapt DocCountFieldType#valueFetcher signature

263ea66

Relates to #64524

javanna added the backport pending label Nov 4, 2020

javanna mentioned this pull request Nov 5, 2020

[7.x] Add doc_count field mapper #64594

Merged

javanna removed the backport pending label Nov 5, 2020

csoulios added a commit to csoulios/elasticsearch that referenced this pull request Nov 5, 2020

Fixed conflict introduced after merging elastic#64524

4ed2309

jtibshirani reviewed Nov 5, 2020

View reviewed changes

javanna added the >breaking-java label Nov 6, 2020

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

Remove ValueFetcher depedendency from MapperService #64524

Remove ValueFetcher depedendency from MapperService #64524

Uh oh!

Conversation

javanna commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 3, 2020

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi commented Nov 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

javanna commented Nov 3, 2020 •

edited

Loading