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

[Maps] support styles on aggregation fields with _of_ in name #51687

Closed
wants to merge 2 commits into from

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Nov 26, 2019

PR #50044 refactor fields and introduced Fields classes. The PR added createField method to AbstractESAggSource that 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 of createField would have also caused problems if ESTermSource instances ever tried to call the method because the split function is tightly coupled to formatMetricKey implementation.

To view the problem, using the web logs sample data set

  1. create a grid source
  2. add a metric aggregation on hour_of_day field
  3. set fill color to use the above aggregation field. Notice that all grids have empty color.

This 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 getMetricFieldForName function which returns fields from the existing metrics list which stores both the raw field name and aggregation type.

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }) {
Copy link
Contributor

@thomasneirynck thomasneirynck Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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).

  2. ESAggSource#createField can only construct fields that are actually available in the EsAggSource, 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.

  1. createField can 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
    return this._source.createField({
    fieldName: fieldDescriptor.name
    });
    ).

Maybe consider (?)

  • Introduce something like VectorSource#getFieldByName. EsAggSource can implement it just with getMetricFieldByName, the other sources can just delegate to use createField by default.
  • Modify
    return this._source.createField({
    fieldName: fieldDescriptor.name
    });
    to return this._source.getFieldByName(descriptor.name)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 15, 2020
@thomasneirynck
Copy link
Contributor

Thx @nreese, as discussed offline, I'll follow up with this

@nreese
Copy link
Contributor Author

nreese commented Jan 15, 2020

closing in favor of #54965

@nreese nreese closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants