-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Rename QueryShardContext#fieldMapper to getFieldType #63399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically everywhere that does this null check should be calling |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
source = Intervals.fixField(useField, fieldType.intervals(query, maxGaps, ordered, analyzer, false)); | ||
} | ||
|
@@ -530,7 +530,7 @@ public IntervalsSource getSource(QueryShardContext context, MappedFieldType fiel | |
} | ||
IntervalsSource source; | ||
if (useField != null) { | ||
fieldType = context.fieldMapper(useField); | ||
fieldType = context.getFieldType(useField); | ||
assert fieldType != null; | ||
source = Intervals.fixField(useField, fieldType.intervals(prefix, 0, false, analyzer, true)); | ||
} | ||
|
@@ -645,7 +645,7 @@ public IntervalsSource getSource(QueryShardContext context, MappedFieldType fiel | |
} | ||
IntervalsSource source; | ||
if (useField != null) { | ||
fieldType = context.fieldMapper(useField); | ||
fieldType = context.getFieldType(useField); | ||
assert fieldType != null; | ||
checkPositions(fieldType); | ||
if (this.analyzer == null) { | ||
|
@@ -782,7 +782,7 @@ public IntervalsSource getSource(QueryShardContext context, MappedFieldType fiel | |
} | ||
IntervalsSource source; | ||
if (useField != null) { | ||
fieldType = context.fieldMapper(useField); | ||
fieldType = context.getFieldType(useField); | ||
assert fieldType != null; | ||
checkPositions(fieldType); | ||
if (this.analyzer == null) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.