SQL: Perform lazy evaluation of mismatched mappings #35676
Conversation
|
Pinging @elastic/es-search-aggs |
|
/cc @alexfrancoeur @elastic/kibana-canvas |
|
retest this please |
Move away from performing eager, fail-fast validation of mismatched mapping to a lazy evaluation based on the fields actually used in the query. This allows queries to run on the parts of the indices that "work" which is not just convenient but also a necessity for large mappings (like logging) where alignment is hard/impossible to achieve. Fix elastic#35659
| public boolean equals(Object obj) { | ||
| if (super.equals(obj)) { | ||
| InvalidMappedField other = (InvalidMappedField) obj; | ||
| return Objects.equals(errorMessage, other.errorMessage); |
There was a problem hiding this comment.
Minor, but If the objects are equal (this == other) we don't need to check, on the other hand might be ugly to implement completely the super.equals here.
There was a problem hiding this comment.
Right, that's why the call to the equals above.
| } | ||
| FieldCapabilities parentCap = map.values().iterator().next(); | ||
| parent = createField(parentName, globalCaps, hierarchicalMapping, flattedMapping, | ||
| s -> createField(s, parentCap.getType(), new TreeMap<>(), parentCap.isAggregatable())); |
There was a problem hiding this comment.
maybe I'm overlooking something, but why do you need a new TreeMap here, Collections.emptyMap() won't work?
There was a problem hiding this comment.
The map is used for the properties of the parent; a treeMap is used since the children need to be sorted out alphabetically. The field caps does not return fields in order anymore and thus this needs to be maintained internally.
emptyMap is still used for fields that do not have children but the call above is for a parent (which by definition has).
| errorMessage.append("[" + indexPattern + "] points to indices with incompatible mappings: "); | ||
| errorMessage.append("field [" + name + "] is aggregateable except in "); | ||
| errorMessage.append(Arrays.toString(fieldCap.nonAggregatableIndices())); | ||
| errorMessage.insert(0, "mapped as [" + types.size() + "] incompatible types: "); |
There was a problem hiding this comment.
I understand the need to use types.size() as it can be of large values when multiple indices are resolved and used in a query, but would you consider at least for smaller size values - like 1,2,3 - to use the word instead? (one, two, three)
There was a problem hiding this comment.
How would that work? First 1 should never appear so it's about 2, 3 (or whatever limit) and the rest.
This means the message can be "mapped as [three] incompatible types" vs "mapped as [5] incompatible types" since we don't have any utility to convert the numbers into words.
Further more words are never used instead of numbers in any error message that I can think of plus it decreases the readability of the message imo - [5] is shorter and more concise than [five] which arguably should not have the [, ] around.
|
Wooooooooo |
Move away from performing eager, fail-fast validation of mismatched mapping to a lazy evaluation based on the fields actually used in the query. This allows queries to run on the parts of the indices that "work" which is not just convenient but also a necessity for large mappings (like logging) where alignment is hard/impossible to achieve. Fix #35659 (cherry picked from commit 03f0037)
Move away from performing eager, fail-fast validation of mismatched
mapping to a lazy evaluation based on the fields actually used in the
query. This allows queries to run on the parts of the indices that
"work" which is not just convenient but also a necessity for large
mappings (like logging) where alignment is hard/impossible to achieve.
Fix #35659
Note this PR picked up uses #35675 until it gets merged into master (see the first commit) - it's the second commit which is relevant.