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.

Sign up for GitHub

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

removing schema references from vis types #20489

Merged
merged 4 commits into from
Jul 12, 2018
Merged

removing schema references from vis types #20489

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d1c8627
removing schema references from vis types
ppisljar Jul 5, 2018
a3145fc
adding agg_param_writer back
ppisljar Jul 11, 2018
e11b98f
fixing visualize_data_loader props reference
ppisljar Jul 11, 2018
4d82688
props.searchSource
ppisljar Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core_plugins/metric_vis/public/__tests__/metric_vis_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('metric vis', function () {
fieldFormatter: () => {
return formatter;
},
schema: {}
type: {}
};

let metricController;
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/metric_vis/public/metric_vis_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export class MetricVisComponent extends Component {
table.columns.forEach((column, columnIndex) => {
const aggConfig = column.aggConfig;

if (aggConfig && aggConfig.schema.group === 'buckets') {
if (aggConfig && aggConfig.type.type === 'buckets') {
Copy link
Member

Choose a reason for hiding this comment

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

😞type.type I'm not a fan of repetitions. I've just had a conversation with @timroes about headaches created by the namings of AggConfig, AggConfigs, AggParam, AggParams, aggs, agg, AggType etc.

I'm writing aloud here:
so the reason about having buckets and metrics is clear: metrics agg types are used as dependant variable (the usual y value of an expression) and the buckets are use as independent variable (usually the x variable in the expression).
We have many possible aggtypes for each dependant and independant variable. So the initial way of call it group could be a better or at least more readable name for this , and you will call it like: aggConfig.type.group. Can me also some more specific like groupId since buckets and metrics are identifiers of the group and not general variables.
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

i agree, however i would not put that in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

So the initial way of call it group could be a better or at least more readable name

i agree, however i would not put that in this PR

I am not sure I follow why changing type.type to type.group would not be part of this PR yet this is the PR that is changing aggConfig.schema.group to aggConfig.type.type.

Copy link
Member Author

@ppisljar ppisljar Jul 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

as the changes from aggConfig.schema.group to aggConfig.type.type do not include all the references to type.type it would increase the size of already big pr ...

i will open one to do this rename as soon as i get back from vacation i promise :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Makes sense

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.
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/agg_response/hierarchical/__tests__/transform_aggregation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('buildHierarchicalData()', function () {
return {
id: id,
name: name,
schema: { group: 'buckets' },
type: { type: 'buckets' },
getKey: (bucket) => bucket.key,
fieldFormatter: _.constant(String)
};
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/agg_response/tabify/_get_columns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function tabifyGetColumns(aggs, minimal, hierarchical) {

// separate the metrics
const grouped = _.groupBy(aggs, function (agg) {
return agg.schema.group;
return agg.type.type;
});

if (!grouped.buckets) {
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/agg_response/tabify/tabify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ function collectBucket(write, bucket, key, aggScale) {
const aggInfo = agg.write(write.aggs);
aggScale *= aggInfo.metricScale || 1;

switch (agg.schema.group) {
switch (agg.type.type) {
case 'buckets':
const buckets = new TabifyBuckets(bucket[agg.id], agg.params);
if (buckets.length) {
Expand Down Expand Up @@ -103,7 +103,7 @@ function collectBucket(write, bucket, key, aggScale) {
function passEmptyBuckets(write, bucket, key, aggScale) {
const agg = write.aggStack.shift();

switch (agg.schema.group) {
switch (agg.type.type) {
case 'metrics':
// pass control back to collectBucket()
write.aggStack.unshift(agg);
Expand Down
50 changes: 9 additions & 41 deletions src/ui/public/agg_types/__tests__/agg_param_writer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@
import _ from 'lodash';
import { VisProvider } from '../../vis';
import { aggTypes } from '..';
import { VisTypesRegistryProvider } from '../../registry/vis_types';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { AggConfig } from '../../vis/agg_config';

// eslint-disable-next-line @elastic/kibana-custom/no-default-export
export default function AggParamWriterHelper(Private) {
const Vis = Private(VisProvider);
const visTypes = Private(VisTypesRegistryProvider);
const stubbedLogstashIndexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);

/**
Expand Down Expand Up @@ -60,32 +57,16 @@ export default function AggParamWriterHelper(Private) {
// not configurable right now, but totally required
self.indexPattern = stubbedLogstashIndexPattern;

// the vis type we will use to write the aggParams
self.visType = null;

// the schema that the aggType satisfies
self.visAggSchema = null;

// find a suitable vis type and schema
_.find(visTypes, function (visType) {
const schema = _.find(visType.schemas.all, function (schema) {
// type, type, type, type, type... :(
return schema.group === self.aggType.type;
});

if (schema) {
self.visType = visType;
self.visAggSchema = schema;
return true;
}
});

if (!self.aggType || !self.visType || !self.visAggSchema) {
throw new Error('unable to find a usable visType and schema for the ' + opts.aggType + ' agg type');
}

self.vis = new Vis(self.indexPattern, {
type: self.visType.name
type: 'histogram',
aggs: [{
id: 1,
type: self.aggType.name,
params: {}
}]
});
}

Expand All @@ -108,23 +89,10 @@ export default function AggParamWriterHelper(Private) {
}


const agg = new AggConfig(self.vis, {
id: 1,
schema: self.visAggSchema.name,
type: self.aggType.name,
params: paramValues
});

self.vis.setState({
type: self.vis.type.name,
aggs: [agg.toJSON()]
});

const aggConfig = _.find(self.vis.aggs, function (aggConfig) {
return aggConfig.type === self.aggType;
});
const aggConfig = self.vis.aggs[0];
aggConfig.setParams(paramValues);

return aggConfig.type.params.write(aggConfig, self.vis.aggs);
return aggConfig.write(self.vis.aggs);
};

return AggParamWriter;
Expand Down
3 changes: 0 additions & 3 deletions src/ui/public/agg_types/buckets/_terms_other_bucket_helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,6 @@ const buildOtherBucketAgg = (aggConfigs, aggWithOtherBucket, response) => {
const filterAgg = new AggConfig(aggConfigs[index].vis, {
type: 'filters',
id: 'other',
schema: {
group: 'buckets'
}
});

// nest all the child aggregations of aggWithOtherBucket
Expand Down
3 changes: 1 addition & 2 deletions src/ui/public/agg_types/buckets/geo_hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,7 @@ export const geoHashBucketAgg = new BucketAggType({
enabled: true,
params: {
field: agg.getField()
},
schema: 'metric'
}
}));
}

Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/agg_types/metrics/lib/parent_pipeline_agg_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const parentPipelineAggController = function ($scope) {
$scope.$watch('agg.params.metricAgg', updateOrderAgg);

$scope.$on('$destroy', function () {
const lastBucket = _.findLast($scope.vis.getAggConfig(), agg => agg.schema.group === 'buckets');
const lastBucket = _.findLast($scope.vis.getAggConfig(), agg => agg.type.type === 'buckets');
if ($scope.aggForm && $scope.aggForm.agg) {
$scope.aggForm.agg.$setValidity('bucket', true);
}
Expand All @@ -43,7 +43,7 @@ const parentPipelineAggController = function ($scope) {
};

function checkBuckets() {
const lastBucket = _.findLast($scope.vis.getAggConfig(), agg => agg.schema.group === 'buckets');
const lastBucket = _.findLast($scope.vis.getAggConfig(), agg => agg.type.type === 'buckets');
const bucketHasType = lastBucket && lastBucket.type;
const bucketIsHistogram = bucketHasType && ['date_histogram', 'histogram'].includes(lastBucket.type.name);
const canUseAggregation = lastBucket && bucketIsHistogram;
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/agg_types/metrics/top_hit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export const topHitMetricAgg = new MetricAggType({
],
controller: function ($scope) {
$scope.options = [];
$scope.$watchGroup([ 'agg.vis.type.name', 'agg.params.field.type' ], function ([ visName, fieldType ]) {
$scope.$watchGroup([ 'vis.type.name', 'agg.params.field.type' ], function ([ visName, fieldType ]) {
if (fieldType && visName) {
$scope.options = _.filter($scope.aggParam.options, option => {
return option.isCompatibleVis(visName) && option.isCompatibleType(fieldType);
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/agg_config_result.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default function AggConfigResult(aggConfig, parent, value, key, filters)
this.filters = filters;
this.$parent = parent;

if (aggConfig.schema.group === 'buckets') {
if (aggConfig.type.type === 'buckets') {
this.type = 'bucket';
} else {
this.type = 'metric';
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/vis/agg_configs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class AggConfigs extends IndexedArray {

parseParentAggs(dslLvlCursor, dsl);

if (config.schema.group === 'buckets' && i < list.length - 1) {
if (config.type.type === 'buckets' && i < list.length - 1) {
// buckets that are not the last item in the list accept sub-aggs
subAggs = dsl.aggs || (dsl.aggs = {});
}
Expand All @@ -167,7 +167,7 @@ class AggConfigs extends IndexedArray {
return aggs ? requestValuesAggs.concat(aggs) : requestValuesAggs;
}, []);
//move metrics to the end
return _.sortBy(aggregations, agg => agg.schema.group === 'metrics' ? 1 : 0);
return _.sortBy(aggregations, agg => agg.type.type === 'metrics' ? 1 : 0);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/visualize/loader/visualize_data_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export class VisualizeDataLoader {
return this._visData;
}
catch (e) {
this.props.searchSource.cancelQueued();
Copy link
Contributor

Choose a reason for hiding this comment

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

props actually has a searchSource reference. Shouldn't we rather use that one, to not make more references to Vis?

this._vis.searchSource.cancelQueued();
this._vis.requestError = e;
if (isTermSizeZeroError(e)) {
return toastNotifications.addDanger(
Expand Down