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

fixes filtering on other bucket outside of vislib #19860

Merged
merged 9 commits into from
Jun 20, 2018
Merged
17 changes: 10 additions & 7 deletions src/core_plugins/metric_vis/public/metric_vis_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,25 @@ export class MetricVisComponent extends Component {
const labels = this._getLabels();
const metrics = [];

tableGroups.tables.forEach((table) => {
tableGroups.tables.forEach((table, tableIndex) => {
let bucketAgg;
let rowHeaderIndex;

table.columns.forEach((column, i) => {
table.columns.forEach((column, columnIndex) => {
const aggConfig = column.aggConfig;

if (aggConfig && aggConfig.schema.group === 'buckets') {
bucketAgg = aggConfig;
// Store the current index, so we later know in which position in the
// row array, the bucket agg key will be, so we can create filters on it.
rowHeaderIndex = i;
rowHeaderIndex = columnIndex;
return;
}

table.rows.forEach(row => {
table.rows.forEach((row, rowIndex) => {

let title = column.title;
let value = row[i];
let value = row[columnIndex];
const color = this._getColor(value, labels, colors);

if (isPercentageMode) {
Expand All @@ -143,6 +143,9 @@ export class MetricVisComponent extends Component {
bgColor: shouldColor && config.style.bgColor ? color : null,
lightText: shouldColor && config.style.bgColor && this._needsLightText(color),
filterKey: rowHeaderIndex !== undefined ? row[rowHeaderIndex] : null,
tableIndex: tableIndex,
rowIndex: rowIndex,
columnIndex: rowHeaderIndex,
bucketAgg: bucketAgg,
});
});
Expand All @@ -156,8 +159,8 @@ export class MetricVisComponent extends Component {
if (!metric.filterKey || !metric.bucketAgg) {
return;
}
const filter = metric.bucketAgg.createFilter(metric.filterKey);
this.props.vis.API.queryFilter.addFilters(filter);
const table = this.props.visData.tables[metric.tableIndex];
this.props.vis.API.events.addFilter(table, metric.columnIndex, metric.rowIndex);
};

_renderMetric = (metric, index) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,8 @@ export function RegionMapsVisualizationProvider(Private, Notifier, config) {
return;
}

const agg = this._vis.aggs.bySchemaName.segment[0];
const filter = agg.createFilter(event);
this._vis.API.queryFilter.addFilters(filter);
const rowIndex = this._chartData.tables[0].rows.findIndex(row => row[0] === event);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought: For region map the whole Other/Missing bucket is completely useless at the moment, since it only looks at the bucket keys (i.e. other and missing) and not at the label the user specified. So they will most likely never match any shape, at least not on the maps delivered via EMS.

With one of the upcoming PRs for roll-up support we'll be able to disable specific editor settings based on the vis, so we might want to actually hide the Other and missing setting for region maps. (cc: @thomasneirynck)

this._vis.API.events.addFilter(this._chartData.tables[0], 0, rowIndex);
});
this._choroplethLayer.on('styleChanged', (event) => {
const shouldShowWarning = this._vis.params.isDisplayWarning && config.get('visualization:regionmap:showWarnings');
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/tagcloud/public/tag_cloud.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ class TagCloud extends EventEmitter {
const self = this;
enteringTags.on({
click: function (event) {
self.emit('select', event.rawText);
self.emit('select', event);
},
mouseover: function () {
d3.select(this).style('cursor', 'pointer');
Expand Down
13 changes: 9 additions & 4 deletions src/core_plugins/tagcloud/public/tag_cloud_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ export class TagCloudVisualization {
if (!this._bucketAgg) {
return;
}
const filter = this._bucketAgg.createFilter(event);
this._vis.API.queryFilter.addFilters(filter);
this._vis.API.events.addFilter(
event.meta.data, 0, event.meta.rowIndex
);
});
this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');

Expand Down Expand Up @@ -119,12 +120,16 @@ export class TagCloudVisualization {
this._bucketAgg = null;
}

const tags = data.rows.map(row => {
const tags = data.rows.map((row, rowIndex) => {
const [tag, count] = row;
return {
displayText: this._bucketAgg ? this._bucketAgg.fieldFormatter()(tag) : tag,
rawText: tag,
value: count
value: count,
meta: {
data: data,
rowIndex: rowIndex,
}
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,7 @@ describe('Terms Agg Other bucket helper', () => {
vis.aggs[0],
otherAggConfig);

expect(mergedResponse.aggregations['1'].buckets[3].key).to.equal('Other');
expect(mergedResponse.aggregations['1'].buckets[3].filters.length).to.equal(2);
expect(mergedResponse.aggregations['1'].buckets[3].key).to.equal('__other__');
});

it('correctly merges other bucket with nested terms agg', () => {
Expand All @@ -255,8 +254,7 @@ describe('Terms Agg Other bucket helper', () => {
const mergedResponse = mergeOtherBucketAggResponse(vis.aggs, nestedTermResponse,
nestedOtherResponse, vis.aggs[1], otherAggConfig);

expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].key).to.equal('Other');
expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].filters.length).to.equal(2);
expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].key).to.equal('__other__');
});

});
Expand All @@ -265,17 +263,8 @@ describe('Terms Agg Other bucket helper', () => {
it('correctly updates missing bucket key', () => {
init(visConfigNestedTerm);
const updatedResponse = updateMissingBucket(singleTermResponse, vis.aggs, vis.aggs[0]);
expect(updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === 'Missing')).to.not.be('undefined');
expect(updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === '__missing__')).to.not.be('undefined');
});

it('correctly sets the bucket filter', () => {
const updatedResponse = updateMissingBucket(singleTermResponse, vis.aggs, vis.aggs[0]);
const missingBucket = updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === 'Missing');
expect(missingBucket.filters).to.not.be('undefined');
expect(missingBucket.filters[0]).to.eql({
meta: { index: 'logstash-*', negate: true },
exists: { field: 'geo.src' }
});
});
});
});
31 changes: 31 additions & 0 deletions src/ui/public/agg_types/__tests__/buckets/create_filter/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,36 @@ describe('AggConfig Filters', function () {
expect(filterTrue.query.match).to.have.property('ssl');
expect(filterTrue.query.match.ssl).to.have.property('query', true);
});

it('should generate correct __missing__ filter', () => {
const vis = new Vis(indexPattern, {
type: 'histogram',
aggs: [ { type: 'terms', schema: 'segment', params: { field: '_type' } } ]
});
const aggConfig = vis.aggs.byTypeName.terms[0];
const filter = createFilterTerms(aggConfig, '__missing__');
expect(filter).to.have.property('exists');
expect(filter.exists).to.have.property('field', '_type');
expect(filter).to.have.property('meta');
expect(filter.meta).to.have.property('index', indexPattern.id);
expect(filter.meta).to.have.property('negate', true);
});

it('should generate correct __other__ filter', () => {
const vis = new Vis(indexPattern, {
type: 'histogram',
aggs: [ { type: 'terms', schema: 'segment', params: { field: '_type' } } ]
});
const aggConfig = vis.aggs.byTypeName.terms[0];
const filter = createFilterTerms(aggConfig, '__other__', { terms: ['apache'] })[0];
expect(filter).to.have.property('query');
expect(filter.query).to.have.property('bool');
expect(filter.query.bool).to.have.property('should');
expect(filter.query.bool.should[0]).to.have.property('match_phrase');
expect(filter.query.bool.should[0].match_phrase).to.have.property('_type', 'apache');
expect(filter).to.have.property('meta');
expect(filter.meta).to.have.property('index', indexPattern.id);
expect(filter.meta).to.have.property('negate', true);
});
});
});
30 changes: 30 additions & 0 deletions src/ui/public/agg_types/__tests__/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,36 @@ describe('Terms Agg', function () {
expect($rootScope.agg.params.orderBy).to.be('_key');
});

describe('custom field formatter', () => {
beforeEach(() => {
init({
responseValueAggs: [
{
id: 'agg1',
type: {
name: 'count'
}
}
],
aggParams: {
otherBucketLabel: 'Other',
missingBucketLabel: 'Missing'
}
});
$rootScope.$digest();
});

it ('converts __other__ key', () => {
const formatter = $rootScope.agg.type.getFormat($rootScope.agg).getConverterFor('text');
expect(formatter('__other__')).to.be('Other');
});

it ('converts __missing__ key', () => {
const formatter = $rootScope.agg.type.getFormat($rootScope.agg).getConverterFor('text');
expect(formatter('__missing__')).to.be('Missing');
});
});

it('adds "custom metric" option');
it('lists all metric agg responses');
it('lists individual values of a multi-value metric');
Expand Down
17 changes: 9 additions & 8 deletions src/ui/public/agg_types/buckets/_terms_other_bucket_helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import _ from 'lodash';
import { AggConfig } from '../../vis/agg_config';
import { buildPhrasesFilter } from '../../filter_manager/lib/phrases';
import { buildExistsFilter } from '../../filter_manager/lib/exists';
import { buildPhrasesFilter } from '../../filter_manager/lib/phrases';
import { buildQueryFromFilters } from '../../courier/data_source/build_query/from_filters';

/**
Expand Down Expand Up @@ -102,6 +102,7 @@ const getOtherAggTerms = (requestAgg, key, otherAgg) => {
);
};


const buildOtherBucketAgg = (aggConfigs, aggWithOtherBucket, response) => {
const bucketAggs = aggConfigs.filter(agg => agg.type.type === 'buckets');
const index = bucketAggs.findIndex(agg => agg.id === aggWithOtherBucket.id);
Expand Down Expand Up @@ -175,12 +176,11 @@ const mergeOtherBucketAggResponse = (aggsConfig, response, otherResponse, otherA
const phraseFilter = buildPhrasesFilter(otherAgg.params.field, requestFilterTerms, otherAgg.params.field.indexPattern);
phraseFilter.meta.negate = true;
bucket.filters = [ phraseFilter ];
bucket.key = otherAgg.params.otherBucketLabel;
bucket.key = '__other__';
Copy link
Member

Choose a reason for hiding this comment

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

What about moving __other__ and __missing__ strings to an enum object somewhere?
It can be easily refactored in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tim is gonna try with using symbols in a separate PR


if (aggResultBuckets.some(bucket => bucket.key === '__missing__')) {
bucket.filters.push(buildExistsFilter(otherAgg.params.field, otherAgg.params.field.indexPattern));
}

aggResultBuckets.push(bucket);
});
return updatedResponse;
Expand All @@ -190,12 +190,13 @@ const updateMissingBucket = (response, aggConfigs, agg) => {
const updatedResponse = _.cloneDeep(response);
const aggResultBuckets = getAggConfigResultMissingBuckets(updatedResponse.aggregations, agg.id);
aggResultBuckets.forEach(bucket => {
bucket.key = agg.params.missingBucketLabel;
const existsFilter = buildExistsFilter(agg.params.field, agg.params.field.indexPattern);
existsFilter.meta.negate = true;
bucket.filters = [ existsFilter ];
bucket.key = '__missing__';
Copy link
Member

Choose a reason for hiding this comment

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

Same as comment above

});
return updatedResponse;
};

export { buildOtherBucketAgg, mergeOtherBucketAggResponse, updateMissingBucket };
export {
buildOtherBucketAgg,
mergeOtherBucketAggResponse,
updateMissingBucket,
};
27 changes: 25 additions & 2 deletions src/ui/public/agg_types/buckets/create_filter/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,30 @@
*/

import { buildPhraseFilter } from '../../../filter_manager/lib/phrase';
import { buildPhrasesFilter } from '../../../filter_manager/lib/phrases';
import { buildExistsFilter } from '../../../filter_manager/lib/exists';

export function createFilterTerms(aggConfig, key) {
return buildPhraseFilter(aggConfig.params.field, key, aggConfig.vis.indexPattern);
export function createFilterTerms(aggConfig, key, params) {
const field = aggConfig.params.field;
const indexPattern = field.indexPattern;

if (key === '__other__') {
const terms = params.terms;

const phraseFilter = buildPhrasesFilter(field, terms, indexPattern);
phraseFilter.meta.negate = true;

const filters = [phraseFilter];

if (terms.some(term => term === '__missing__')) {
filters.push(buildExistsFilter(field, indexPattern));
}

return filters;
} else if (key === '__missing__') {
const existsFilter = buildExistsFilter(field, indexPattern);
existsFilter.meta.negate = true;
return existsFilter;
}
return buildPhraseFilter(field, key, indexPattern);
}
16 changes: 16 additions & 0 deletions src/ui/public/agg_types/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,22 @@ export const termsBucketAgg = new BucketAggType({
const params = agg.params;
return agg.getFieldDisplayName() + ': ' + params.order.display;
},
getFormat: function (bucket) {
return {
getConverterFor: (type) => {
return (val) => {
if (val === '__other__') {
return bucket.params.otherBucketLabel;
}
if (val === '__missing__') {
return bucket.params.missingBucketLabel;
}
const converter = bucket.params.field.format.getConverterFor(type);
return converter(val);
};
}
};
},
createFilter: createFilterTerms,
postFlightRequest: async (resp, aggConfigs, aggConfig, nestedSearchSource) => {
if (aggConfig.params.otherBucket) {
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/__tests__/_agg_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ describe('AggConfig', function () {
type: 'table',
aggs: [
{
type: 'terms',
type: 'histogram',
schema: 'bucket',
params: { field: 'bytes' }
}
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/vis/agg_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ AggConfig.prototype.isFilterable = function () {
return _.isFunction(this.type.createFilter);
};

AggConfig.prototype.createFilter = function (key) {
AggConfig.prototype.createFilter = function (key, params = {}) {
if (!this.isFilterable()) {
throw new TypeError('The "' + this.type.title + '" aggregation does not support filtering.');
}
Expand All @@ -197,7 +197,7 @@ AggConfig.prototype.createFilter = function (key) {
throw new TypeError(message);
}

return this.type.createFilter(this, key);
return this.type.createFilter(this, key, params);
};

/**
Expand Down
26 changes: 26 additions & 0 deletions src/ui/public/vis/vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ import { queryManagerFactory } from '../query_manager';
import { SearchSourceProvider } from '../courier/data_source/search_source';
import { SavedObjectsClientProvider } from '../saved_objects';

const getTerms = (table, columnIndex, rowIndex) => {
// get only rows where cell value matches current row for all the fields before columnIndex
const rows = table.rows.filter(row => row.every((cell, i) => cell === table.rows[rowIndex][i] || i >= columnIndex));
const terms = rows.map(row => row[columnIndex]);

return [...new Set(terms.filter(term => {
const notOther = term !== '__other__';
const notMissing = term !== '__missing__';
return notOther && notMissing;
}))];
};

export function VisProvider(Private, Promise, indexPatterns, timefilter, getAppState) {
const visTypes = Private(VisTypesRegistryProvider);
const brushEvent = Private(UtilsBrushEventProvider);
Expand Down Expand Up @@ -75,9 +87,23 @@ export function VisProvider(Private, Promise, indexPatterns, timefilter, getAppS
queryFilter: queryFilter,
queryManager: queryManagerFactory(getAppState),
events: {
// the filter method will be removed in the near feature
// you should rather use addFilter method below
filter: (event) => {
const appState = getAppState();
filterBarClickHandler(appState)(event);
},
addFilter: (data, columnIndex, rowIndex) => {
const agg = data.columns[columnIndex].aggConfig;
let filter = [];
const value = data.rows[rowIndex][columnIndex];
if (agg.type.name === 'terms' && agg.params.otherBucket) {
const terms = getTerms(data, columnIndex, rowIndex);
filter = agg.createFilter(value, { terms });
} else {
filter = agg.createFilter(value);
}
queryFilter.addFilters(filter);
}, brush: (event) => {
const appState = getAppState();
brushEvent(appState)(event);
Expand Down