From f5f0601e53cd5cd90f46713fba12595396e10446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 16 Jan 2017 17:00:28 +0100 Subject: [PATCH 1/4] Add test to check if NULL category count values properly --- test/acceptance/dataviews/aggregation.js | 51 ++++++++++++++++++------ 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 2986a174..27018ae6 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/test/acceptance/dataviews/aggregation.js @@ -13,8 +13,11 @@ describe('aggregations', function() { } }); - function aggregationOperationMapConfig(operation) { - return { + function aggregationOperationMapConfig(operation, column, aggregationColumn) { + column = column || 'adm0name'; + aggregationColumn = aggregationColumn || 'pop_max'; + + var mapConfig = { version: '1.5.0', layers: [ { @@ -23,20 +26,22 @@ describe('aggregations', function() { sql: 'select * from populated_places_simple_reduced', cartocss: '#layer0 { marker-fill: red; marker-width: 10; }', cartocss_version: '2.0.1', - widgets: { - adm0name: { - type: 'aggregation', - options: { - column: 'adm0name', - aggregation: operation, - aggregationColumn: 'pop_max' - } - } - } + widgets: {} } } ] }; + + mapConfig.layers[0].options.widgets[column] = { + type: 'aggregation', + options: { + column: column, + aggregation: operation, + aggregationColumn: aggregationColumn + } + }; + + return mapConfig; } var operations = ['count', 'sum', 'avg', 'max', 'min']; @@ -56,4 +61,26 @@ describe('aggregations', function() { }); }); }); + + it('should count NULL category', function (done) { + this.testClient = new TestClient(aggregationOperationMapConfig('count', 'namepar')); + this.testClient.getDataview('namepar', { own_filter: 0 }, function (err, aggregation) { + assert.ifError(err); + + assert.ok(aggregation); + assert.equal(aggregation.type, 'aggregation'); + assert.ok(aggregation.categories); + + var hasNullCategory = false; + aggregation.categories.forEach(function (category) { + if (category.category === null) { + assert.ok(category.value > 0); + hasNullCategory = true; + } + }); + assert.ok(hasNullCategory, 'there is no category NULL'); + + done(); + }); + }); }); From 6555353e0ec63fb0a4767cf92ee54c8f0c5b33a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 16 Jan 2017 19:23:08 +0100 Subject: [PATCH 2/4] Improve test to handle NULL values in category and aggregation columns using any operation --- test/acceptance/dataviews/aggregation.js | 48 ++++++++++++++---------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 27018ae6..1f45c0c5 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/test/acceptance/dataviews/aggregation.js @@ -3,7 +3,7 @@ require('../../support/test_helper'); var assert = require('../../support/assert'); var TestClient = require('../../support/test-client'); -describe('aggregations', function() { +describe('aggregations happy cases', function() { afterEach(function(done) { if (this.testClient) { @@ -13,9 +13,10 @@ describe('aggregations', function() { } }); - function aggregationOperationMapConfig(operation, column, aggregationColumn) { + function aggregationOperationMapConfig(operation, query, column, aggregationColumn) { column = column || 'adm0name'; aggregationColumn = aggregationColumn || 'pop_max'; + query = query || 'select * from populated_places_simple_reduced'; var mapConfig = { version: '1.5.0', @@ -23,7 +24,7 @@ describe('aggregations', function() { { type: 'mapnik', options: { - sql: 'select * from populated_places_simple_reduced', + sql: query, cartocss: '#layer0 { marker-fill: red; marker-width: 10; }', cartocss_version: '2.0.1', widgets: {} @@ -62,25 +63,34 @@ describe('aggregations', function() { }); }); - it('should count NULL category', function (done) { - this.testClient = new TestClient(aggregationOperationMapConfig('count', 'namepar')); - this.testClient.getDataview('namepar', { own_filter: 0 }, function (err, aggregation) { - assert.ifError(err); + var query = [ + 'select 1 as val, \'a\' as cat, ST_Transform(ST_SetSRID(ST_MakePoint(0,0),4326),3857) as the_geom_webmercator', + 'select null, \'b\', ST_Transform(ST_SetSRID(ST_MakePoint(0,1),4326),3857)', + 'select null, \'b\', ST_Transform(ST_SetSRID(ST_MakePoint(1,0),4326),3857)', + 'select null, null, ST_Transform(ST_SetSRID(ST_MakePoint(1,1),4326),3857)' + ].join(' UNION ALL '); - assert.ok(aggregation); - assert.equal(aggregation.type, 'aggregation'); - assert.ok(aggregation.categories); + operations.forEach(function (operation) { + it('should handle NULL values in category and aggregation columns using "' + operation + '" as aggregation operation', function (done) { + this.testClient = new TestClient(aggregationOperationMapConfig(operation, query, 'cat', 'val')); + this.testClient.getDataview('cat', { own_filter: 0 }, function (err, aggregation) { + assert.ifError(err); - var hasNullCategory = false; - aggregation.categories.forEach(function (category) { - if (category.category === null) { - assert.ok(category.value > 0); - hasNullCategory = true; - } + assert.ok(aggregation); + assert.equal(aggregation.type, 'aggregation'); + assert.ok(aggregation.categories); + + var hasNullCategory = false; + aggregation.categories.forEach(function (category) { + if (category.category === null) { + assert.ok(category.value > 0); + hasNullCategory = true; + } + }); + assert.ok(hasNullCategory, 'there is no category NULL'); + + done(); }); - assert.ok(hasNullCategory, 'there is no category NULL'); - - done(); }); }); }); From a4a1fb930aae8b45711cae543409cbd36c260c81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 17 Jan 2017 17:09:17 +0100 Subject: [PATCH 3/4] Be able to not compute NULL categories and null values wheter aggregation operation is not 'count' --- lib/cartodb/models/dataview/aggregation.js | 43 +++++++++++++++++----- test/acceptance/dataviews/aggregation.js | 20 ++++++++-- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 3b0642b9..a80efbc7 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -19,25 +19,37 @@ var rankedCategoriesQueryTpl = dot.template([ ' SELECT {{=it._column}} AS category, {{=it._aggregation}} AS value,', ' row_number() OVER (ORDER BY {{=it._aggregation}} desc) as rank', ' FROM ({{=it._query}}) _cdb_aggregation_all', + ' {{?it._aggregationColumn!==null}}WHERE {{=it._aggregationColumn}} IS NOT NULL{{?}}', ' GROUP BY {{=it._column}}', ' ORDER BY 2 DESC', ')' ].join('\n')); -var categoriesSummaryQueryTpl = dot.template([ - 'categories_summary AS(', - ' SELECT count(1) categories_count, max(value) max_val, min(value) min_val', +var categoriesSummaryMinMaxQueryTpl = dot.template([ + 'categories_summary_min_max AS(', + ' SELECT max(value) max_val, min(value) min_val', ' FROM categories', ')' ].join('\n')); +var categoriesSummaryCountQueryTpl = dot.template([ + 'categories_summary_count AS(', + ' SELECT count(1) AS categories_count', + ' FROM (', + ' SELECT {{=it._column}} AS category', + ' FROM ({{=it._query}}) _cdb_categories', + ' GROUP BY {{=it._column}}', + ' ) _cdb_categories_count', + ')' +].join('\n')); + var rankedAggregationQueryTpl = dot.template([ 'SELECT CAST(category AS text), value, false as agg, nulls_count, min_val, max_val, count, categories_count', - ' FROM categories, summary, categories_summary', + ' FROM categories, summary, categories_summary_min_max, categories_summary_count', ' WHERE rank < {{=it._limit}}', 'UNION ALL', 'SELECT \'Other\' category, sum(value), true as agg, nulls_count, min_val, max_val, count, categories_count', - ' FROM categories, summary, categories_summary', + ' FROM categories, summary, categories_summary_min_max, categories_summary_count', ' WHERE rank >= {{=it._limit}}', 'GROUP BY nulls_count, min_val, max_val, count, categories_count' ].join('\n')); @@ -45,7 +57,7 @@ var rankedAggregationQueryTpl = dot.template([ var aggregationQueryTpl = dot.template([ 'SELECT CAST({{=it._column}} AS text) AS category, {{=it._aggregation}} AS value, false as agg,', ' nulls_count, min_val, max_val, count, categories_count', - 'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary', + 'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count', 'GROUP BY category, nulls_count, min_val, max_val, count, categories_count', 'ORDER BY value DESC' ].join('\n')); @@ -114,6 +126,7 @@ Aggregation.prototype.sql = function(psql, override, callback) { var _query = this.query; var aggregationSql; + if (!!override.ownFilter) { aggregationSql = [ "WITH", @@ -125,9 +138,14 @@ Aggregation.prototype.sql = function(psql, override, callback) { rankedCategoriesQueryTpl({ _query: _query, _column: this.column, - _aggregation: this.getAggregationSql() + _aggregation: this.getAggregationSql(), + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null }), - categoriesSummaryQueryTpl({ + categoriesSummaryMinMaxQueryTpl({ + _query: _query, + _column: this.column + }), + categoriesSummaryCountQueryTpl({ _query: _query, _column: this.column }) @@ -150,9 +168,14 @@ Aggregation.prototype.sql = function(psql, override, callback) { rankedCategoriesQueryTpl({ _query: _query, _column: this.column, - _aggregation: this.getAggregationSql() + _aggregation: this.getAggregationSql(), + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null }), - categoriesSummaryQueryTpl({ + categoriesSummaryMinMaxQueryTpl({ + _query: _query, + _column: this.column + }), + categoriesSummaryCountQueryTpl({ _query: _query, _column: this.column }) diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 1f45c0c5..9fc9a219 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/test/acceptance/dataviews/aggregation.js @@ -71,7 +71,14 @@ describe('aggregations happy cases', function() { ].join(' UNION ALL '); operations.forEach(function (operation) { - it('should handle NULL values in category and aggregation columns using "' + operation + '" as aggregation operation', function (done) { + var not = operation === 'count' ? ' not ' : ' '; + var description = 'should' + + not + + 'handle NULL values in category and aggregation columns using "' + + operation + + '" as aggregation operation'; + + it(description, function (done) { this.testClient = new TestClient(aggregationOperationMapConfig(operation, query, 'cat', 'val')); this.testClient.getDataview('cat', { own_filter: 0 }, function (err, aggregation) { assert.ifError(err); @@ -79,15 +86,22 @@ describe('aggregations happy cases', function() { assert.ok(aggregation); assert.equal(aggregation.type, 'aggregation'); assert.ok(aggregation.categories); + assert.equal(aggregation.categoriesCount, 3); + assert.equal(aggregation.count, 4); + assert.equal(aggregation.nulls, 1); var hasNullCategory = false; aggregation.categories.forEach(function (category) { if (category.category === null) { - assert.ok(category.value > 0); hasNullCategory = true; } }); - assert.ok(hasNullCategory, 'there is no category NULL'); + + if (operation === 'count') { + assert.ok(hasNullCategory, 'aggregation has not a category NULL'); + } else { + assert.ok(!hasNullCategory, 'aggregation has category NULL'); + } done(); }); From 83301238d236ee0e5727a6934f3b98bbedad7519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 17 Jan 2017 17:10:08 +0100 Subject: [PATCH 4/4] Port changes to overviews --- .../models/dataview/overviews/aggregation.js | 42 ++++++++++++++----- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/models/dataview/overviews/aggregation.js b/lib/cartodb/models/dataview/overviews/aggregation.js index a3d01b06..da63b27f 100644 --- a/lib/cartodb/models/dataview/overviews/aggregation.js +++ b/lib/cartodb/models/dataview/overviews/aggregation.js @@ -18,25 +18,37 @@ var rankedCategoriesQueryTpl = dot.template([ ' SELECT {{=it._column}} AS category, {{=it._aggregation}} AS value,', ' row_number() OVER (ORDER BY {{=it._aggregation}} desc) as rank', ' FROM ({{=it._query}}) _cdb_aggregation_all', + ' {{?it._aggregationColumn!==null}}WHERE {{=it._aggregationColumn}} IS NOT NULL{{?}}', ' GROUP BY {{=it._column}}', ' ORDER BY 2 DESC', ')' ].join('\n')); -var categoriesSummaryQueryTpl = dot.template([ - 'categories_summary AS(', - ' SELECT count(1) categories_count, max(value) max_val, min(value) min_val', +var categoriesSummaryMinMaxQueryTpl = dot.template([ + 'categories_summary_min_max AS(', + ' SELECT max(value) max_val, min(value) min_val', ' FROM categories', ')' ].join('\n')); +var categoriesSummaryCountQueryTpl = dot.template([ + 'categories_summary_count AS(', + ' SELECT count(1) AS categories_count', + ' FROM (', + ' SELECT {{=it._column}} AS category', + ' FROM ({{=it._query}}) _cdb_categories', + ' GROUP BY {{=it._column}}', + ' ) _cdb_categories_count', + ')' +].join('\n')); + var rankedAggregationQueryTpl = dot.template([ 'SELECT CAST(category AS text), value, false as agg, nulls_count, min_val, max_val, count, categories_count', - ' FROM categories, summary, categories_summary', + ' FROM categories, summary, categories_summary_min_max, categories_summary_count', ' WHERE rank < {{=it._limit}}', 'UNION ALL', 'SELECT \'Other\' category, sum(value), true as agg, nulls_count, min_val, max_val, count, categories_count', - ' FROM categories, summary, categories_summary', + ' FROM categories, summary, categories_summary_min_max, categories_summary_count', ' WHERE rank >= {{=it._limit}}', 'GROUP BY nulls_count, min_val, max_val, count, categories_count' ].join('\n')); @@ -44,7 +56,7 @@ var rankedAggregationQueryTpl = dot.template([ var aggregationQueryTpl = dot.template([ 'SELECT CAST({{=it._column}} AS text) AS category, {{=it._aggregation}} AS value, false as agg,', ' nulls_count, min_val, max_val, count, categories_count', - 'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary', + 'FROM ({{=it._query}}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count', 'GROUP BY category, nulls_count, min_val, max_val, count, categories_count', 'ORDER BY value DESC' ].join('\n')); @@ -85,9 +97,14 @@ Aggregation.prototype.sql = function(psql, override, callback) { rankedCategoriesQueryTpl({ _query: _query, _column: this.column, - _aggregation: this.getAggregationSql() + _aggregation: this.getAggregationSql(), + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null }), - categoriesSummaryQueryTpl({ + categoriesSummaryMinMaxQueryTpl({ + _query: _query, + _column: this.column + }), + categoriesSummaryCountQueryTpl({ _query: _query, _column: this.column }) @@ -110,9 +127,14 @@ Aggregation.prototype.sql = function(psql, override, callback) { rankedCategoriesQueryTpl({ _query: _query, _column: this.column, - _aggregation: this.getAggregationSql() + _aggregation: this.getAggregationSql(), + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null }), - categoriesSummaryQueryTpl({ + categoriesSummaryMinMaxQueryTpl({ + _query: _query, + _column: this.column + }), + categoriesSummaryCountQueryTpl({ _query: _query, _column: this.column })