-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 5 commits
06e4b68
e0ae00e
32b9181
9986996
fd967c6
fcc9df6
eac9af5
adf08b9
6b6db61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. With one of the upcoming PRs for roll-up support we'll be able to disable specific editor settings based on the |
||
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'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
this._data, 0, this._data.rows.findIndex(row => row[0] === event) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest instead of trying to find the row, we change the tagcloud visualization to attach that data and pass it when clicking a tag: diff --git a/src/core_plugins/tagcloud/public/tag_cloud.js b/src/core_plugins/tagcloud/public/tag_cloud.js
index db3b1a8506..3d158ec4d5 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud.js
@@ -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');
diff --git a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
index e9cc02ee9e..0fb5cc11ee 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
@@ -47,7 +47,7 @@ export class TagCloudVisualization {
return;
}
this._vis.API.events.addFilter(
- this._data, 0, this._data.rows.findIndex(row => row[0] === event)
+ this._data, 0, event.meta.rowIndex
);
});
this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');
@@ -121,12 +121,15 @@ 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: {
+ rowIndex,
+ },
};
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should even attach the data to each tag's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i.e. diff --git a/src/core_plugins/tagcloud/public/tag_cloud.js b/src/core_plugins/tagcloud/public/tag_cloud.js
index db3b1a8506..3d158ec4d5 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud.js
@@ -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');
diff --git a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
index e9cc02ee9e..e3e4bee1dd 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
@@ -47,7 +47,7 @@ export class TagCloudVisualization {
return;
}
this._vis.API.events.addFilter(
- this._data, 0, this._data.rows.findIndex(row => row[0] === event)
+ event.meta.data, 0, event.meta.rowIndex
);
});
this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');
@@ -121,12 +121,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,
+ rowIndex,
+ },
};
}); |
||
); | ||
}); | ||
this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete'); | ||
|
||
|
@@ -112,6 +113,7 @@ export class TagCloudVisualization { | |
} | ||
|
||
const data = response.tables[0]; | ||
this._data = data; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be removed now (sorry was missing in my diff). |
||
const segmentAggs = this._vis.aggs.bySchemaName.segment; | ||
if (segmentAggs && segmentAggs.length > 0) { | ||
this._bucketAgg = segmentAggs[0]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
/** | ||
|
@@ -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); | ||
|
@@ -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__'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about moving There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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__'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,17 @@ 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to follow styleguides on braces for conditionals |
||
return bucket.params.field.format.getConverterFor(type)(val); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slight performance improvement, move getFormat: function (bucket) {
return {
getConverterFor: (type) => {
const converter = bucket.params.field.format.getConverterFor(type);
return (val) => {
if (val === '__other__') return bucket.params.otherBucketLabel;
if (val === '__missing__') return bucket.params.missingBucketLabel;
return converter(val);
};
}
};
}, |
||
}; | ||
} | ||
}; | ||
}, | ||
createFilter: createFilterTerms, | ||
postFlightRequest: async (resp, aggConfigs, aggConfig, nestedSearchSource) => { | ||
if (aggConfig.params.otherBucket) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a mistake and should still be
this.props.vis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even after fixing that, filtering on metric does not work anymore at all. Neither on
Other
bucket nor an any other split bucket.