removing schemas from agg configs by ppisljar · Pull Request #58462 · elastic/kibana

ppisljar · 2020-02-25T12:28:05Z

Summary

removes schemas dependency from aggconfigs

saved object format was not changed, schema: string still exists on serialized aggconfigs
on aggconfig instance schema is now also just a string (and not schema instance)
editor is responsible for matching this to actual schema instance to get the information it needs

Checklist

Delete any items that are not applicable to this PR.

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~

elasticmachine · 2020-02-25T12:28:07Z

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

ppisljar · 2020-03-04T12:51:20Z

src/legacy/core_plugins/vis_default_editor/public/components/sidebar/state/reducers.ts

-      const payloadAggConfig = action.payload as IAggConfig;
-      const aggConfig = state.aggs.createAggConfig(payloadAggConfig, {
+      const { schema } = action.payload;
+      const defaultConfig =


handling defaults when adding a new aggregation

ppisljar · 2020-03-04T12:52:04Z

src/legacy/core_plugins/vis_default_editor/public/vis_type_agg_filter.ts

@@ -1,33 +0,0 @@
-/*
- * Licensed to Elasticsearch B.V. under one or more contributor


we no longer need this as aggFilter information is no longer present on schema, or anywhere on agg.

ppisljar · 2020-03-04T12:52:39Z

src/legacy/core_plugins/visualizations/public/np_ready/public/vis_impl.js

-        this.type.schemas.all
-      );
+      let configStates = state.aggs ? state.aggs.aggs || state.aggs : [];
+      configStates = this.initializeDefaultsFromSchemas(configStates, this.type.schemas.all || []);


handling defaults

lukeelmers

updates LGTM, pending approval from someone who is more familiar with the default vis editor. 😄

# Conflicts: # src/legacy/core_plugins/data/public/search/aggs/agg_types.ts

sulemanof

Please, make sure filtering works correctly!

sulemanof · 2020-03-05T08:30:17Z

src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts

+  allowedAggs: string[]
 ): ComboBoxGroupedOptions<IAggType> {
-  const aggTypeOptions = aggTypeFilters.filter((aggTypes as any)[groupName], indexPattern, agg);
+  const aggTypeOptions = aggTypeFilters


Since the aggTypeFilters is used only in rollup for adding a filter,
makes sense to get rid of it at all.
Please, take a look at this comment https://github.com/elastic/kibana/pull/57695/files/555e19e6ecf08b05e0dc39ebdc92d9f2d5bf32ef#r380614658

Of course, you can skip it for now, but anyway it's worth to do.

yes, lets do that in a followup

sulemanof · 2020-03-05T12:28:17Z

src/legacy/core_plugins/data/public/search/aggs/buckets/terms.ts

-    name: 'orderAgg',
-    // This string is never visible to the user so it doesn't need to be translated
-    title: 'Order Agg',
-    hideCustomLabel: true,


The custom label should not be presented in terms -> order agg

sulemanof · 2020-03-05T12:38:59Z

src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts

+  const aggTypeOptions = aggTypeFilters
+    .filter((aggTypes as any)[groupName], indexPattern, agg, aggFilter)
+    .filter(aggType => {
+      return filterByName([aggType], allowedAggs).length !== 0;


Can't we just merge both arrays, instead of adding the second filter, which do the same thing?

No, it won't give the same results

sulemanof · 2020-03-05T12:48:40Z

src/legacy/core_plugins/data/public/search/aggs/param_types/field.test.ts

-          get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
-            ? '*'
-            : KBN_FIELD_TYPES.NUMBER,
+        filterFieldTypes: () => KBN_FIELD_TYPES.NUMBER,


Make it as function become redundant.
In such a case, filterFieldTypes type could get rid of function type,
and the getAvailableFields could be simplified:

sulemanof · 2020-03-05T12:49:32Z

src/legacy/core_plugins/data/public/search/aggs/param_types/field.test.ts

-          get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
-            ? '*'
-            : KBN_FIELD_TYPES.NUMBER,
+        filterFieldTypes: (aggConfig: IMetricAggConfig) => '*',


It's a * by default

Suggested change

filterFieldTypes: (aggConfig: IMetricAggConfig) => '*',

oh sorry,
I just see that these are in tests file,
anyway I mean, that filterFieldTypes as a function became redundant,
so you could even remove it from tests

sulemanof · 2020-03-05T14:14:59Z

src/legacy/core_plugins/vis_default_editor/public/components/controls/rows_or_columns.tsx

-  const idSelected = `visEditorSplitBy__${agg.params.row}`;
+function RowsOrColumnsControl({ editorStateParams, setStateParamValue }: AggControlProps) {
+  if (editorStateParams.row === undefined) {
+    editorStateParams.row = true;


We do not prefer using object mutations in React, especially props mutations,
it may lead to issues which are hard to find then.

kibanamachine · 2020-03-05T16:18:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 54e0a11

History

💔 Build #31077 failed 7dc487e
💚 Build #31044 succeeded 2964295
💚 Build #30970 succeeded a01f9aa
💚 Build #30752 succeeded 338dbf6
💔 Build #30429 failed 7bd59ce98dea6057c1999a82d5e7c72de40eaba9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

sulemanof

All the comments were fixed, didn’t find anything else. It’s a great step in simplifying agg configs!
LGTM

ppisljar added WIP Work in progress v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 25, 2020

ppisljar requested a review from a team February 25, 2020 12:28

ppisljar requested a review from a team as a code owner February 25, 2020 12:28

lukeelmers mentioned this pull request Feb 27, 2020

Search 👉 New Platform #42639

Closed

14 tasks

ppisljar force-pushed the agg/schemas branch from 2dabcc8 to 9ee6762 Compare February 27, 2020 13:40

ppisljar added 6 commits March 2, 2020 08:07

removing schemas from aggconfigs

1c45299

moving to vis_default_editor

03da927

updating tests

d3aecb6

moving initing defaults from schemas to vis

36c7bc4

updating editor

573c7e1

updating after rebase

c24a818

ppisljar force-pushed the agg/schemas branch from 9ee6762 to c24a818 Compare March 2, 2020 14:15

ppisljar added 3 commits March 2, 2020 11:23

fixes

2b4fb94

adding utility functions

622b14b

fixing tests

34aa486

ppisljar force-pushed the agg/schemas branch 5 times, most recently from b3798fb to 839edb1 Compare March 3, 2020 19:28

ppisljar removed the WIP Work in progress label Mar 3, 2020

ppisljar changed the title ~~[WIP] removing schemas from agg configs~~ removing schemas from agg configs Mar 3, 2020

ppisljar force-pushed the agg/schemas branch from 839edb1 to 71d4d0d Compare March 3, 2020 23:23

fixes

a255bb3

ppisljar force-pushed the agg/schemas branch from 71d4d0d to a255bb3 Compare March 3, 2020 23:39

ppisljar commented Mar 4, 2020

View reviewed changes

schemaTitle

338dbf6

lukeelmers approved these changes Mar 4, 2020

View reviewed changes

Merge branch 'master' into agg/schemas

a01f9aa

# Conflicts: # src/legacy/core_plugins/data/public/search/aggs/agg_types.ts

sulemanof suggested changes Mar 5, 2020

View reviewed changes

sulemanof reviewed Mar 5, 2020

View reviewed changes

updating based on review

2964295

ppisljar force-pushed the agg/schemas branch from 7cfee51 to 2964295 Compare March 5, 2020 11:23

sulemanof reviewed Mar 5, 2020

View reviewed changes

ppisljar added 3 commits March 5, 2020 08:49

updating based on review

a3a08dd

merging filter arrays

7dc487e

hidding custom label on pipeline aggs

fc48c65

sulemanof reviewed Mar 5, 2020

View reviewed changes

ppisljar added 2 commits March 5, 2020 09:19

fixing

e35ffbb

using callback to modify row option

54e0a11

sulemanof approved these changes Mar 5, 2020

View reviewed changes

lukeelmers merged commit 87a3d02 into elastic:master Mar 5, 2020

lukeelmers mentioned this pull request Mar 5, 2020

[7.x] Remove schemas from agg configs (#58462) #59449

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Mar 5, 2020

Remove schemas from agg configs (elastic#58462)

772bb6d

lukeelmers mentioned this pull request Mar 5, 2020

Fix agg type shims and move paginated table to kibana_legacy #57695

Closed

lukeelmers added a commit that referenced this pull request Mar 5, 2020

Remove schemas from agg configs (#58462) (#59449)

e540979

flash1293 mentioned this pull request May 29, 2020

Kibana 7.7.0: Chart split orientation is wrong #67444

Closed

alexwizp mentioned this pull request Jun 1, 2020

Kibana 7.7.0: Chart split orientation is wrong #67840

Merged

7 tasks

		@@ -1,33 +0,0 @@
		/*
		* Licensed to Elasticsearch B.V. under one or more contributor

Conversation

ppisljar commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

sulemanof left a comment

Choose a reason for hiding this comment

Uh oh!

sulemanof Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sulemanof Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 5, 2020

💚 Build Succeeded

History

Uh oh!

sulemanof left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ppisljar commented Feb 25, 2020 •

edited

Loading

sulemanof Mar 5, 2020 •

edited

Loading

sulemanof Mar 5, 2020 •

edited

Loading