changing the way aggconfig field filter works #22756
Conversation
| @@ -54,8 +54,7 @@ function AggParams(params) { | |||
| AggParams.Super.call(this, { | |||
| index: ['name'], | |||
| initialSet: params.map(function (config) { | |||
| const type = config.name === 'field' ? config.name : config.type; | |||
| const Class = paramTypeMap[type] || paramTypeMap._default; | |||
| const Class = paramTypeMap[config.type] || paramTypeMap._default; | |||
There was a problem hiding this comment.
setting param type is now obligatory (before we defaulted to field type if name was field)
|
|
||
| const isNumber = function (type) { | ||
| return type === 'number'; | ||
| }; | ||
|
|
||
| aggTypeFieldFilters.addFilter( |
There was a problem hiding this comment.
top_hit now adds a filter for the fields which will be used by editor
| /** | ||
| * Get the options for this field from the indexPattern | ||
| */ | ||
| FieldParamType.prototype.getFieldOptions = function (aggConfig) { |
There was a problem hiding this comment.
method was renamed to getAvailableFields
| */ | ||
| FieldParamType.prototype.getAvailableFields = function (fields) { | ||
|
|
||
| return new IndexedArray({ |
There was a problem hiding this comment.
it filters all the fields down to the fields supported by this aggregation
| indexPattern: IndexPattern, | ||
| aggConfig: AggConfig | ||
| ) { | ||
| public filter(fields: any[], fieldParamType: FieldParamType, aggConfig: AggConfig, vis: Vis) { |
There was a problem hiding this comment.
this filters are executed by editor now
There was a problem hiding this comment.
(so we can have access to vis object)
| @@ -19,7 +19,7 @@ | |||
|
|
|||
| import 'angular-ui-select'; | |||
| import { uiModules } from '../modules'; | |||
| import { getFieldOptions } from './lib/filter_editor_utils'; | |||
| import { getFilterableFields } from './lib/filter_editor_utils'; | |||
There was a problem hiding this comment.
this was renamed as the old name was used for multiple functions, where none of them got you the field options :)
ppisljar
added
review
Feature:Visualizations
markov00
added
the
release_note:plugin_api_changes
|
@ppisljar Please check this: https://l7xor5om2l.codesandbox.io/ to conform the dev docs to the "standard" |
| if (aggConfig.type.name !== 'top_hit' || vis.type.name === 'table' || vis.type.name === 'metric') { | ||
| return true; | ||
| } | ||
| return field.type === 'number'; |
There was a problem hiding this comment.
nit: because we have a isNumber function above, we can use it :)
| } | ||
|
|
||
| return filterByType([field], filterFieldTypes).length !== 0; | ||
| }), ['type', 'name']), |
There was a problem hiding this comment.
What if we first filter to an object and than sort by? it will be a bit more readable so you dont have to look 10 rows below to understand what is the sorting by params
💔 Build Failed |
💔 Build Failed |
|
retest |
💚 Build Succeeded |
|
@jen-huang could you please check that these changes will still work for roll-up support. We needed to change the existing implementation a bit for the pipeline refactoring. |
💔 Build Failed |
for #21827 we need to change the way we filter which fields aggregation supports.
currently the top hit aggregation would show all the fields on table or metric visualization and only number fields on other visualizations.
the filtering should not happen on aggconfigs level, as as far as aggregations are concerned top_hit can
work with every field type. We need to have additional level of filtering on the editor level however.
qa: nothing should change, top_hits should still show all fields with table or metric visualization and only number fields otherwise, just implementation is different.
Dev Docs
AggConfig field param
Every aggregation (
AggConfig) withfieldparameter type needs to set the type tofieldnow (as before setting the name tofieldwas sufficient.)We don't recommend writing your own aggregations. We don't consider this to be a public API, and it's very likely this will change in the future. Since we know a couple of plugins use this nevertheless, we are announcing this change here.