SearchLookup to not require MapperService as a constructor argument#64017
Conversation
SearchLookup currently takes the whole MapperService as an argument, and it passes it over to DocLookup and FieldsLookup. In practice, what they all need is only a way to lookup field types given a field name. With this commit, SearchLookup requires a Function<String,MappedFieldType> to be created rather than the whole MapperService. This helps clarifying the dependency between MapperService and SearchLookup, especially as we introduce runtime fields defined in the search request: they can be exposed at QueryShardContext level, through its lookup methods, rather than at the MapperService level which QueryShardContext depends on.
|
Pinging @elastic/es-search (:Search/Mapping) |
This sounds like it could make a lot of tests a lot shorter! |
I have shortened a few, hopefully I have not missed others that I could have shortened, I will have another look just to make sure. |
| service = new ExpressionScriptEngine(); | ||
| lookup = new SearchLookup(mapperService, (ignored, lookup) -> fieldData); | ||
| lookup = new SearchLookup(field -> field.equals("field") || field.equals("alias") ? fieldType : null, | ||
| (ignored, lookup) -> fieldData); |
There was a problem hiding this comment.
I wonder what these were testing before. You change doesn't make it worse though!
I wonder if this change in general could make field aliases simpler. Like, in a follow up.
There was a problem hiding this comment.
I wondered the same, especially why expression tests need to specifically test field aliases, that should not be necessary?
There was a problem hiding this comment.
I'm guessing that aliases at some point were different beasts and the test tested something. Or maybe we ended up copy and pasting stuff?
|
run elasticsearch-ci/default-distro |
| if (this.lookup == null) { | ||
| this.lookup = new SearchLookup( | ||
| mapperService, | ||
| this::getFieldType, |
There was a problem hiding this comment.
this means the lookup does a tiny bit more than before: handling unmapped fields. I wonder if that is good or bad, but tests seem to be happy either way. If we want to preserve the previous behaviour, we can easily do so.
| values.add(mapper.type()); | ||
| } | ||
| if (TypeFieldType.NAME.equals(fieldType.name())) { | ||
| values = Collections.singletonList(((TypeFieldType)fieldType).getType()); |
There was a problem hiding this comment.
this is not fantastic, yet it does the job... do we have better options? I like that this way we still only need the lookup function as argument.
There was a problem hiding this comment.
So I think we can actually remove this in master, as using the _type field in scripts is deprecated in 7.x. But you might want to keep it for the backport, and then remove it later.
romseygeek
left a comment
There was a problem hiding this comment.
LGTM. I particularly like all the lines that are removing Mockito imports, always a good sign!
|
|
||
| DocLookup(MapperService mapperService, Function<MappedFieldType, IndexFieldData<?>> fieldDataLookup) { | ||
| this.mapperService = mapperService; | ||
| DocLookup(Function<String, MappedFieldType> fieldTypeLookup, Function<MappedFieldType, IndexFieldData<?>> fieldDataLookup) { |
There was a problem hiding this comment.
I wonder if we could go even further in a follow up, and just have one String->IndexFieldData lookup?
There was a problem hiding this comment.
interesting idea, and this could be applied to other places too, where all we do is look up the type by name and then look up fielddata by field type. I will look into this.
There was a problem hiding this comment.
I looked into this and I found logic around the field type in the callers, so I will leave this for another time :)
| values.add(mapper.type()); | ||
| } | ||
| if (TypeFieldType.NAME.equals(fieldType.name())) { | ||
| values = Collections.singletonList(((TypeFieldType)fieldType).getType()); |
There was a problem hiding this comment.
So I think we can actually remove this in master, as using the _type field in scripts is deprecated in 7.x. But you might want to keep it for the backport, and then remove it later.
…lastic#64017) SearchLookup currently takes the whole MapperService as an argument, and it passes it over to DocLookup and FieldsLookup. In practice, what they all need is only a way to lookup field types given a field name. With this commit, SearchLookup requires a Function<String,MappedFieldType> to be created rather than the whole MapperService. This helps clarifying the dependency between MapperService and SearchLookup, especially as we introduce runtime fields defined in the search request: they can be exposed at QueryShardContext level, through its lookup methods, rather than at the MapperService level which QueryShardContext depends on.
…64017) SearchLookup currently takes the whole MapperService as an argument, and it passes it over to DocLookup and FieldsLookup. In practice, what they all need is only a way to lookup field types given a field name. With this commit, SearchLookup requires a Function<String,MappedFieldType> to be created rather than the whole MapperService. This helps clarifying the dependency between MapperService and SearchLookup, especially as we introduce runtime fields defined in the search request: they can be exposed at QueryShardContext level, through its lookup methods, rather than at the MapperService level which QueryShardContext depends on.
SearchLookup currently takes the whole MapperService as an argument, and it passes it over to DocLookup and FieldsLookup. In practice, what they all need is only a way to lookup field types given a field name.
With this commit, SearchLookup requires a Function<String,MappedFieldType> to be created rather than the whole MapperService. This helps clarifying the dependency between MapperService and SearchLookup, especially as we introduce runtime fields defined in the search request: they can be exposed at QueryShardContext level, through its lookup methods, rather than at the MapperService level which QueryShardContext depends on.