Fix numerous checks for equality and compatibility in mapper field types #13206
Conversation
The field type tests for mappings had a huge hole: check compatibility was not tested directly at all! I had meant for this to happen in a follow up after elastic#8871, and was relying on existing mapping tests. However, there were a number of issues. This change reworks the fieldtype tests to be able to check all settable properties on a field type work with checkCompatibility. It fixes a handful of small bugs in various field types. In particular, analyzer comparison was just wrong: it was comparing reference equality for search analyzer instead of the analyzer name. There was also no check for search quote analyzer. closes elastic#13112
rjernst
added
>bug
:Search Foundations/Mapping
| if (!(o instanceof NamedAnalyzer)) return false; | ||
| NamedAnalyzer that = (NamedAnalyzer) o; | ||
| return Objects.equals(name, that.name); | ||
| } |
There was a problem hiding this comment.
I don't have a better idea, but this makes me slightly nervous since nothing in the API prevents you from doing
Analyzer a1 = new NamedAnalyzer("foo", someAnalyzer);
Analyzer a2 = new NamedAnalyzer("foo", otherAnalyzer);There was a problem hiding this comment.
Since an analyzer is essentially code (made up of bundles of other code like tokenizers and token filters), I don't think there is really a way to make them comparable. So I'm not sure there is really anything we can do, the name is the only comparable identifier we have.
|
I left one question about the equality checks but I don't have better ideas... This is good progress compared to what we have in master though so feel free to push even if you don't have ideas how to improve it either. |
Fix numerous checks for equality and compatibility in mapper field types
|
I will backport this to 2.0 as well, but am waiting on the backport of some geo changes that will make the backport clean. |
Fix numerous checks for equality and compatibility in mapper field types
|
Here is the 2.0 commit: 9e10e88 |
The field type tests for mappings had a huge hole: check compatibility
was not tested directly at all! I had meant for this to happen in a
follow up after #8871, and was relying on existing mapping tests.
However, there were a number of issues.
This change reworks the fieldtype tests to be able to check all settable
properties on a field type work with checkCompatibility. It fixes a
handful of small bugs in various field types. In particular, analyzer
comparison was just wrong: it was comparing reference equality for
search analyzer instead of the analyzer name. There was also no check
for search quote analyzer.
closes #13112