[Maps] support styles on aggregation fields with _of_ in name#51687
[Maps] support styles on aggregation fields with _of_ in name#51687nreese wants to merge 2 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-gis (Team:Geo) |
💚 Build Succeeded |
thomasneirynck
left a comment
There was a problem hiding this comment.
Thanks for simplification, and obviously the fix of a long-standing bug flying under the radar.
See below for comment.
| source: this, | ||
| origin: this.getOriginForField() | ||
| }); | ||
| createField({ fieldName }) { |
There was a problem hiding this comment.
Great simplification! The main consequence now is that EsAggSource#createField is no longer a factory-function and creates inconsistencies with the other implementations of VectorSource#createField.
-
By using the
getMetricFieldForName, which only will return previously created objects, this continues to work as expected because all object-instances are guaranteed to be read-only, so reuse is not an issue. But that's implicit and never enforced (ie. redux requires persistence to always flow through the store, and we enforce this through code-review primarily). -
ESAggSource#createFieldcan only construct fields that are actually available in theEsAggSource, which is inconsistent with all the other implementations. It is a fine limitation, but implementations should be consistent. e.g. consider EMS-sources disallowing creating fields which are not present in the manifest, ES-source disallowing creating fields which are not present in the index-pattern, etc...).
This was the reason we weren't reusing getMetricFieldForName here originally, instead doing a manual lookup for a custom label first.
createFieldcan no longer be used transparently, regardless of caller. e.g. sometimes it is used in the Source#constructor as a shorthand (which would require creation-of-fields from scratch). Other times, it's used from outside (like here, where the bug arises).kibana/x-pack/legacy/plugins/maps/public/layers/styles/vector/vector_style.js
Lines 455 to 457 in 05f765a
Maybe consider (?)
- Introduce something like
VectorSource#getFieldByName.EsAggSourcecan implement it just withgetMetricFieldByName, the other sources can just delegate to usecreateFieldby default. - Modify to
kibana/x-pack/legacy/plugins/maps/public/layers/styles/vector/vector_style.js
Lines 455 to 457 in 05f765a
return this._source.getFieldByName(descriptor.name)
💚 Build Succeeded |
|
Thx @nreese, as discussed offline, I'll follow up with this |
|
closing in favor of #54965 |
PR #50044 refactor fields and introduced Fields classes. The PR added
createFieldmethod toAbstractESAggSourcethat would split the raw field name and aggregation type from the style field name. The problem is that the logic for splitting did not work when the field contained the string_of_. The implementation ofcreateFieldwould have also caused problems ifESTermSourceinstances ever tried to call the method because the split function is tightly coupled toformatMetricKeyimplementation.To view the problem, using the web logs sample data set
hour_of_dayfieldThis PR fixes the problem by removing the need to split the raw field and aggregation type from the field name. This is done by just using the
getMetricFieldForNamefunction which returns fields from the existing metrics list which stores both the raw field name and aggregation type.