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
Completely move doc values and fielddata settings to field types #12014
Conversation
…types While MappedFieldType contains settings for doc values and fielddata, AbstractFieldMapper had special logic in its constructor that required setting these on the field type from there. This change removes those settings from the AbstractFieldMapper constructor. As a result, defaultDocValues(), defaultFieldType() and defaultFieldDataType() are no longer needed.
@@ -81,7 +81,7 @@ public static boolean getSettingsRequireUnits() { | |||
private transient ClassLoader classLoader; | |||
|
|||
Settings(Map<String, String> settings, ClassLoader classLoader) { | |||
this.settings = ImmutableMap.copyOf(settings); | |||
this.settings = ImmutableMap.copyOf(new TreeMap<>(settings)); |
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.
Why do we need a TreeMap here?
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.
This makes the order of iteration consistent for serialization.
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 changed this to ImmutableSortedMap.copyOf, and added a comment that this is needed for consistent serialization. In the future, we can change this to Collections.unmodifiableMap()
Thanks @jpountz, I pushed a new commit. |
LGTM |
Completely move doc values and fielddata settings to field types
While MappedFieldType contains settings for doc values and fielddata,
AbstractFieldMapper had special logic in its constructor that
required setting these on the field type from there. This change
removes those settings from the AbstractFieldMapper constructor.
As a result, defaultDocValues(), and defaultFieldDataType() are
no longer needed, and defaultFieldType() is now an internal detail
of AbstractFieldMapper.