MappedFieldType no longer requires equals/hashCode/clone #59212
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
| eagerGlobalOrdinals, meta); | ||
| } | ||
|
|
||
| // TODO: we need to override freeze() and add safety checks that all settings are actually set |
There was a problem hiding this comment.
This TODO doesn't make sense now that we don't extend lucene's FieldType
| protected MappedFieldType createDefaultFieldType(String name, Map<String, String> meta) { | ||
| return new BinaryFieldMapper.BinaryFieldType(name, true, meta); | ||
| } | ||
| } |
There was a problem hiding this comment.
FieldTypeTestCase extensions that test only equals and hashCode can just be removed entirely. BinaryFieldType doesn't have any search methods on it, and docvalues are tested in BinaryFieldMapperTests
|
|
||
| public class TextFieldTypeTests extends FieldTypeTestCase<TextFieldType> { | ||
|
|
||
| @Before |
There was a problem hiding this comment.
We check that these modifications block merging in TextFieldMapperTests, so we can safely remove them here - this is just checking the equals implementation, which is now gone.
There was a problem hiding this comment.
👍. That is a better place for them.
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
This is a nice simplification! I left one comment about an accidental change in a test.
I was also curious, do we have a standard practice as to when we implement equals/ hashCode versus omitting them? We certainly implement these on request and response objects, but I think we make a case-by-case choice for internal objects like MappedFieldType ?
|
|
||
| public void testIsAggregatable() { | ||
| MappedFieldType fieldType = createDefaultFieldType("field", Collections.emptyMap()); | ||
| MappedFieldType fieldType = new RankFeatureFieldMapper.RankFeatureFieldType("field", Collections.emptyMap(), true); |
There was a problem hiding this comment.
I don't think this should have changed, now we're creating RankFeatureFieldType (singular not plural)?
There was a problem hiding this comment.
Oh, good catch! Autocomplete is not always my friend...
I think we only do this one request and response objects so we can assert round tripping them. |
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
This is great! @jtibshirani found something worth fixing before merging, but it is wonderful to see so much subtracted code!
| this.positiveScoreImpact = ref.positiveScoreImpact; | ||
| } | ||
|
|
||
| public RankFeatureFieldType clone() { |
| eagerGlobalOrdinals, meta); | ||
| } | ||
|
|
||
| // TODO: we need to override freeze() and add safety checks that all settings are actually set |
| if (o == null || getClass() != o.getClass()) return false; | ||
| if (!super.equals(o)) return false; | ||
| PrefixFieldType that = (PrefixFieldType) o; | ||
| return minChars == that.minChars && |
There was a problem hiding this comment.
Yikes! This was never really equality!
|
|
||
| public class TextFieldTypeTests extends FieldTypeTestCase<TextFieldType> { | ||
|
|
||
| @Before |
There was a problem hiding this comment.
👍. That is a better place for them.
|
|
||
| /** Base test case for subclasses of MappedFieldType */ | ||
| public abstract class FieldTypeTestCase<T extends MappedFieldType> extends ESTestCase { | ||
| public abstract class FieldTypeTestCase extends ESTestCase { |
There was a problem hiding this comment.
It might be worth dropping this in a followup. It is just about empty now.
There was a problem hiding this comment.
The mock shard contexts are still pretty widely used, so I think it's worth keeping for now.
|
@elasticmachine update branch |
With the removal of mapping types and the immutability of FieldTypeLookup in #58162, we no longer have any cause to compare MappedFieldType instances. This means that we can remove all equals and hashCode implementations, and in addition we no longer need the clone implementations which were required for equals/hashcode testing. This greatly simplifies implementing new MappedFieldTypes, which will be particularly useful for the runtime fields project.
The MappedFieldType#updateMeta method was used for testing equality checks, but we no longer need these after #59212 , so we can remove this method and make meta final.
The MappedFieldType#updateMeta method was used for testing equality checks, but we no longer need these after #59212 , so we can remove this method and make meta final.
6 participants
With the removal of mapping types and the immutability of FieldTypeLookup in #58162, we no longer
have any cause to compare MappedFieldType instances. This means that we can remove all equals
and hashCode implementations, and in addition we no longer need the clone implementations which
were required for equals/hashcode testing. This greatly simplifies implementing new MappedFieldTypes,
which will be particularly useful for the runtime fields project.