Rename QueryShardContext#fieldMapper to getFieldType #63399
Conversation
Given that we have a class called `FieldMapper` and that the `fieldMapper` method exposed by `QueryShardContext` actually allows to get a `MappedFieldType` given its name, this commit renames such method to `getFieldType`
javanna
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
| @@ -108,7 +108,7 @@ public String getWriteableName() { | |||
|
|
|||
| @Override | |||
| protected Query doToQuery(QueryShardContext context) throws IOException { | |||
| MappedFieldType fieldType = context.fieldMapper(field); | |||
| MappedFieldType fieldType = context.getFieldType(field); | |||
There was a problem hiding this comment.
I find some of these usages suspicious: do they really mean to check if the returned field type is null after the unmapped fields handling (unmapped fields may get mapped as text), or would they rather need to call isFieldMapped ?
There was a problem hiding this comment.
I'm pretty sure these should be calling isFieldMapped. We should maybe think about moving the unmapped field handling out of QueryShardContext and have it at the callsites that really need it? It's a bit too magic otherwise.
| @@ -108,7 +108,7 @@ public String getWriteableName() { | |||
|
|
|||
| @Override | |||
| protected Query doToQuery(QueryShardContext context) throws IOException { | |||
| MappedFieldType fieldType = context.fieldMapper(field); | |||
| MappedFieldType fieldType = context.getFieldType(field); | |||
There was a problem hiding this comment.
I'm pretty sure these should be calling isFieldMapped. We should maybe think about moving the unmapped field handling out of QueryShardContext and have it at the callsites that really need it? It's a bit too magic otherwise.
| @@ -152,7 +152,7 @@ public static FieldMaskingSpanQueryBuilder fromXContent(XContentParser parser) t | |||
| @Override | |||
| protected SpanQuery doToQuery(QueryShardContext context) throws IOException { | |||
| String fieldInQuery = fieldName; | |||
| MappedFieldType fieldType = context.fieldMapper(fieldName); | |||
| MappedFieldType fieldType = context.getFieldType(fieldName); | |||
| if (fieldType != null) { | |||
| fieldInQuery = fieldType.name(); | |||
| } | |||
There was a problem hiding this comment.
Basically everywhere that does this null check should be calling isMapped instead.
| @@ -147,7 +147,7 @@ public IntervalsSource getSource(QueryShardContext context, MappedFieldType fiel | |||
| } | |||
| IntervalsSource source; | |||
| if (useField != null) { | |||
| fieldType = context.fieldMapper(useField); | |||
| fieldType = context.getFieldType(useField); | |||
| assert fieldType != null; | |||
There was a problem hiding this comment.
These assertions look dodgy (I think I put them in in the first place, so I'm allowed to say that). It should be throwing an error if passed a field that doesn't exist or doesn't support text searches.
|
run elasticsearch-ci/2 |
Given that we have a class called `FieldMapper` and that the `fieldMapper` method exposed by `QueryShardContext` actually allows to get a `MappedFieldType` given its name, this commit renames such method to `getFieldType`
Given that we have a class called `FieldMapper` and that the `fieldMapper` method exposed by `QueryShardContext` actually allows to get a `MappedFieldType` given its name, this commit renames such method to `getFieldType`
Given that we have a class called
FieldMapperand that thefieldMappermethod exposed byQueryShardContextactually allows to get aMappedFieldTypegiven its name, this commit renames such method togetFieldType