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

simplified tabify #19061

Merged
merged 26 commits into from
Aug 30, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
05616c8
simplified tabify
ppisljar May 15, 2018
782b7b7
updating point series responseConverter to work with new tabify
ppisljar May 15, 2018
d1fbdcd
updating geoJSON response converter to work with new tabify
ppisljar May 15, 2018
eb8fe8e
adding table response handler (for splitting tables)
ppisljar Jul 26, 2018
ae1b902
updating tabify based visualizations to work with new format
ppisljar Jul 26, 2018
ec821ee
rebasing on top of master
ppisljar Jul 26, 2018
75cb639
fixing inspector
ppisljar Jul 30, 2018
2342dd1
fixing some more issues
ppisljar Jul 30, 2018
0ac4ed6
handling no results
ppisljar Jul 30, 2018
aafcd01
vislib needs support for split tables
ppisljar Jul 30, 2018
7cb2e56
handle tag cloud no term agg defined
ppisljar Jul 30, 2018
662eca8
correctly handling missing values
ppisljar Jul 30, 2018
37f13e5
fixing typescript errors
ppisljar Jul 31, 2018
d304c79
fixes
ppisljar Aug 1, 2018
6a74f00
fixing (most) unit tests
ppisljar Aug 6, 2018
d083735
fixing remaining unit tests
ppisljar Aug 6, 2018
a346d76
renaming column.title to column.name (to match canvas)
ppisljar Aug 15, 2018
8173c32
adding tests
ppisljar Aug 15, 2018
aa54b93
updating based on tims review
ppisljar Aug 20, 2018
dc23e88
fixing bug discovered by bahvya
ppisljar Aug 27, 2018
dd09305
adding back partial rows
ppisljar Aug 28, 2018
9ab1e66
adding back partial rows part 2
ppisljar Aug 29, 2018
5fc0cd5
fixing test
ppisljar Aug 29, 2018
baabce5
correctly settings metricsAtAllLevels
ppisljar Aug 29, 2018
9e4e786
tims comments
ppisljar Aug 30, 2018
d964ffc
merge master
ppisljar Aug 30, 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/kbn_vislib_vis_types/public/gauge.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,6 @@ export default function GaugeVisType(Private) {
}
])
},
handleNoResults: true
useCustomNoDataScreen: true
});
}
2 changes: 1 addition & 1 deletion src/core_plugins/kbn_vislib_vis_types/public/goal.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,6 @@ export default function GoalVisType(Private) {
}
])
},
handleNoResults: true
useCustomNoDataScreen: true
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import 'ui/query_bar';
import { hasSearchStategyForIndexPattern, isDefaultTypeIndexPattern } from 'ui/courier';
import { toastNotifications } from 'ui/notify';
import { VisProvider } from 'ui/vis';
import { BasicResponseHandlerProvider } from 'ui/vis/response_handlers/basic';
import { VislibResponseHandlerProvider } from 'ui/vis/response_handlers/vislib';
import { DocTitleProvider } from 'ui/doc_title';
import PluginsKibanaDiscoverHitSortFnProvider from '../_hit_sort_fn';
import { FilterBarQueryFilterProvider } from 'ui/filter_bar/query_filter';
Expand Down Expand Up @@ -152,7 +152,7 @@ function discoverController(
const docTitle = Private(DocTitleProvider);
const HitSortFn = Private(PluginsKibanaDiscoverHitSortFnProvider);
const queryFilter = Private(FilterBarQueryFilterProvider);
const responseHandler = Private(BasicResponseHandlerProvider).handler;
const responseHandler = Private(VislibResponseHandlerProvider).handler;
const filterManager = Private(FilterManagerProvider);
const notify = new Notifier({
location: 'Discover'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import $ from 'jquery';
import _ from 'lodash';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import { TableResponseHandlerProvider } from 'ui/vis/response_handlers/table';
import { LegacyResponseHandlerProvider } from 'ui/vis/response_handlers/legacy';
import { VisProvider } from 'ui/vis';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { AppStateProvider } from 'ui/state_management/app_state';
Expand All @@ -45,7 +45,7 @@ describe('Table Vis Controller', function () {
fixtures = require('fixtures/fake_hierarchical_data');
AppState = Private(AppStateProvider);
Vis = Private(VisProvider);
tableAggResponse = Private(TableResponseHandlerProvider).handler;
tableAggResponse = Private(LegacyResponseHandlerProvider).handler;
}));

function OneRangeVis(params) {
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/table_vis/public/table_vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function TableVisTypeProvider(Private) {
}
])
},
responseHandler: 'table',
responseHandler: 'legacy',
responseHandlerConfig: {
asAggConfigResults: true
},
Expand Down
2 changes: 1 addition & 1 deletion src/core_plugins/tagcloud/public/tag_cloud_vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,6 @@ VisTypesRegistryProvider.register(function (Private) {
}
])
},
handleNoResults: true
useCustomNoDataScreen: true
});
});
6 changes: 3 additions & 3 deletions src/core_plugins/tagcloud/public/tag_cloud_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ export class TagCloudVisualization {

const hasTags = data.columns.length === 2;
const tagColumn = hasTags ? data.columns[0].id : -1;
const countColumn = data.columns[hasTags ? 1 : 0].id;
const metricColumn = data.columns[hasTags ? 1 : 0].id;
const tags = data.rows.map((row, rowIndex) => {
const tag = row[tagColumn] || 'all';
const count = row[countColumn];
const metric = row[metricColumn];
return {
displayText: this._bucketAgg ? this._bucketAgg.fieldFormatter()(tag) : tag,
rawText: tag,
value: count,
value: metric,
meta: {
data: data,
rowIndex: rowIndex,
Expand Down
6 changes: 3 additions & 3 deletions src/ui/public/agg_response/tabify/__tests__/_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ describe('tabifyAggResponse Integration', function () {
normalizeIds(vis);

const resp = tabifyAggResponse(vis.getAggConfig(), fixtures.metricOnly, {
minimalColumns: vis.isHierarchical()
metricsAtAllLevels: vis.isHierarchical()
});

expect(resp).to.have.property('rows').and.property('columns');
expect(resp.rows).to.have.length(1);
expect(resp.columns).to.have.length(1);

expect(resp.rows[0]).to.eql({ 'col-0': 1000 });
expect(resp.rows[0]).to.eql({ 'col-0-agg_1': 1000 });
expect(resp.columns[0]).to.have.property('aggConfig', vis.aggs[0]);
});

Expand Down Expand Up @@ -159,7 +159,7 @@ describe('tabifyAggResponse Integration', function () {
// the existing bucket and it's metric

vis.isHierarchical = _.constant(true);
const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { minimalColumns: false });
const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { metricsAtAllLevels: true });

expectColumns(tabbed, [ext, avg, src, avg, os, avg]);

Expand Down
20 changes: 10 additions & 10 deletions src/ui/public/agg_response/tabify/__tests__/_response_writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ describe('TabbedAggResponseWriter class', function () {
expect(responseWriter.columns.length).to.eql(3);
});

it('correctly generates columns with minimal columns set to false', () => {
const minimalColumnsResponseWriter = createResponseWritter(twoSplitsAggConfig, { minimalColumns: false });
it('correctly generates columns with metricsAtAllLevels set to true', () => {
const minimalColumnsResponseWriter = createResponseWritter(twoSplitsAggConfig, { metricsAtAllLevels: true });
expect(minimalColumnsResponseWriter.columns.length).to.eql(4);
});
});
Expand All @@ -84,7 +84,7 @@ describe('TabbedAggResponseWriter class', function () {
let responseWriter;

beforeEach(() => {
responseWriter = createResponseWritter(splitAggConfig);
responseWriter = createResponseWritter(splitAggConfig, { partialRows: true });
});

it('adds the row to the array', () => {
Expand Down Expand Up @@ -117,18 +117,18 @@ describe('TabbedAggResponseWriter class', function () {
});

it('produces correct response', () => {
responseWriter.rowBuffer['col-0'] = 'US';
responseWriter.rowBuffer['col-1'] = 5;
responseWriter.rowBuffer['col-0-1'] = 'US';
responseWriter.rowBuffer['col-1-2'] = 5;
responseWriter.row();
const response = responseWriter.response();
expect(response).to.have.property('rows');
expect(response.rows).to.eql([{ 'col-0': 'US', 'col-1': 5 }]);
expect(response.rows).to.eql([{ 'col-0-1': 'US', 'col-1-2': 5 }]);
expect(response).to.have.property('columns');
expect(response.columns.length).to.equal(2);
expect(response.columns[0]).to.have.property('id', 'col-0');
expect(response.columns[0]).to.have.property('id', 'col-0-1');
expect(response.columns[0]).to.have.property('name', 'geo.src: Descending');
expect(response.columns[0]).to.have.property('aggConfig');
expect(response.columns[1]).to.have.property('id', 'col-1');
expect(response.columns[1]).to.have.property('id', 'col-1-2');
expect(response.columns[1]).to.have.property('name', 'Count');
expect(response.columns[1]).to.have.property('aggConfig');
});
Expand All @@ -139,10 +139,10 @@ describe('TabbedAggResponseWriter class', function () {
expect(response.rows.length).to.be(0);
expect(response).to.have.property('columns');
expect(response.columns.length).to.equal(2);
expect(response.columns[0]).to.have.property('id', 'col-0');
expect(response.columns[0]).to.have.property('id', 'col-0-1');
expect(response.columns[0]).to.have.property('name', 'geo.src: Descending');
expect(response.columns[0]).to.have.property('aggConfig');
expect(response.columns[1]).to.have.property('id', 'col-1');
expect(response.columns[1]).to.have.property('id', 'col-1-2');
expect(response.columns[1]).to.have.property('name', 'Count');
expect(response.columns[1]).to.have.property('aggConfig');
});
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/agg_response/tabify/_get_columns.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import _ from 'lodash';
const getColumn = (agg, i) => {
return {
aggConfig: agg,
id: `col-${i}`,
id: `col-${i}-${agg.id}`,
name: agg.makeLabel()
};
};
Expand Down
26 changes: 17 additions & 9 deletions src/ui/public/agg_response/tabify/_response_writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,27 @@ import { tabifyGetColumns } from './_get_columns';
* Writer class that collects information about an aggregation response and
* produces a table, or a series of tables.
*
* @param {Vis} vis - the vis object to which the aggregation response correlates
* @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates
* @param {boolean} minimalColumns - setting to false will produce metrics for every bucket
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs adjustements to the new parameters partialRows and metricsAtAllLevels now.

*/

This comment was marked as resolved.

function TabbedAggResponseWriter(aggs, opts) {
this.opts = opts || {};
function TabbedAggResponseWriter(aggs, { metricsAtAllLevels = false, partialRows = false }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

function TabbedAggResponseWriter(aggs, { metricsAtAllLevels = false, partialRows = false }) {
->
function TabbedAggResponseWriter(aggs, { metricsAtAllLevels = false, partialRows = false } = {}) {

That way it will also work if the user didn't specify a 2nd parameter at all.

this.rowBuffer = {};
this.bucketBuffer = [];
this.metricBuffer = [];

// by default minimalColumns is set to true
this.opts.minimalColumns = !(this.opts.minimalColumns === false);

this.metricsForAllBuckets = metricsAtAllLevels;
this.partialRows = partialRows;
this.aggs = aggs;
this.columns = tabifyGetColumns(aggs.getResponseAggs(), this.opts.minimalColumns);
this.columns = tabifyGetColumns(aggs.getResponseAggs(), !metricsAtAllLevels);
this.aggStack = [...this.columns];

this.rows = [];
}

TabbedAggResponseWriter.prototype.isPartialRow = function (row) {
return !this.columns.map(column => row.hasOwnProperty(column.id)).every(c => (c === true));
};

/**
* Create a new row by reading the row buffer and bucketBuffer
*/
Expand All @@ -49,7 +53,11 @@ TabbedAggResponseWriter.prototype.row = function () {
this.rowBuffer[bucket.id] = bucket.value;
});

if (!toArray(this.rowBuffer).length) {
this.metricBuffer.forEach(metric => {
this.rowBuffer[metric.id] = metric.value;
});

if (!toArray(this.rowBuffer).length || (!this.partialRows && this.isPartialRow(this.rowBuffer))) {
return;
}

Expand All @@ -60,7 +68,7 @@ TabbedAggResponseWriter.prototype.row = function () {
/**
* Get the actual response
*
* @return {object} - the final table-tree
* @return {object} - the final table
*/
TabbedAggResponseWriter.prototype.response = function () {
return {
Expand Down
8 changes: 6 additions & 2 deletions src/ui/public/agg_response/tabify/tabify.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ function collectBucket(write, bucket, key, aggScale) {
const buckets = new TabifyBuckets(bucket[agg.id], agg.params);
if (buckets.length) {
buckets.forEach(function (subBucket, key) {
// if the bucket doesn't have value don't add it to the row
// we don't want rows like: { column1: undefined, column2: 10 }
const bucketValue = agg.getKey(subBucket, key);

This comment was marked as resolved.

const hasBucketValue = typeof bucketValue !== 'undefined';
if (hasBucketValue) {
Expand All @@ -63,7 +65,7 @@ function collectBucket(write, bucket, key, aggScale) {
write.bucketBuffer.pop();
}
});
} else if (write.partialRows && write.metricsForAllBuckets && write.minimalColumns) {
} else if (write.partialRows) {
// we don't have any buckets, but we do have metrics at this
// level, then pass all the empty buckets and jump back in for
// the metrics.
Expand All @@ -83,7 +85,7 @@ function collectBucket(write, bucket, key, aggScale) {
if (aggScale !== 1) {
value *= aggScale;
}
write.rowBuffer[column.id] = value;
write.metricBuffer.push({ id: column.id, value: value });

if (!write.aggStack.length) {
// row complete
Expand All @@ -93,6 +95,8 @@ function collectBucket(write, bucket, key, aggScale) {
collectBucket(write, bucket, key, aggScale);
}

write.metricBuffer.pop();

break;
}

Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/agg_table/__tests__/_group.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import $ from 'jquery';
import ngMock from 'ng_mock';
import expect from 'expect.js';
import fixtures from 'fixtures/fake_hierarchical_data';
import { TableResponseHandlerProvider } from '../../vis/response_handlers/table';
import { LegacyResponseHandlerProvider } from '../../vis/response_handlers/legacy';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { VisProvider } from '../../vis';
describe('AggTableGroup Directive', function () {
Expand All @@ -34,7 +34,7 @@ describe('AggTableGroup Directive', function () {

beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function ($injector, Private) {
tableAggResponse = Private(TableResponseHandlerProvider).handler;
tableAggResponse = Private(LegacyResponseHandlerProvider).handler;
indexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);
Vis = Private(VisProvider);

Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/agg_table/__tests__/_table.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import ngMock from 'ng_mock';
import expect from 'expect.js';
import fixtures from 'fixtures/fake_hierarchical_data';
import sinon from 'sinon';
import { TableResponseHandlerProvider } from '../../vis/response_handlers/table';
import { LegacyResponseHandlerProvider } from '../../vis/response_handlers/legacy';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { VisProvider } from '../../vis';
describe('AggTable Directive', function () {
Expand All @@ -37,7 +37,7 @@ describe('AggTable Directive', function () {

beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function ($injector, Private, config) {
tableAggResponse = Private(TableResponseHandlerProvider).handler;
tableAggResponse = Private(LegacyResponseHandlerProvider).handler;
indexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);
Vis = Private(VisProvider);
settings = config;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import ngMock from 'ng_mock';
import expect from 'expect.js';
import sinon from 'sinon';
import { AggResponseIndexProvider } from '../../../agg_response';
import { BasicResponseHandlerProvider } from '../../response_handlers/basic';
import { VislibResponseHandlerProvider } from '../../response_handlers/vislib';

describe('renderbot#buildChartData', function () {
let buildChartData;
Expand All @@ -31,7 +31,7 @@ describe('renderbot#buildChartData', function () {
beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function (Private) {
aggResponse = Private(AggResponseIndexProvider);
buildChartData = Private(BasicResponseHandlerProvider).handler;
buildChartData = Private(VislibResponseHandlerProvider).handler;
}));

describe('for hierarchical vis', function () {
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/vis/__tests__/response_handlers/basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import ngMock from 'ng_mock';
import expect from 'expect.js';
import { BasicResponseHandlerProvider } from '../../response_handlers/basic';
import { VislibResponseHandlerProvider } from '../../response_handlers/vislib';
import { VisProvider } from '../..';
import fixtures from 'fixtures/fake_hierarchical_data';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
Expand All @@ -39,7 +39,7 @@ describe('Basic Response Handler', function () {

beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function (Private) {
basicResponseHandler = Private(BasicResponseHandlerProvider).handler;
basicResponseHandler = Private(VislibResponseHandlerProvider).handler;
Vis = Private(VisProvider);
indexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);
}));
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/__tests__/vis_types/vislib_vis_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('Vislib Vis Type', function () {
describe('initialization', () => {
it('should set the basic response handler if not set', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

it should set the **vislib** response handler if not set

const visType = new VislibVisType(visConfig);
expect(visType.responseHandler).to.equal('basic');
expect(visType.responseHandler).to.equal('vislib');
});

it('should not change response handler if its already set', () => {
Expand Down
4 changes: 1 addition & 3 deletions src/ui/public/vis/request_handlers/courier.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ const CourierRequestHandlerProvider = function () {
*/
async function buildTabularInspectorData(vis, searchSource, aggConfigs) {
const table = tabifyAggResponse(aggConfigs, searchSource.finalResponse, {
canSplit: false,
asAggConfigResults: false,
partialRows: true,
isHierarchical: vis.isHierarchical(),
metricsAtAllLevels: vis.isHierarchical(),
});
const columns = table.columns.map((col, index) => {
const field = col.aggConfig.getField();
Expand Down
Loading