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

Field list - from indexed array to arrays and maps #47921

Merged
merged 21 commits into from
Oct 14, 2019

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Oct 11, 2019

Summary

Changed field list from indexed array to a native array and maps.

The field list interface is here - https://github.com/elastic/kibana/pull/47921/files#diff-7fdc99852731187e4db7a38a227962abR25

The rest of the pr concerns changes related to consuming the altered api.

[x] Test with beats - thousands of fields

addresses #43440

@mattkime mattkime requested a review from a team as a code owner October 11, 2019 03:45
@mattkime mattkime force-pushed the field_list_not_indexed-array_v2 branch from 142511e to 08ad9ad Compare October 11, 2019 03:54
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattkime mattkime force-pushed the field_list_not_indexed-array_v2 branch from 1b2f34c to febac6e Compare October 12, 2019 02:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@mattkime mattkime added v8.0.0 v7.5.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Oct 12, 2019
@mattkime
Copy link
Contributor Author

retest

@mattkime mattkime requested a review from a team as a code owner October 12, 2019 04:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mattkime mattkime changed the title Field list not indexed array v2 Field list - from indexed array to arrays and maps Oct 12, 2019
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

remove(field: FieldType): void;
}

export class FieldList extends Array<Field> implements FieldListInterface {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we loose extends Array as thats already part of FieldListInterface definition ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to be needed. Seems that FieldList doesn't inherit that trait from FieldListInterface.

(function flatten(obj, keyPrefix = '') {
keyPrefix = keyPrefix ? keyPrefix + '.' : '';
_.forOwn(obj, function(val, key) {
key = keyPrefix + key;

if (deep) {
const isNestedField = fields[key] && fields[key].type === 'nested';
const field = fields(key);
Copy link
Member

Choose a reason for hiding this comment

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

i see .. nice :)

Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

Canvas changes LGTM 👍

@mattkime
Copy link
Contributor Author

@elasticmachine merge master

@mattkime
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mattkime mattkime merged commit b92faf2 into elastic:master Oct 14, 2019
mattkime added a commit to mattkime/kibana that referenced this pull request Oct 14, 2019
@mattkime mattkime added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Oct 14, 2019
mattkime added a commit that referenced this pull request Oct 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukasolson
Copy link
Member

lukasolson commented Oct 14, 2019

I think this PR is the culprit for the following console error I'm seeing now when hovering over the magnifying glasses in Discover for filtering on fields:

Kapture 2019-10-14 at 16 59 08

Reason being is that the field list now contains a direct reference to the index pattern it is created from, which also has the field list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants