Skip to content
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

Merged
merged 3 commits into from Jul 6, 2015

Conversation

@rjernst
Copy link
Member

commented Jul 3, 2015

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.

…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));

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 6, 2015

Contributor

Why do we need a TreeMap here?

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 6, 2015

Author Member

This makes the order of iteration consistent for serialization.

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 6, 2015

Author Member

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()

@@ -84,6 +85,7 @@
protected Builder(String name, MappedFieldType fieldType) {
super(name);
this.fieldType = fieldType.clone();
this.defaultFieldType = fieldType.clone();

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 6, 2015

Contributor

Should we freeze() it in build()?

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 6, 2015

Author Member

Yes, good catch!

MultiFields multiFields, CopyTo copyTo) {
// LUCENE 4 UPGRADE: Since we can't do anything before the super call, we have to push the boost check down to subclasses

This comment has been minimized.

Copy link
@jpountz

jpountz Jul 6, 2015

Contributor

thanks for removing this one :)

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2015

Thanks @jpountz, I pushed a new commit.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2015

LGTM

rjernst added a commit that referenced this pull request Jul 6, 2015
Completely move doc values and fielddata settings to field types
@rjernst rjernst merged commit ff0f480 into elastic:master Jul 6, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@rjernst rjernst deleted the rjernst:remove/mapper-doc-values branch Jul 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.