Add AggTypeFieldFilters to filter out fields in vis editor #20539
Conversation
jen-huang
added
Feature:Visualizations
💔 Build Failed |
| @@ -0,0 +1,20 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file should be named .d.ts not .d.js
There was a problem hiding this comment.
the .d.ts version already existed. not sure how the compiled version got generated and committed 😳anyway - removed 🙂
| const filterByType = propFilter('type'); | ||
|
|
||
| /** | ||
| * This filter checks the defined aggFilter in the schemas of that visualization |
There was a problem hiding this comment.
This documentation doesn't look quiet right :)
|
|
||
| if (fieldParamType.filterFieldTypes) { | ||
| let filters = fieldParamType.filterFieldTypes; | ||
| if (_.isFunction(fieldParamType.filterFieldTypes)) { |
There was a problem hiding this comment.
If we use lodash here we need to import it. I would also suggest that we import it named import { isFunction } from 'lodash'; due to better tree shaking in the end.
| import { AggConfig } from '../../../vis'; | ||
|
|
||
| type AggTypeFieldFilter = ( | ||
| fields: any[], |
There was a problem hiding this comment.
This signature is wrong. It should be field: any. TypeScript don't catch this error, since we use any in this place, and any[] is always assignable to any anyways.
|
|
||
| return new IndexedArray({ | ||
| index: ['name'], | ||
| group: ['type'], | ||
| initialSet: fields | ||
| initialSet: _.sortBy(fields, ['type', 'name']), |
There was a problem hiding this comment.
I know that's not introduced by you, but since that's the only line using lodash in that file, maybe switch over to named import: import { sortBy } from 'lodash';
| return false; | ||
| } | ||
|
|
||
| if (fieldParamType.filterFieldTypes) { |
There was a problem hiding this comment.
Maybe use destructuring here to reduce repetition, and use an early exit so that the bulk of the work is done at the end of the function? We could also use a ternary to make the binary assignment of filters clearer.
const { filterFieldTypes } = fieldParamType;
if (!filterFieldTypes) {
return true;
}
let filters = _.isFunction(filterFieldTypes)
? filterFieldTypes.bind(this, aggConfig.vis)
: filterFieldTypes;
return filterByType([field], filters).length !== 0;There was a problem hiding this comment.
restructured this function with this feedback 🙂
|
|
||
| type AggTypeFieldFilter = ( | ||
| fields: any[], | ||
| aggType: AggType, |
There was a problem hiding this comment.
I think we should not pass in aggType directly to the filters. aggConfig.type will always retrieve the type and we have no ambitions to remove that, since aggConfig is pretty much just one specifically configered aggregation that always will have a type. So I think we should rather reduce this to just pass in aggConfig here.
There was a problem hiding this comment.
removed references to aggType
💔 Build Failed |
jen-huang
force-pushed
the
aggtype-fields-filter
branch
from
494c14e
to
d80d270
Compare
💚 Build Succeeded |
|
@jen-huang how do I test this? Thanks! |
Similar to #19913, this adds an additional
AggTypeFieldFiltersregistry to filter out fields for an aggregation type.This is need to support rollups (#20004). Rollups not only limits the agg type users can query, but also limits fields for a particular agg type.
The current filtering performed by
FieldParamType.prototype.getFieldOptions()has been converted to be a registered filter.