-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Avoid using dynamic templates for semantic text fields #108771
Avoid using dynamic templates for semantic text fields #108771
Conversation
Pinging @elastic/es-search (Team:Search) |
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.
Refactored this class to make it a base test case
@@ -143,66 +137,4 @@ public void testBulkOperations() throws Exception { | |||
} | |||
} | |||
|
|||
private void storeSparseModel() throws Exception { |
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.
Refactored out some utility classes to be reused
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.
LGTM
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.
LGTM! Left one comment about potentially improving test ergonomics.
|
||
protected abstract String getTypeName(); | ||
|
||
protected abstract String getMapping(); |
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.
Nit: The format of the string returned by this method is quite specific and could trip people up in the future. Would a Map<String, Object>
return type here work? We could then convert it to JSON in the abstract class. WDYT?
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've been thinking about this too - I'd like to have more use cases before reaching a decision about the format. I don't see a particular advantage on using something similar to MapperTestCase.minimalMapping()
, at least for now.
Avoids
semantic_text
fields to be used in dynamic templates.We need the
semantic_text
fields to be already in the mapping so we can perform inference, so we're disallowing it based on the work done in #107491 .I've refactored some of the existing tests so we can ensure that the field mapper does comply with the requirements, and created some utilities for dealing with inference services.