-
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 8 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 |
---|---|---|
|
@@ -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,18 @@ 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 |
||
const converter = bucket.params.field.format.getConverterFor(type); | ||
return converter(val); | ||
}; | ||
} | ||
}; | ||
}, | ||
createFilter: createFilterTerms, | ||
postFlightRequest: async (resp, aggConfigs, aggConfig, nestedSearchSource) => { | ||
if (aggConfig.params.otherBucket) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,13 @@ 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 => term !== '__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. 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. And subsequent question: why if we filter by other, we are also filtering by "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. if Missing was one of the buckets (for example 400, 404, Missing) then Other means not 400, non 404 and not missing (so exists) 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. Right, I was considering missing as not part of the "whole" since you can show/hide missing from the editor. |
||
}; | ||
|
||
export function VisProvider(Private, Promise, indexPatterns, timefilter, getAppState) { | ||
const visTypes = Private(VisTypesRegistryProvider); | ||
const brushEvent = Private(UtilsBrushEventProvider); | ||
|
@@ -75,9 +82,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); | ||
|
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.
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
andmissing
) 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)