diff --git a/NEWS.md b/NEWS.md index ec0ceef7..d0fbde2c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,8 +1,10 @@ # Changelog ## 2.88.3 -Released 2017-mm-dd +Released 2017-03-02 +Bug fixes: +- Category dataviews now uses the proper aggregation function for the 'Other' category. See https://github.com/CartoDB/Windshaft-cartodb/issues/628 ## 2.88.2 Released 2017-02-23 diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index a80efbc7..c15f0506 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -48,7 +48,8 @@ var rankedAggregationQueryTpl = dot.template([ ' 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', + 'SELECT \'Other\' category, {{=it._aggregationFn}}(value) as value, true as agg, nulls_count, min_val, max_val,', + ' count, categories_count', ' 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' @@ -129,27 +130,7 @@ Aggregation.prototype.sql = function(psql, override, callback) { if (!!override.ownFilter) { aggregationSql = [ - "WITH", - [ - summaryQueryTpl({ - _query: _query, - _column: this.column - }), - rankedCategoriesQueryTpl({ - _query: _query, - _column: this.column, - _aggregation: this.getAggregationSql(), - _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null - }), - categoriesSummaryMinMaxQueryTpl({ - _query: _query, - _column: this.column - }), - categoriesSummaryCountQueryTpl({ - _query: _query, - _column: this.column - }) - ].join(',\n'), + this.getCategoriesCTESql(_query, this.column, this.aggregation, this.aggregationColumn), aggregationQueryTpl({ _query: _query, _column: this.column, @@ -159,30 +140,11 @@ Aggregation.prototype.sql = function(psql, override, callback) { ].join('\n'); } else { aggregationSql = [ - "WITH", - [ - summaryQueryTpl({ - _query: _query, - _column: this.column - }), - rankedCategoriesQueryTpl({ - _query: _query, - _column: this.column, - _aggregation: this.getAggregationSql(), - _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null - }), - categoriesSummaryMinMaxQueryTpl({ - _query: _query, - _column: this.column - }), - categoriesSummaryCountQueryTpl({ - _query: _query, - _column: this.column - }) - ].join(',\n'), + this.getCategoriesCTESql(_query, this.column, this.aggregation, this.aggregationColumn), rankedAggregationQueryTpl({ _query: _query, _column: this.column, + _aggregationFn: this.aggregation !== 'count' ? this.aggregation : 'sum', _limit: CATEGORIES_LIMIT }) ].join('\n'); @@ -193,6 +155,32 @@ Aggregation.prototype.sql = function(psql, override, callback) { return callback(null, aggregationSql); }; +Aggregation.prototype.getCategoriesCTESql = function(query, column, aggregation, aggregationColumn) { + return [ + "WITH", + [ + summaryQueryTpl({ + _query: query, + _column: column + }), + rankedCategoriesQueryTpl({ + _query: query, + _column: column, + _aggregation: this.getAggregationSql(), + _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null + }), + categoriesSummaryMinMaxQueryTpl({ + _query: query, + _column: column + }), + categoriesSummaryCountQueryTpl({ + _query: query, + _column: column + }) + ].join(',\n') + ].join('\n'); +}; + var aggregationFnQueryTpl = dot.template('{{=it._aggregationFn}}({{=it._aggregationColumn}})'); Aggregation.prototype.getAggregationSql = function() { return aggregationFnQueryTpl({ diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 1af8d15d..6e8f6f65 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -90,7 +90,7 @@ }, "mime-types": { "version": "2.1.14", - "from": "mime-types@>=2.1.7 <2.2.0", + "from": "mime-types@>=2.1.13 <2.2.0", "resolved": "https://registry.npmjs.org/mime-types/-/mime-types-2.1.14.tgz", "dependencies": { "mime-db": { @@ -228,9 +228,9 @@ } }, "safe-json-stringify": { - "version": "1.0.3", + "version": "1.0.4", "from": "safe-json-stringify@>=1.0.0 <2.0.0", - "resolved": "https://registry.npmjs.org/safe-json-stringify/-/safe-json-stringify-1.0.3.tgz" + "resolved": "https://registry.npmjs.org/safe-json-stringify/-/safe-json-stringify-1.0.4.tgz" }, "moment": { "version": "2.17.1", @@ -609,7 +609,7 @@ }, "type-is": { "version": "1.6.14", - "from": "type-is@>=1.6.10 <1.7.0", + "from": "type-is@>=1.6.6 <1.7.0", "resolved": "https://registry.npmjs.org/type-is/-/type-is-1.6.14.tgz", "dependencies": { "media-typer": { @@ -869,9 +869,9 @@ } }, "is-my-json-valid": { - "version": "2.15.0", + "version": "2.16.0", "from": "is-my-json-valid@>=2.12.4 <3.0.0", - "resolved": "https://registry.npmjs.org/is-my-json-valid/-/is-my-json-valid-2.15.0.tgz", + "resolved": "https://registry.npmjs.org/is-my-json-valid/-/is-my-json-valid-2.16.0.tgz", "dependencies": { "generate-function": { "version": "2.0.0", @@ -976,9 +976,9 @@ } }, "sshpk": { - "version": "1.10.2", + "version": "1.11.0", "from": "sshpk@>=1.7.0 <2.0.0", - "resolved": "https://registry.npmjs.org/sshpk/-/sshpk-1.10.2.tgz", + "resolved": "https://registry.npmjs.org/sshpk/-/sshpk-1.11.0.tgz", "dependencies": { "asn1": { "version": "0.2.3", @@ -3919,7 +3919,7 @@ }, "minimatch": { "version": "3.0.3", - "from": "minimatch@>=2.0.0 <3.0.0||>=3.0.0 <4.0.0", + "from": "minimatch@>=3.0.2 <4.0.0", "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.3.tgz", "dependencies": { "brace-expansion": { @@ -4030,7 +4030,7 @@ }, "minimatch": { "version": "3.0.3", - "from": "minimatch@>=2.0.0 <3.0.0||>=3.0.0 <4.0.0", + "from": "minimatch@>=3.0.0 <4.0.0", "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.3.tgz", "dependencies": { "brace-expansion": { diff --git a/test/acceptance/dataviews/aggregation.js b/test/acceptance/dataviews/aggregation.js index 9fc9a219..259cd2af 100644 --- a/test/acceptance/dataviews/aggregation.js +++ b/test/acceptance/dataviews/aggregation.js @@ -1,5 +1,4 @@ require('../../support/test_helper'); - var assert = require('../../support/assert'); var TestClient = require('../../support/test-client'); @@ -107,4 +106,43 @@ describe('aggregations happy cases', function() { }); }); }); + + var operations_and_values = {'count': 9, 'sum': 45, 'avg': 5, 'max': 9, 'min': 1}; + + var query_other = [ + 'select generate_series(1,3) as val, \'other_a\' as cat, NULL as the_geom_webmercator', + 'select generate_series(4,6) as val, \'other_b\' as cat, NULL as the_geom_webmercator', + 'select generate_series(7,9) as val, \'other_c\' as cat, NULL as the_geom_webmercator', + 'select generate_series(10,12) as val, \'category_1\' as cat, NULL as the_geom_webmercator', + 'select generate_series(10,12) as val, \'category_2\' as cat, NULL as the_geom_webmercator', + 'select generate_series(10,12) as val, \'category_3\' as cat, NULL as the_geom_webmercator', + 'select generate_series(10,12) as val, \'category_4\' as cat, NULL as the_geom_webmercator', + 'select generate_series(10,12) as val, \'category_5\' as cat, NULL as the_geom_webmercator' + ].join(' UNION ALL '); + + Object.keys(operations_and_values).forEach(function (operation) { + var description = 'should aggregate OTHER category using "' + operation + '"'; + + it(description, function (done) { + this.testClient = new TestClient(aggregationOperationMapConfig(operation, query_other, 'cat', 'val')); + this.testClient.getDataview('cat', { own_filter: 0 }, function (err, aggregation) { + assert.ifError(err); + + assert.ok(aggregation); + assert.equal(aggregation.type, 'aggregation'); + assert.ok(aggregation.categories); + assert.equal(aggregation.categoriesCount, 8); + assert.equal(aggregation.count, 24); + assert.equal(aggregation.nulls, 0); + + var aggregated_categories = aggregation.categories.filter( function(category) { + return category.agg === true; + }); + assert.equal(aggregated_categories.length, 1); + assert.equal(aggregated_categories[0].value, operations_and_values[operation]); + + done(); + }); + }); + }); });