From 6fd3388fa2de1ac9fd855ea102146fe9636f8969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 18:38:54 +0200 Subject: [PATCH 01/28] Replace dot templates by ES6 string templates --- lib/cartodb/models/dataview/aggregation.js | 252 ++++++++++++--------- 1 file changed, 149 insertions(+), 103 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 77f37723..1a4fc3ed 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -2,90 +2,134 @@ var _ = require('underscore'); var BaseWidget = require('./base'); var debug = require('debug')('windshaft:widget:aggregation'); -var dot = require('dot'); -dot.templateSettings.strip = false; +const filteredQueryTpl = ctx => ` + filtered_source AS ( + SELECT * + FROM (${ctx._query}) _cdb_filtered_source + ${ctx._aggregationColumn && ctx._isFloatColumn ? ` + WHERE + ${ctx._aggregationColumn} != 'infinity'::float + AND + ${ctx._aggregationColumn} != '-infinity'::float + AND + ${ctx._aggregationColumn} != 'NaN'::float` : + '' + } + ) +`; -var filteredQueryTpl = dot.template([ - 'filtered_source AS (', - ' SELECT *', - ' FROM ({{=it._query}}) _cdb_filtered_source', - ' {{?it._aggregationColumn && it._isFloatColumn}}WHERE', - ' {{=it._aggregationColumn}} != \'infinity\'::float', - ' AND', - ' {{=it._aggregationColumn}} != \'-infinity\'::float', - ' AND', - ' {{=it._aggregationColumn}} != \'NaN\'::float{{?}}', - ')' -].join(' \n')); +const summaryQueryTpl = ctx => ` + summary AS ( + SELECT + count(1) AS count, + sum(CASE WHEN ${ctx._column} IS NULL THEN 1 ELSE 0 END) AS nulls_count + ${ctx._isFloatColumn ? `, + sum( + CASE + WHEN ${ctx._aggregationColumn} = 'infinity'::float OR ${ctx._aggregationColumn} = '-infinity'::float + THEN 1 + ELSE 0 + END + ) AS infinities_count, + sum(CASE WHEN ${ctx._aggregationColumn} = 'NaN'::float THEN 1 ELSE 0 END) AS nans_count` : + '' + } + FROM (${ctx._query}) _cdb_aggregation_nulls + ) +`; -var summaryQueryTpl = dot.template([ - 'summary AS (', - ' SELECT', - ' count(1) AS count,', - ' sum(CASE WHEN {{=it._column}} IS NULL THEN 1 ELSE 0 END) AS nulls_count', - ' {{?it._isFloatColumn}},sum(', - ' CASE', - ' WHEN {{=it._aggregationColumn}} = \'infinity\'::float OR {{=it._aggregationColumn}} = \'-infinity\'::float', - ' THEN 1', - ' ELSE 0', - ' END', - ' ) AS infinities_count,', - ' sum(CASE WHEN {{=it._aggregationColumn}} = \'NaN\'::float THEN 1 ELSE 0 END) AS nans_count{{?}}', - ' FROM ({{=it._query}}) _cdb_aggregation_nulls', - ')' -].join('\n')); +const rankedCategoriesQueryTpl = ctx => ` + categories AS( + SELECT + ${ctx._column} AS category, + ${ctx._aggregation} AS value, + row_number() OVER (ORDER BY ${ctx._aggregation} desc) as rank + FROM filtered_source + ${ctx._aggregationColumn !== null ? `WHERE ${ctx._aggregationColumn} IS NOT NULL` : ''} + GROUP BY ${ctx._column} + ORDER BY 2 DESC + ) +`; -var rankedCategoriesQueryTpl = dot.template([ - 'categories AS(', - ' SELECT {{=it._column}} AS category, {{=it._aggregation}} AS value,', - ' row_number() OVER (ORDER BY {{=it._aggregation}} desc) as rank', - ' FROM filtered_source', - ' {{?it._aggregationColumn!==null}}WHERE {{=it._aggregationColumn}} IS NOT NULL{{?}}', - ' GROUP BY {{=it._column}}', - ' ORDER BY 2 DESC', - ')' -].join('\n')); +const categoriesSummaryMinMaxQueryTpl = () => ` + categories_summary_min_max AS( + SELECT + max(value) max_val, + min(value) min_val + FROM categories + ) +`; -var categoriesSummaryMinMaxQueryTpl = dot.template([ - 'categories_summary_min_max AS(', - ' SELECT max(value) max_val, min(value) min_val', - ' FROM categories', - ')' -].join('\n')); +const categoriesSummaryCountQueryTpl = ctx => ` + categories_summary_count AS( + SELECT count(1) AS categories_count + FROM ( + SELECT ${ctx._column} AS category + FROM (${ctx._query}) _cdb_categories + GROUP BY ${ctx._column} + ) _cdb_categories_count + ) +`; -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')); +const specialNumericValuesColumns = () => `, nans_count, infinities_count`; -var rankedAggregationQueryTpl = dot.template([ - 'SELECT CAST(category AS text), value, false as agg, nulls_count, min_val, max_val,', - ' count, categories_count{{?it._isFloatColumn}}, nans_count, infinities_count{{?}}', - ' FROM categories, summary, categories_summary_min_max, categories_summary_count', - ' WHERE rank < {{=it._limit}}', - 'UNION ALL', - 'SELECT \'Other\' category, {{=it._aggregationFn}}(value) as value, true as agg, nulls_count,', - ' min_val, max_val, count, categories_count{{?it._isFloatColumn}}, nans_count, infinities_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{{?it._isFloatColumn}}, nans_count, infinities_count{{?}}' -].join('\n')); +const rankedAggregationQueryTpl = ctx => ` + SELECT + CAST(category AS text), + value, + false as agg, + nulls_count, + min_val, + max_val, + count, + categories_count + ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + FROM categories, summary, categories_summary_min_max, categories_summary_count + WHERE rank < ${ctx._limit} + UNION ALL + SELECT + 'Other' category, + ${ctx._aggregationFn}(value) as value, + true as agg, + nulls_count, + min_val, + max_val, + count, + categories_count + ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + FROM categories, summary, categories_summary_min_max, categories_summary_count + WHERE rank >= ${ctx._limit} + GROUP BY + nulls_count, + min_val, + max_val, + count, + categories_count + ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } +`; -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{{?it._isFloatColumn}}, nans_count, infinities_count{{?}}', - '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{{?it._isFloatColumn}}, nans_count, infinities_count{{?}}', - 'ORDER BY value DESC' -].join('\n')); +const aggregationQueryTpl = ctx => ` + SELECT + CAST(${ctx._column} AS text) AS category, + ${ctx._aggregation} AS value, + false as agg, + nulls_count, + min_val, + max_val, + count, + categories_count + ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + FROM (${ctx._query}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count + GROUP BY + category, + nulls_count, + min_val, + max_val, + count, + categories_count + ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + ORDER BY value DESC +`; var CATEGORIES_LIMIT = 6; @@ -242,7 +286,8 @@ Aggregation.prototype.getCategoriesCTESql = function(query, column, aggregation, ].join('\n'); }; -var aggregationFnQueryTpl = dot.template('{{=it._aggregationFn}}({{=it._aggregationColumn}})'); +const aggregationFnQueryTpl = ctx => `${ctx._aggregationFn}(${ctx._aggregationColumn})`; + Aggregation.prototype.getAggregationSql = function() { return aggregationFnQueryTpl({ _aggregationFn: this.aggregation, @@ -290,32 +335,33 @@ Aggregation.prototype.format = function(result) { }; }; -var filterCategoriesQueryTpl = dot.template([ - 'SELECT {{=it._column}} AS category, {{=it._value}} AS value', - 'FROM ({{=it._query}}) _cdb_aggregation_search', - 'WHERE CAST({{=it._column}} as text) ILIKE {{=it._userQuery}}', - 'GROUP BY {{=it._column}}' -].join('\n')); - -var searchQueryTpl = dot.template([ - 'WITH', - 'search_unfiltered AS (', - ' {{=it._searchUnfiltered}}', - '),', - 'search_filtered AS (', - ' {{=it._searchFiltered}}', - '),', - 'search_union AS (', - ' SELECT * FROM search_unfiltered', - ' UNION ALL', - ' SELECT * FROM search_filtered', - ')', - 'SELECT category, sum(value) AS value', - 'FROM search_union', - 'GROUP BY category', - 'ORDER BY value desc' -].join('\n')); +const filterCategoriesQueryTpl = ctx => ` + SELECT + ${ctx._column} AS category, + ${ctx._value} AS value + FROM (${ctx._query}) _cdb_aggregation_search + WHERE CAST(${ctx._column} as text) ILIKE ${ctx._userQuery} + GROUP BY ${ctx._column} +`; +const searchQueryTpl = ctx => ` + WITH + search_unfiltered AS ( + ${ctx._searchUnfiltered} + ), + search_filtered AS ( + ${ctx._searchFiltered} + ), + search_union AS ( + SELECT * FROM search_unfiltered + UNION ALL + SELECT * FROM search_filtered + ) + SELECT category, sum(value) AS value + FROM search_union + GROUP BY category + ORDER BY value desc +`; Aggregation.prototype.search = function(psql, userQuery, callback) { var self = this; From 33e8657e35e090dbeedff19e06ba39bed746f06a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 18:40:09 +0200 Subject: [PATCH 02/28] Declare constants with const keyword --- lib/cartodb/models/dataview/aggregation.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 1a4fc3ed..8ba8621f 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -131,9 +131,9 @@ const aggregationQueryTpl = ctx => ` ORDER BY value DESC `; -var CATEGORIES_LIMIT = 6; +const CATEGORIES_LIMIT = 6; -var VALID_OPERATIONS = { +const VALID_OPERATIONS = { count: [], sum: ['aggregationColumn'], avg: ['aggregationColumn'], @@ -141,7 +141,7 @@ var VALID_OPERATIONS = { max: ['aggregationColumn'] }; -var TYPE = 'aggregation'; +const TYPE = 'aggregation'; /** { From 306df5be5a2d665ddf6500b65987c3712929de9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:16:08 +0200 Subject: [PATCH 03/28] Replace underscore's function by ES6 equivalents --- lib/cartodb/models/dataview/aggregation.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 8ba8621f..5cddac18 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -1,4 +1,3 @@ -var _ = require('underscore'); var BaseWidget = require('./base'); var debug = require('debug')('windshaft:widget:aggregation'); @@ -153,11 +152,13 @@ const TYPE = 'aggregation'; } */ function Aggregation(query, options, queries) { - if (!_.isString(options.column)) { + options = options || {}; + + if (typeof options.column !== 'string') { throw new Error('Aggregation expects `column` in widget options'); } - if (!_.isString(options.aggregation)) { + if (typeof options.aggregation !== 'string') { throw new Error('Aggregation expects `aggregation` operation in widget options'); } @@ -166,7 +167,8 @@ function Aggregation(query, options, queries) { } var requiredOptions = VALID_OPERATIONS[options.aggregation]; - var missingOptions = _.difference(requiredOptions, Object.keys(options)); + var missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); + if (missingOptions.length > 0) { throw new Error( "Aggregation '" + options.aggregation + "' is missing some options: " + missingOptions.join(',') @@ -315,11 +317,7 @@ Aggregation.prototype.format = function(result) { minValue = firstRow.min_val; maxValue = firstRow.max_val; categoriesCount = firstRow.categories_count; - - result.rows.forEach(function(row) { - categories.push(_.omit(row, 'count', 'nulls_count', 'min_val', - 'max_val', 'categories_count', 'nans_count', 'infinities_count')); - }); + categories = result.rows.map(({ category, value, agg }) => ({ category, value, agg })); } return { From 4518b7cb6e7436d18acea77c9dcce8c98d49e59a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:17:16 +0200 Subject: [PATCH 04/28] Declare requirement with const keyword --- lib/cartodb/models/dataview/aggregation.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 5cddac18..e8b431b4 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -1,5 +1,5 @@ -var BaseWidget = require('./base'); -var debug = require('debug')('windshaft:widget:aggregation'); +const BaseWidget = require('./base'); +const debug = require('debug')('windshaft:widget:aggregation'); const filteredQueryTpl = ctx => ` filtered_source AS ( From ee471184b91f59e04653a0c0ea3c4ca024135432 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:19:25 +0200 Subject: [PATCH 05/28] Use default values for input params --- lib/cartodb/models/dataview/aggregation.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index e8b431b4..b2cfd212 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -151,9 +151,7 @@ const TYPE = 'aggregation'; } } */ -function Aggregation(query, options, queries) { - options = options || {}; - +function Aggregation(query, options = {}, queries = {}) { if (typeof options.column !== 'string') { throw new Error('Aggregation expects `column` in widget options'); } From 4d1a53c20f1abfef89cd554b2e4f4a13d7b76685 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:27:25 +0200 Subject: [PATCH 06/28] Use string templates to build error message --- lib/cartodb/models/dataview/aggregation.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index b2cfd212..71394211 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -153,24 +153,22 @@ const TYPE = 'aggregation'; */ function Aggregation(query, options = {}, queries = {}) { if (typeof options.column !== 'string') { - throw new Error('Aggregation expects `column` in widget options'); + throw new Error(`Aggregation expects 'column' in widget options`); } if (typeof options.aggregation !== 'string') { - throw new Error('Aggregation expects `aggregation` operation in widget options'); + throw new Error(`Aggregation expects 'aggregation' operation in widget options`); } if (!VALID_OPERATIONS[options.aggregation]) { - throw new Error("Aggregation does not support '" + options.aggregation + "' operation"); + throw new Error(`Aggregation does not support '${options.aggregation}' operation`); } var requiredOptions = VALID_OPERATIONS[options.aggregation]; var missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); if (missingOptions.length > 0) { - throw new Error( - "Aggregation '" + options.aggregation + "' is missing some options: " + missingOptions.join(',') - ); + throw new Error(`Aggregation '${options.aggregation}' is missing some options: ${missingOptions.join(',')}`); } BaseWidget.apply(this); @@ -305,7 +303,6 @@ Aggregation.prototype.format = function(result) { var maxValue = 0; var categoriesCount = 0; - if (result.rows.length) { var firstRow = result.rows[0]; count = firstRow.count; From 75645e2d7af3dcc2b9ca79a6197381c40cad156d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:34:09 +0200 Subject: [PATCH 07/28] Use string templates to build categories CTE sql --- lib/cartodb/models/dataview/aggregation.js | 28 ++++++++++------------ 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 71394211..43dff8c0 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -251,37 +251,35 @@ Aggregation.prototype.sql = function(psql, override, callback) { }; Aggregation.prototype.getCategoriesCTESql = function(query, column, aggregation, aggregationColumn, isFloatColumn) { - return [ - "WITH", - [ - filteredQueryTpl({ + return ` + WITH + ${filteredQueryTpl({ _isFloatColumn: isFloatColumn, _query: this.query, _column: this.column, _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null - }), - summaryQueryTpl({ + })}, + ${summaryQueryTpl({ _isFloatColumn: isFloatColumn, _query: query, _column: column, _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null - }), - rankedCategoriesQueryTpl({ + })}, + ${rankedCategoriesQueryTpl({ _query: query, _column: column, _aggregation: this.getAggregationSql(), _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null - }), - categoriesSummaryMinMaxQueryTpl({ + })}, + ${categoriesSummaryMinMaxQueryTpl({ _query: query, _column: column - }), - categoriesSummaryCountQueryTpl({ + })}, + ${categoriesSummaryCountQueryTpl({ _query: query, _column: column - }) - ].join(',\n') - ].join('\n'); + })} + `; }; const aggregationFnQueryTpl = ctx => `${ctx._aggregationFn}(${ctx._aggregationColumn})`; From 2f4f719f558f7e41701ffa713c9fc77e1c32cdbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Sep 2017 19:42:25 +0200 Subject: [PATCH 08/28] Use object properties in class method instead of passing them as parameters --- lib/cartodb/models/dataview/aggregation.js | 28 +++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 43dff8c0..ed0261ae 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -250,34 +250,34 @@ Aggregation.prototype.sql = function(psql, override, callback) { return callback(null, aggregationSql); }; -Aggregation.prototype.getCategoriesCTESql = function(query, column, aggregation, aggregationColumn, isFloatColumn) { +Aggregation.prototype.getCategoriesCTESql = function() { return ` WITH ${filteredQueryTpl({ - _isFloatColumn: isFloatColumn, + _isFloatColumn: this._isFloatColumn, _query: this.query, _column: this.column, - _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null })}, ${summaryQueryTpl({ - _isFloatColumn: isFloatColumn, - _query: query, - _column: column, - _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null + _isFloatColumn: this._isFloatColumn, + _query: this.query, + _column: this.column, + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null })}, ${rankedCategoriesQueryTpl({ - _query: query, - _column: column, + _query: this.query, + _column: this.column, _aggregation: this.getAggregationSql(), - _aggregationColumn: aggregation !== 'count' ? aggregationColumn : null + _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null })}, ${categoriesSummaryMinMaxQueryTpl({ - _query: query, - _column: column + _query: this.query, + _column: this.column })}, ${categoriesSummaryCountQueryTpl({ - _query: query, - _column: column + _query: this.query, + _column: this.column })} `; }; From a434015d5b3e020175dc38e248aece34c13b1484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 11:27:03 +0200 Subject: [PATCH 09/28] Move categories CTE template out of aggregation class --- lib/cartodb/models/dataview/aggregation.js | 111 ++++++++++----------- 1 file changed, 55 insertions(+), 56 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index ed0261ae..627eb73a 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -130,6 +130,41 @@ const aggregationQueryTpl = ctx => ` ORDER BY value DESC `; +const categoriesCTESqlTpl = ctx => ` + WITH + ${filteredQueryTpl({ + _isFloatColumn: ctx.isFloatColumn, + _query: ctx.query, + _column: ctx.column, + _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + })}, + ${summaryQueryTpl({ + _isFloatColumn: ctx.isFloatColumn, + _query: ctx.query, + _column: ctx.column, + _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + })}, + ${rankedCategoriesQueryTpl({ + _query: ctx.query, + _column: ctx.column, + _aggregation: aggregationFnQueryTpl({ + aggregation: ctx.aggregation, + aggregationColumn: ctx.aggregationColumn || 1 + }), + _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + })}, + ${categoriesSummaryMinMaxQueryTpl({ + _query: ctx.query, + _column: ctx.column + })}, + ${categoriesSummaryCountQueryTpl({ + _query: ctx.query, + _column: ctx.column + })} +`; + +const aggregationFnQueryTpl = ctx => `${ctx.aggregation}(${ctx.aggregationColumn})`; + const CATEGORIES_LIMIT = 6; const VALID_OPERATIONS = { @@ -209,32 +244,37 @@ Aggregation.prototype.sql = function(psql, override, callback) { var aggregationSql; + + if (!!override.ownFilter) { aggregationSql = [ - this.getCategoriesCTESql( - _query, - this.column, - this.aggregation, - this.aggregationColumn, - this._isFloatColumn - ), + categoriesCTESqlTpl({ + query: _query, + column: this.column, + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn, + isFloatColumn: this._isFloatColumn + }), aggregationQueryTpl({ _isFloatColumn: this._isFloatColumn, _query: _query, _column: this.column, - _aggregation: this.getAggregationSql(), + _aggregation: aggregationFnQueryTpl({ + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn || 1 + }), _limit: CATEGORIES_LIMIT }) ].join('\n'); } else { aggregationSql = [ - this.getCategoriesCTESql( - _query, - this.column, - this.aggregation, - this.aggregationColumn, - this._isFloatColumn - ), + categoriesCTESqlTpl({ + query: _query, + column: this.column, + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn, + isFloatColumn: this._isFloatColumn + }), rankedAggregationQueryTpl({ _isFloatColumn: this._isFloatColumn, _query: _query, @@ -250,47 +290,6 @@ Aggregation.prototype.sql = function(psql, override, callback) { return callback(null, aggregationSql); }; -Aggregation.prototype.getCategoriesCTESql = function() { - return ` - WITH - ${filteredQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: this.query, - _column: this.column, - _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null - })}, - ${summaryQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: this.query, - _column: this.column, - _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null - })}, - ${rankedCategoriesQueryTpl({ - _query: this.query, - _column: this.column, - _aggregation: this.getAggregationSql(), - _aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null - })}, - ${categoriesSummaryMinMaxQueryTpl({ - _query: this.query, - _column: this.column - })}, - ${categoriesSummaryCountQueryTpl({ - _query: this.query, - _column: this.column - })} - `; -}; - -const aggregationFnQueryTpl = ctx => `${ctx._aggregationFn}(${ctx._aggregationColumn})`; - -Aggregation.prototype.getAggregationSql = function() { - return aggregationFnQueryTpl({ - _aggregationFn: this.aggregation, - _aggregationColumn: this.aggregationColumn || 1 - }); -}; - Aggregation.prototype.format = function(result) { var categories = []; var count = 0; From 3682740f08ffd9fd43afd0df92003e08010c021f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 12:01:41 +0200 Subject: [PATCH 10/28] Build aggregation query with string templates avoiding to join all inner templates --- lib/cartodb/models/dataview/aggregation.js | 67 ++++++++-------------- 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 627eb73a..2ba36ede 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -240,50 +240,33 @@ Aggregation.prototype.sql = function(psql, override, callback) { return null; } - var _query = this.query; - - var aggregationSql; - - - - if (!!override.ownFilter) { - aggregationSql = [ - categoriesCTESqlTpl({ - query: _query, - column: this.column, + var aggregationSql = ` + ${categoriesCTESqlTpl({ + query: this.query, + column: this.column, + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn, + isFloatColumn: this._isFloatColumn + })} + ${!!override.ownFilter ? + `${aggregationQueryTpl({ + _isFloatColumn: this._isFloatColumn, + _query: this.query, + _column: this.column, + _aggregation: aggregationFnQueryTpl({ aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn, - isFloatColumn: this._isFloatColumn + aggregationColumn: this.aggregationColumn || 1 }), - aggregationQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: _query, - _column: this.column, - _aggregation: aggregationFnQueryTpl({ - aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn || 1 - }), - _limit: CATEGORIES_LIMIT - }) - ].join('\n'); - } else { - aggregationSql = [ - categoriesCTESqlTpl({ - query: _query, - column: this.column, - aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn, - isFloatColumn: this._isFloatColumn - }), - rankedAggregationQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: _query, - _column: this.column, - _aggregationFn: this.aggregation !== 'count' ? this.aggregation : 'sum', - _limit: CATEGORIES_LIMIT - }) - ].join('\n'); - } + _limit: CATEGORIES_LIMIT + })}` : + `${rankedAggregationQueryTpl({ + _isFloatColumn: this._isFloatColumn, + _query: this.query, + _column: this.column, + _aggregationFn: this.aggregation !== 'count' ? this.aggregation : 'sum', + _limit: CATEGORIES_LIMIT + })}`} + `; debug(aggregationSql); From 65dca454f497abf868e05876d2c9f059b27948f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 12:18:03 +0200 Subject: [PATCH 11/28] Move aggragation query to its own query template --- lib/cartodb/models/dataview/aggregation.js | 65 +++++++++++++--------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 2ba36ede..b22d2345 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -165,6 +165,36 @@ const categoriesCTESqlTpl = ctx => ` const aggregationFnQueryTpl = ctx => `${ctx.aggregation}(${ctx.aggregationColumn})`; +const aggregationSqlTpl = ctx => ` + ${categoriesCTESqlTpl({ + query: ctx.query, + column: ctx.column, + aggregation: ctx.aggregation, + aggregationColumn: ctx.aggregationColumn, + isFloatColumn: ctx.isFloatColumn + })} + ${!!ctx.override.ownFilter ? + `${aggregationQueryTpl({ + _isFloatColumn: ctx.isFloatColumn, + _query: ctx.query, + _column: ctx.column, + _aggregation: aggregationFnQueryTpl({ + aggregation: ctx.aggregation, + aggregationColumn: ctx.aggregationColumn || 1 + }), + _limit: CATEGORIES_LIMIT + }) + }` : + `${rankedAggregationQueryTpl({ + _isFloatColumn: ctx.isFloatColumn, + _query: ctx.query, + _column: ctx.column, + _aggregationFn: ctx.aggregation !== 'count' ? ctx.aggregation : 'sum', + _limit: CATEGORIES_LIMIT + })} + `} +`; + const CATEGORIES_LIMIT = 6; const VALID_OPERATIONS = { @@ -240,33 +270,14 @@ Aggregation.prototype.sql = function(psql, override, callback) { return null; } - var aggregationSql = ` - ${categoriesCTESqlTpl({ - query: this.query, - column: this.column, - aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn, - isFloatColumn: this._isFloatColumn - })} - ${!!override.ownFilter ? - `${aggregationQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: this.query, - _column: this.column, - _aggregation: aggregationFnQueryTpl({ - aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn || 1 - }), - _limit: CATEGORIES_LIMIT - })}` : - `${rankedAggregationQueryTpl({ - _isFloatColumn: this._isFloatColumn, - _query: this.query, - _column: this.column, - _aggregationFn: this.aggregation !== 'count' ? this.aggregation : 'sum', - _limit: CATEGORIES_LIMIT - })}`} - `; + var aggregationSql = aggregationSqlTpl({ + override: override, + query: this.query, + column: this.column, + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn, + isFloatColumn: this._isFloatColumn + }); debug(aggregationSql); From b1508af007833d88459424efb1f1a445bff0d912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 13:14:12 +0200 Subject: [PATCH 12/28] Remove prefix to context's props in order to share it throught all templates avoiding to duplicate passing custom params again and again --- lib/cartodb/models/dataview/aggregation.js | 131 +++++++++------------ 1 file changed, 54 insertions(+), 77 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index b22d2345..be54ec7d 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -4,14 +4,14 @@ const debug = require('debug')('windshaft:widget:aggregation'); const filteredQueryTpl = ctx => ` filtered_source AS ( SELECT * - FROM (${ctx._query}) _cdb_filtered_source - ${ctx._aggregationColumn && ctx._isFloatColumn ? ` + FROM (${ctx.query}) _cdb_filtered_source + ${ctx.aggregationColumn && ctx.isFloatColumn ? ` WHERE - ${ctx._aggregationColumn} != 'infinity'::float + ${ctx.aggregationColumn} != 'infinity'::float AND - ${ctx._aggregationColumn} != '-infinity'::float + ${ctx.aggregationColumn} != '-infinity'::float AND - ${ctx._aggregationColumn} != 'NaN'::float` : + ${ctx.aggregationColumn} != 'NaN'::float` : '' } ) @@ -21,31 +21,31 @@ const summaryQueryTpl = ctx => ` summary AS ( SELECT count(1) AS count, - sum(CASE WHEN ${ctx._column} IS NULL THEN 1 ELSE 0 END) AS nulls_count - ${ctx._isFloatColumn ? `, + sum(CASE WHEN ${ctx.column} IS NULL THEN 1 ELSE 0 END) AS nulls_count + ${ctx.isFloatColumn ? `, sum( CASE - WHEN ${ctx._aggregationColumn} = 'infinity'::float OR ${ctx._aggregationColumn} = '-infinity'::float + WHEN ${ctx.aggregationColumn} = 'infinity'::float OR ${ctx.aggregationColumn} = '-infinity'::float THEN 1 ELSE 0 END ) AS infinities_count, - sum(CASE WHEN ${ctx._aggregationColumn} = 'NaN'::float THEN 1 ELSE 0 END) AS nans_count` : + sum(CASE WHEN ${ctx.aggregationColumn} = 'NaN'::float THEN 1 ELSE 0 END) AS nans_count` : '' } - FROM (${ctx._query}) _cdb_aggregation_nulls + FROM (${ctx.query}) _cdb_aggregation_nulls ) `; const rankedCategoriesQueryTpl = ctx => ` categories AS( SELECT - ${ctx._column} AS category, - ${ctx._aggregation} AS value, - row_number() OVER (ORDER BY ${ctx._aggregation} desc) as rank + ${ctx.column} AS category, + ${ctx.aggregationFn} AS value, + row_number() OVER (ORDER BY ${ctx.aggregationFn} desc) as rank FROM filtered_source - ${ctx._aggregationColumn !== null ? `WHERE ${ctx._aggregationColumn} IS NOT NULL` : ''} - GROUP BY ${ctx._column} + ${ctx.aggregationColumn !== null ? `WHERE ${ctx.aggregationColumn} IS NOT NULL` : ''} + GROUP BY ${ctx.column} ORDER BY 2 DESC ) `; @@ -63,9 +63,9 @@ const categoriesSummaryCountQueryTpl = ctx => ` categories_summary_count AS( SELECT count(1) AS categories_count FROM ( - SELECT ${ctx._column} AS category - FROM (${ctx._query}) _cdb_categories - GROUP BY ${ctx._column} + SELECT ${ctx.column} AS category + FROM (${ctx.query}) _cdb_categories + GROUP BY ${ctx.column} ) _cdb_categories_count ) `; @@ -82,43 +82,43 @@ const rankedAggregationQueryTpl = ctx => ` max_val, count, categories_count - ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + ${ctx.isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } FROM categories, summary, categories_summary_min_max, categories_summary_count - WHERE rank < ${ctx._limit} + WHERE rank < ${ctx.limit} UNION ALL SELECT 'Other' category, - ${ctx._aggregationFn}(value) as value, + ${ctx.aggregation !== 'count' ? ctx.aggregation : 'sum'}(value) as value, true as agg, nulls_count, min_val, max_val, count, categories_count - ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + ${ctx.isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } FROM categories, summary, categories_summary_min_max, categories_summary_count - WHERE rank >= ${ctx._limit} + WHERE rank >= ${ctx.limit} GROUP BY nulls_count, min_val, max_val, count, categories_count - ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + ${ctx.isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } `; const aggregationQueryTpl = ctx => ` SELECT - CAST(${ctx._column} AS text) AS category, - ${ctx._aggregation} AS value, + CAST(${ctx.column} AS text) AS category, + ${ctx.aggregationFn} AS value, false as agg, nulls_count, min_val, max_val, count, categories_count - ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } - FROM (${ctx._query}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count + ${ctx.isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + FROM (${ctx.query}) _cdb_aggregation_all, summary, categories_summary_min_max, categories_summary_count GROUP BY category, nulls_count, @@ -126,73 +126,45 @@ const aggregationQueryTpl = ctx => ` max_val, count, categories_count - ${ctx._isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } + ${ctx.isFloatColumn ? `${specialNumericValuesColumns(ctx)}` : '' } ORDER BY value DESC `; const categoriesCTESqlTpl = ctx => ` WITH ${filteredQueryTpl({ - _isFloatColumn: ctx.isFloatColumn, - _query: ctx.query, - _column: ctx.column, - _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + isFloatColumn: ctx.isFloatColumn, + query: ctx.query, + column: ctx.column, + aggregationColumn: ctx.aggregationColumn })}, ${summaryQueryTpl({ - _isFloatColumn: ctx.isFloatColumn, - _query: ctx.query, - _column: ctx.column, - _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + isFloatColumn: ctx.isFloatColumn, + query: ctx.query, + column: ctx.column, + aggregationColumn: ctx.aggregationColumn })}, ${rankedCategoriesQueryTpl({ - _query: ctx.query, - _column: ctx.column, - _aggregation: aggregationFnQueryTpl({ - aggregation: ctx.aggregation, - aggregationColumn: ctx.aggregationColumn || 1 - }), - _aggregationColumn: ctx.aggregation !== 'count' ? ctx.aggregationColumn : null + query: ctx.query, + column: ctx.column, + aggregationFn: ctx.aggregationFn, + aggregationColumn: ctx.aggregationColumn })}, ${categoriesSummaryMinMaxQueryTpl({ - _query: ctx.query, - _column: ctx.column + query: ctx.query, + column: ctx.column })}, ${categoriesSummaryCountQueryTpl({ - _query: ctx.query, - _column: ctx.column + query: ctx.query, + column: ctx.column })} `; const aggregationFnQueryTpl = ctx => `${ctx.aggregation}(${ctx.aggregationColumn})`; const aggregationSqlTpl = ctx => ` - ${categoriesCTESqlTpl({ - query: ctx.query, - column: ctx.column, - aggregation: ctx.aggregation, - aggregationColumn: ctx.aggregationColumn, - isFloatColumn: ctx.isFloatColumn - })} - ${!!ctx.override.ownFilter ? - `${aggregationQueryTpl({ - _isFloatColumn: ctx.isFloatColumn, - _query: ctx.query, - _column: ctx.column, - _aggregation: aggregationFnQueryTpl({ - aggregation: ctx.aggregation, - aggregationColumn: ctx.aggregationColumn || 1 - }), - _limit: CATEGORIES_LIMIT - }) - }` : - `${rankedAggregationQueryTpl({ - _isFloatColumn: ctx.isFloatColumn, - _query: ctx.query, - _column: ctx.column, - _aggregationFn: ctx.aggregation !== 'count' ? ctx.aggregation : 'sum', - _limit: CATEGORIES_LIMIT - })} - `} + ${categoriesCTESqlTpl(ctx)} + ${!!ctx.override.ownFilter ? `${aggregationQueryTpl(ctx)}` : `${rankedAggregationQueryTpl(ctx)}`} `; const CATEGORIES_LIMIT = 6; @@ -275,8 +247,13 @@ Aggregation.prototype.sql = function(psql, override, callback) { query: this.query, column: this.column, aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn, - isFloatColumn: this._isFloatColumn + aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null, + aggregationFn: aggregationFnQueryTpl({ + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn || 1 + }), + isFloatColumn: this._isFloatColumn, + limit: CATEGORIES_LIMIT }); debug(aggregationSql); From 132fce84c57f247d6ac30ebf92fc9ebb7d169021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:07:30 +0200 Subject: [PATCH 13/28] Remove string template in the middle --- lib/cartodb/models/dataview/aggregation.js | 41 +++++----------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index be54ec7d..6f87aba1 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -130,40 +130,15 @@ const aggregationQueryTpl = ctx => ` ORDER BY value DESC `; -const categoriesCTESqlTpl = ctx => ` - WITH - ${filteredQueryTpl({ - isFloatColumn: ctx.isFloatColumn, - query: ctx.query, - column: ctx.column, - aggregationColumn: ctx.aggregationColumn - })}, - ${summaryQueryTpl({ - isFloatColumn: ctx.isFloatColumn, - query: ctx.query, - column: ctx.column, - aggregationColumn: ctx.aggregationColumn - })}, - ${rankedCategoriesQueryTpl({ - query: ctx.query, - column: ctx.column, - aggregationFn: ctx.aggregationFn, - aggregationColumn: ctx.aggregationColumn - })}, - ${categoriesSummaryMinMaxQueryTpl({ - query: ctx.query, - column: ctx.column - })}, - ${categoriesSummaryCountQueryTpl({ - query: ctx.query, - column: ctx.column - })} -`; - const aggregationFnQueryTpl = ctx => `${ctx.aggregation}(${ctx.aggregationColumn})`; -const aggregationSqlTpl = ctx => ` - ${categoriesCTESqlTpl(ctx)} +const aggregationDataviewQueryTpl = ctx => ` + WITH + ${filteredQueryTpl(ctx)}, + ${summaryQueryTpl(ctx)}, + ${rankedCategoriesQueryTpl(ctx)}, + ${categoriesSummaryMinMaxQueryTpl(ctx)}, + ${categoriesSummaryCountQueryTpl(ctx)} ${!!ctx.override.ownFilter ? `${aggregationQueryTpl(ctx)}` : `${rankedAggregationQueryTpl(ctx)}`} `; @@ -242,7 +217,7 @@ Aggregation.prototype.sql = function(psql, override, callback) { return null; } - var aggregationSql = aggregationSqlTpl({ + var aggregationSql = aggregationDataviewQueryTpl({ override: override, query: this.query, column: this.column, From e2ab48bee2630050a8636776ebe069238c4fe398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:22:58 +0200 Subject: [PATCH 14/28] Remove prefix to context's properties --- lib/cartodb/models/dataview/aggregation.js | 40 ++++++++++++---------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 6f87aba1..575c7202 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -273,20 +273,20 @@ Aggregation.prototype.format = function(result) { const filterCategoriesQueryTpl = ctx => ` SELECT - ${ctx._column} AS category, - ${ctx._value} AS value - FROM (${ctx._query}) _cdb_aggregation_search - WHERE CAST(${ctx._column} as text) ILIKE ${ctx._userQuery} - GROUP BY ${ctx._column} + ${ctx.column} AS category, + ${ctx.value} AS value + FROM (${ctx.query}) _cdb_aggregation_search + WHERE CAST(${ctx.column} as text) ILIKE ${ctx.userQuery} + GROUP BY ${ctx.column} `; const searchQueryTpl = ctx => ` WITH search_unfiltered AS ( - ${ctx._searchUnfiltered} + ${ctx.searchUnfiltered} ), search_filtered AS ( - ${ctx._searchFiltered} + ${ctx.searchFiltered} ), search_union AS ( SELECT * FROM search_unfiltered @@ -302,26 +302,28 @@ const searchQueryTpl = ctx => ` Aggregation.prototype.search = function(psql, userQuery, callback) { var self = this; - var _userQuery = psql.escapeLiteral('%' + userQuery + '%'); - var _value = this.aggregation !== 'count' && this.aggregationColumn ? + var escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); + var value = this.aggregation !== 'count' && this.aggregationColumn ? this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; // TODO unfiltered will be wrong as filters are already applied at this point var query = searchQueryTpl({ - _searchUnfiltered: filterCategoriesQueryTpl({ - _query: this.query, - _column: this.column, - _value: '0', - _userQuery: _userQuery + searchUnfiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: '0', + userQuery: escapedUserQuery }), - _searchFiltered: filterCategoriesQueryTpl({ - _query: this.query, - _column: this.column, - _value: _value, - _userQuery: _userQuery + searchFiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: value, + userQuery: escapedUserQuery }) }); + debug(query); + psql.query(query, function(err, result) { if (err) { return callback(err, result); From 0a894da0df1cf6da599e85623500ab6cda5b099b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:27:57 +0200 Subject: [PATCH 15/28] Remove prefix to context's properties --- lib/cartodb/models/dataview/aggregation.js | 30 ++++++++++++---------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 575c7202..ecf9419c 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -283,10 +283,20 @@ const filterCategoriesQueryTpl = ctx => ` const searchQueryTpl = ctx => ` WITH search_unfiltered AS ( - ${ctx.searchUnfiltered} + ${filterCategoriesQueryTpl({ + query: ctx.query, + column: ctx.column, + value: '0', + userQuery: escapedUserQuery + })} ), search_filtered AS ( - ${ctx.searchFiltered} + ${filterCategoriesQueryTpl({ + query: ctx.query, + column: ctx.column, + value: value, + userQuery: escapedUserQuery + })} ), search_union AS ( SELECT * FROM search_unfiltered @@ -308,18 +318,10 @@ Aggregation.prototype.search = function(psql, userQuery, callback) { // TODO unfiltered will be wrong as filters are already applied at this point var query = searchQueryTpl({ - searchUnfiltered: filterCategoriesQueryTpl({ - query: this.query, - column: this.column, - value: '0', - userQuery: escapedUserQuery - }), - searchFiltered: filterCategoriesQueryTpl({ - query: this.query, - column: this.column, - value: value, - userQuery: escapedUserQuery - }) + query: this.query, + column: this.column, + value: value, + userQuery: escapedUserQuery }); debug(query); From 02949003a96b6eaec2bee1257bd8f26353e49c11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:48:54 +0200 Subject: [PATCH 16/28] Build search query in two steps --- lib/cartodb/models/dataview/aggregation.js | 30 ++++++++++------------ 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index ecf9419c..575c7202 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -283,20 +283,10 @@ const filterCategoriesQueryTpl = ctx => ` const searchQueryTpl = ctx => ` WITH search_unfiltered AS ( - ${filterCategoriesQueryTpl({ - query: ctx.query, - column: ctx.column, - value: '0', - userQuery: escapedUserQuery - })} + ${ctx.searchUnfiltered} ), search_filtered AS ( - ${filterCategoriesQueryTpl({ - query: ctx.query, - column: ctx.column, - value: value, - userQuery: escapedUserQuery - })} + ${ctx.searchFiltered} ), search_union AS ( SELECT * FROM search_unfiltered @@ -318,10 +308,18 @@ Aggregation.prototype.search = function(psql, userQuery, callback) { // TODO unfiltered will be wrong as filters are already applied at this point var query = searchQueryTpl({ - query: this.query, - column: this.column, - value: value, - userQuery: escapedUserQuery + searchUnfiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: '0', + userQuery: escapedUserQuery + }), + searchFiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: value, + userQuery: escapedUserQuery + }) }); debug(query); From 086eff01a907e7dc69039a47e51075914ee961f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:57:18 +0200 Subject: [PATCH 17/28] Use ES6 class syntax --- lib/cartodb/models/dataview/aggregation.js | 285 ++++++++++----------- 1 file changed, 142 insertions(+), 143 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 575c7202..039cbea0 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -163,112 +163,158 @@ const TYPE = 'aggregation'; } } */ -function Aggregation(query, options = {}, queries = {}) { - if (typeof options.column !== 'string') { - throw new Error(`Aggregation expects 'column' in widget options`); +module.exports = class Aggregation extends BaseWidget { + constructor (query, options = {}, queries = {}) { + super(); + + if (typeof options.column !== 'string') { + throw new Error(`Aggregation expects 'column' in widget options`); + } + + if (typeof options.aggregation !== 'string') { + throw new Error(`Aggregation expects 'aggregation' operation in widget options`); + } + + if (!VALID_OPERATIONS[options.aggregation]) { + throw new Error(`Aggregation does not support '${options.aggregation}' operation`); + } + + var requiredOptions = VALID_OPERATIONS[options.aggregation]; + var missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); + + if (missingOptions.length > 0) { + throw new Error( + `Aggregation '${options.aggregation}' is missing some options: ${missingOptions.join(',')}` + ); + } + + this.query = query; + this.queries = queries; + this.column = options.column; + this.aggregation = options.aggregation; + this.aggregationColumn = options.aggregationColumn; + this._isFloatColumn = null; } - if (typeof options.aggregation !== 'string') { - throw new Error(`Aggregation expects 'aggregation' operation in widget options`); - } + sql (psql, override, callback) { + var self = this; - if (!VALID_OPERATIONS[options.aggregation]) { - throw new Error(`Aggregation does not support '${options.aggregation}' operation`); - } + if (!callback) { + callback = override; + override = {}; + } - var requiredOptions = VALID_OPERATIONS[options.aggregation]; - var missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); + if (this.aggregationColumn && this._isFloatColumn === null) { + this._isFloatColumn = false; + this.getColumnType(psql, this.aggregationColumn, this.queries.no_filters, function (err, type) { + if (!err && !!type) { + self._isFloatColumn = type.float; + } + self.sql(psql, override, callback); + }); + return null; + } - if (missingOptions.length > 0) { - throw new Error(`Aggregation '${options.aggregation}' is missing some options: ${missingOptions.join(',')}`); - } - - BaseWidget.apply(this); - - this.query = query; - this.queries = queries; - this.column = options.column; - this.aggregation = options.aggregation; - this.aggregationColumn = options.aggregationColumn; - this._isFloatColumn = null; -} - -Aggregation.prototype = new BaseWidget(); -Aggregation.prototype.constructor = Aggregation; - -module.exports = Aggregation; - -Aggregation.prototype.sql = function(psql, override, callback) { - var self = this; - - if (!callback) { - callback = override; - override = {}; - } - - if (this.aggregationColumn && this._isFloatColumn === null) { - this._isFloatColumn = false; - this.getColumnType(psql, this.aggregationColumn, this.queries.no_filters, function (err, type) { - if (!err && !!type) { - self._isFloatColumn = type.float; - } - self.sql(psql, override, callback); - }); - return null; - } - - var aggregationSql = aggregationDataviewQueryTpl({ - override: override, - query: this.query, - column: this.column, - aggregation: this.aggregation, - aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null, - aggregationFn: aggregationFnQueryTpl({ + var aggregationSql = aggregationDataviewQueryTpl({ + override: override, + query: this.query, + column: this.column, aggregation: this.aggregation, - aggregationColumn: this.aggregationColumn || 1 - }), - isFloatColumn: this._isFloatColumn, - limit: CATEGORIES_LIMIT - }); + aggregationColumn: this.aggregation !== 'count' ? this.aggregationColumn : null, + aggregationFn: aggregationFnQueryTpl({ + aggregation: this.aggregation, + aggregationColumn: this.aggregationColumn || 1 + }), + isFloatColumn: this._isFloatColumn, + limit: CATEGORIES_LIMIT + }); - debug(aggregationSql); + debug(aggregationSql); - return callback(null, aggregationSql); -}; - -Aggregation.prototype.format = function(result) { - var categories = []; - var count = 0; - var nulls = 0; - var nans = 0; - var infinities = 0; - var minValue = 0; - var maxValue = 0; - var categoriesCount = 0; - - if (result.rows.length) { - var firstRow = result.rows[0]; - count = firstRow.count; - nulls = firstRow.nulls_count; - nans = firstRow.nans_count; - infinities = firstRow.infinities_count; - minValue = firstRow.min_val; - maxValue = firstRow.max_val; - categoriesCount = firstRow.categories_count; - categories = result.rows.map(({ category, value, agg }) => ({ category, value, agg })); + return callback(null, aggregationSql); } - return { - aggregation: this.aggregation, - count: count, - nulls: nulls, - nans: nans, - infinities: infinities, - min: minValue, - max: maxValue, - categoriesCount: categoriesCount, - categories: categories - }; + format (result) { + var categories = []; + var count = 0; + var nulls = 0; + var nans = 0; + var infinities = 0; + var minValue = 0; + var maxValue = 0; + var categoriesCount = 0; + + if (result.rows.length) { + var firstRow = result.rows[0]; + count = firstRow.count; + nulls = firstRow.nulls_count; + nans = firstRow.nans_count; + infinities = firstRow.infinities_count; + minValue = firstRow.min_val; + maxValue = firstRow.max_val; + categoriesCount = firstRow.categories_count; + categories = result.rows.map(({ category, value, agg }) => ({ category, value, agg })); + } + + return { + aggregation: this.aggregation, + count: count, + nulls: nulls, + nans: nans, + infinities: infinities, + min: minValue, + max: maxValue, + categoriesCount: categoriesCount, + categories: categories + }; + } + + search (psql, userQuery, callback) { + var self = this; + + var escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); + var value = this.aggregation !== 'count' && this.aggregationColumn ? + this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; + + // TODO unfiltered will be wrong as filters are already applied at this point + var query = searchQueryTpl({ + searchUnfiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: '0', + userQuery: escapedUserQuery + }), + searchFiltered: filterCategoriesQueryTpl({ + query: this.query, + column: this.column, + value: value, + userQuery: escapedUserQuery + }) + }); + + debug(query); + + psql.query(query, function(err, result) { + if (err) { + return callback(err, result); + } + + return callback(null, {type: self.getType(), categories: result.rows }); + }, true); // use read-only transaction + } + + getType () { + return TYPE; + } + + toString () { + return JSON.stringify({ + _type: TYPE, + _query: this.query, + _column: this.column, + _aggregation: this.aggregation + }); + } }; const filterCategoriesQueryTpl = ctx => ` @@ -298,50 +344,3 @@ const searchQueryTpl = ctx => ` GROUP BY category ORDER BY value desc `; - -Aggregation.prototype.search = function(psql, userQuery, callback) { - var self = this; - - var escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); - var value = this.aggregation !== 'count' && this.aggregationColumn ? - this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; - - // TODO unfiltered will be wrong as filters are already applied at this point - var query = searchQueryTpl({ - searchUnfiltered: filterCategoriesQueryTpl({ - query: this.query, - column: this.column, - value: '0', - userQuery: escapedUserQuery - }), - searchFiltered: filterCategoriesQueryTpl({ - query: this.query, - column: this.column, - value: value, - userQuery: escapedUserQuery - }) - }); - - debug(query); - - psql.query(query, function(err, result) { - if (err) { - return callback(err, result); - } - - return callback(null, {type: self.getType(), categories: result.rows }); - }, true); // use read-only transaction -}; - -Aggregation.prototype.getType = function() { - return TYPE; -}; - -Aggregation.prototype.toString = function() { - return JSON.stringify({ - _type: TYPE, - _query: this.query, - _column: this.column, - _aggregation: this.aggregation - }); -}; From 46254eaf743f7851878a0a7baef52a342379b342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 15:59:15 +0200 Subject: [PATCH 18/28] rename BaseWidget by BaseDataview --- lib/cartodb/models/dataview/aggregation.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 039cbea0..f0ea5608 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -1,4 +1,4 @@ -const BaseWidget = require('./base'); +const BaseDataview = require('./base'); const debug = require('debug')('windshaft:widget:aggregation'); const filteredQueryTpl = ctx => ` @@ -163,7 +163,7 @@ const TYPE = 'aggregation'; } } */ -module.exports = class Aggregation extends BaseWidget { +module.exports = class Aggregation extends BaseDataview { constructor (query, options = {}, queries = {}) { super(); @@ -176,7 +176,7 @@ module.exports = class Aggregation extends BaseWidget { } if (!VALID_OPERATIONS[options.aggregation]) { - throw new Error(`Aggregation does not support '${options.aggregation}' operation`); + throw new Error(`Aggregation does not support '${options.aggregation}' operation`); } var requiredOptions = VALID_OPERATIONS[options.aggregation]; From cd13107a4dbefada5ac6e0fdbf27346261c18cae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:08:12 +0200 Subject: [PATCH 19/28] Use ES6 let & const to declare variables --- lib/cartodb/models/dataview/aggregation.js | 34 +++++++++++----------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index f0ea5608..b1e56f28 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -179,8 +179,8 @@ module.exports = class Aggregation extends BaseDataview { throw new Error(`Aggregation does not support '${options.aggregation}' operation`); } - var requiredOptions = VALID_OPERATIONS[options.aggregation]; - var missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); + const requiredOptions = VALID_OPERATIONS[options.aggregation]; + const missingOptions = requiredOptions.filter(requiredOption => !options.hasOwnProperty(requiredOption)); if (missingOptions.length > 0) { throw new Error( @@ -197,7 +197,7 @@ module.exports = class Aggregation extends BaseDataview { } sql (psql, override, callback) { - var self = this; + const self = this; if (!callback) { callback = override; @@ -215,7 +215,7 @@ module.exports = class Aggregation extends BaseDataview { return null; } - var aggregationSql = aggregationDataviewQueryTpl({ + const aggregationSql = aggregationDataviewQueryTpl({ override: override, query: this.query, column: this.column, @@ -235,17 +235,17 @@ module.exports = class Aggregation extends BaseDataview { } format (result) { - var categories = []; - var count = 0; - var nulls = 0; - var nans = 0; - var infinities = 0; - var minValue = 0; - var maxValue = 0; - var categoriesCount = 0; + let categories = []; + let count = 0; + let nulls = 0; + let nans = 0; + let infinities = 0; + let minValue = 0; + let maxValue = 0; + let categoriesCount = 0; if (result.rows.length) { - var firstRow = result.rows[0]; + const firstRow = result.rows[0]; count = firstRow.count; nulls = firstRow.nulls_count; nans = firstRow.nans_count; @@ -270,14 +270,14 @@ module.exports = class Aggregation extends BaseDataview { } search (psql, userQuery, callback) { - var self = this; + const self = this; - var escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); - var value = this.aggregation !== 'count' && this.aggregationColumn ? + const escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); + const value = this.aggregation !== 'count' && this.aggregationColumn ? this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; // TODO unfiltered will be wrong as filters are already applied at this point - var query = searchQueryTpl({ + const query = searchQueryTpl({ searchUnfiltered: filterCategoriesQueryTpl({ query: this.query, column: this.column, From b05083bcfc09ebf6b1326bcb394905a8c46ef196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:09:44 +0200 Subject: [PATCH 20/28] Move search's templates along aggregation's templates --- lib/cartodb/models/dataview/aggregation.js | 56 +++++++++++----------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index b1e56f28..d51edf04 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -142,6 +142,34 @@ const aggregationDataviewQueryTpl = ctx => ` ${!!ctx.override.ownFilter ? `${aggregationQueryTpl(ctx)}` : `${rankedAggregationQueryTpl(ctx)}`} `; +const filterCategoriesQueryTpl = ctx => ` + SELECT + ${ctx.column} AS category, + ${ctx.value} AS value + FROM (${ctx.query}) _cdb_aggregation_search + WHERE CAST(${ctx.column} as text) ILIKE ${ctx.userQuery} + GROUP BY ${ctx.column} +`; + +const searchQueryTpl = ctx => ` + WITH + search_unfiltered AS ( + ${ctx.searchUnfiltered} + ), + search_filtered AS ( + ${ctx.searchFiltered} + ), + search_union AS ( + SELECT * FROM search_unfiltered + UNION ALL + SELECT * FROM search_filtered + ) + SELECT category, sum(value) AS value + FROM search_union + GROUP BY category + ORDER BY value desc +`; + const CATEGORIES_LIMIT = 6; const VALID_OPERATIONS = { @@ -316,31 +344,3 @@ module.exports = class Aggregation extends BaseDataview { }); } }; - -const filterCategoriesQueryTpl = ctx => ` - SELECT - ${ctx.column} AS category, - ${ctx.value} AS value - FROM (${ctx.query}) _cdb_aggregation_search - WHERE CAST(${ctx.column} as text) ILIKE ${ctx.userQuery} - GROUP BY ${ctx.column} -`; - -const searchQueryTpl = ctx => ` - WITH - search_unfiltered AS ( - ${ctx.searchUnfiltered} - ), - search_filtered AS ( - ${ctx.searchFiltered} - ), - search_union AS ( - SELECT * FROM search_unfiltered - UNION ALL - SELECT * FROM search_filtered - ) - SELECT category, sum(value) AS value - FROM search_union - GROUP BY category - ORDER BY value desc -`; From 746292610aeec58b901e569a9d2110c2c0fa6ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:10:23 +0200 Subject: [PATCH 21/28] Rename debug namespace --- lib/cartodb/models/dataview/aggregation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index d51edf04..b6f54263 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -1,5 +1,5 @@ const BaseDataview = require('./base'); -const debug = require('debug')('windshaft:widget:aggregation'); +const debug = require('debug')('windshaft:dataview:aggregation'); const filteredQueryTpl = ctx => ` filtered_source AS ( From cc2cf78264bcc6459ec8610ff12621ec90823ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:12:39 +0200 Subject: [PATCH 22/28] Rename all 'widget' ocurrences by 'dataview' --- lib/cartodb/models/dataview/aggregation.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index b6f54263..80f77062 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -196,11 +196,11 @@ module.exports = class Aggregation extends BaseDataview { super(); if (typeof options.column !== 'string') { - throw new Error(`Aggregation expects 'column' in widget options`); + throw new Error(`Aggregation expects 'column' in dataview options`); } if (typeof options.aggregation !== 'string') { - throw new Error(`Aggregation expects 'aggregation' operation in widget options`); + throw new Error(`Aggregation expects 'aggregation' operation in dataview options`); } if (!VALID_OPERATIONS[options.aggregation]) { From c2e99219effef22af3bdc2c3b0718e6f9c8045cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:30:46 +0200 Subject: [PATCH 23/28] Use ES6 goodies to refactor format method --- lib/cartodb/models/dataview/aggregation.js | 43 ++++++++-------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 80f77062..e60697cb 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -263,37 +263,26 @@ module.exports = class Aggregation extends BaseDataview { } format (result) { - let categories = []; - let count = 0; - let nulls = 0; - let nans = 0; - let infinities = 0; - let minValue = 0; - let maxValue = 0; - let categoriesCount = 0; - - if (result.rows.length) { - const firstRow = result.rows[0]; - count = firstRow.count; - nulls = firstRow.nulls_count; - nans = firstRow.nans_count; - infinities = firstRow.infinities_count; - minValue = firstRow.min_val; - maxValue = firstRow.max_val; - categoriesCount = firstRow.categories_count; - categories = result.rows.map(({ category, value, agg }) => ({ category, value, agg })); - } + const { + count = 0, + nulls_count = 0, + nans_count = 0, + infinities_count = 0, + min_val = 0, + max_val = 0, + categories_count = 0 + } = result.rows[0] || {}; return { aggregation: this.aggregation, count: count, - nulls: nulls, - nans: nans, - infinities: infinities, - min: minValue, - max: maxValue, - categoriesCount: categoriesCount, - categories: categories + nulls: nulls_count, + nans: nans_count, + infinities: infinities_count, + min: min_val, + max: max_val, + categoriesCount: categories_count, + categories: result.rows.map(({ category, value, agg }) => ({ category, value, agg })) }; } From 46860541fe035c5627b8576741bcf3bf721d1977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:42:25 +0200 Subject: [PATCH 24/28] Apply extract method to validate input options --- lib/cartodb/models/dataview/aggregation.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index e60697cb..c7626736 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -195,6 +195,17 @@ module.exports = class Aggregation extends BaseDataview { constructor (query, options = {}, queries = {}) { super(); + this._checkOptions(options); + + this.query = query; + this.queries = queries; + this.column = options.column; + this.aggregation = options.aggregation; + this.aggregationColumn = options.aggregationColumn; + this._isFloatColumn = null; + } + + _checkOptions () { if (typeof options.column !== 'string') { throw new Error(`Aggregation expects 'column' in dataview options`); } @@ -215,13 +226,6 @@ module.exports = class Aggregation extends BaseDataview { `Aggregation '${options.aggregation}' is missing some options: ${missingOptions.join(',')}` ); } - - this.query = query; - this.queries = queries; - this.column = options.column; - this.aggregation = options.aggregation; - this.aggregationColumn = options.aggregationColumn; - this._isFloatColumn = null; } sql (psql, override, callback) { From 38e2c040d175a4171ed469fb9f2fc210d93da5bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:45:45 +0200 Subject: [PATCH 25/28] Use template string to escape literals --- lib/cartodb/models/dataview/aggregation.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index c7626736..29eebe06 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -292,8 +292,7 @@ module.exports = class Aggregation extends BaseDataview { search (psql, userQuery, callback) { const self = this; - - const escapedUserQuery = psql.escapeLiteral('%' + userQuery + '%'); + const escapedUserQuery = psql.escapeLiteral(`%${userQuery}%`); const value = this.aggregation !== 'count' && this.aggregationColumn ? this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; From 1f3b0beddfa5120d56a7337a0fd5c3de23a1f3f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:47:05 +0200 Subject: [PATCH 26/28] Fix missing parameter --- lib/cartodb/models/dataview/aggregation.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 29eebe06..f621e536 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -205,7 +205,7 @@ module.exports = class Aggregation extends BaseDataview { this._isFloatColumn = null; } - _checkOptions () { + _checkOptions (options) { if (typeof options.column !== 'string') { throw new Error(`Aggregation expects 'column' in dataview options`); } @@ -294,7 +294,8 @@ module.exports = class Aggregation extends BaseDataview { const self = this; const escapedUserQuery = psql.escapeLiteral(`%${userQuery}%`); const value = this.aggregation !== 'count' && this.aggregationColumn ? - this.aggregation + '(' + this.aggregationColumn + ')' : 'count(1)'; + `${this.aggregation}(${this.aggregationColumn})` : + 'count(1)'; // TODO unfiltered will be wrong as filters are already applied at this point const query = searchQueryTpl({ From 51907b95459b653af790f7c137d5edc9bf867906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Sep 2017 16:56:55 +0200 Subject: [PATCH 27/28] Apply extract method to condition --- lib/cartodb/models/dataview/aggregation.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index f621e536..7f6c198a 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -236,7 +236,7 @@ module.exports = class Aggregation extends BaseDataview { override = {}; } - if (this.aggregationColumn && this._isFloatColumn === null) { + if (this._shouldCheckColumnType()) { this._isFloatColumn = false; this.getColumnType(psql, this.aggregationColumn, this.queries.no_filters, function (err, type) { if (!err && !!type) { @@ -266,6 +266,10 @@ module.exports = class Aggregation extends BaseDataview { return callback(null, aggregationSql); } + _shouldCheckColumnType () { + return this.aggregationColumn && this._isFloatColumn === null; + } + format (result) { const { count = 0, From f63fab40ed5ae6715f3f5b85ebc24b2bcd8b1ce5 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 18 Sep 2017 11:34:18 +0200 Subject: [PATCH 28/28] Removing 'self' vars using arrow functions --- lib/cartodb/models/dataview/aggregation.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/models/dataview/aggregation.js b/lib/cartodb/models/dataview/aggregation.js index 7f6c198a..b4c59af2 100644 --- a/lib/cartodb/models/dataview/aggregation.js +++ b/lib/cartodb/models/dataview/aggregation.js @@ -229,8 +229,6 @@ module.exports = class Aggregation extends BaseDataview { } sql (psql, override, callback) { - const self = this; - if (!callback) { callback = override; override = {}; @@ -238,11 +236,11 @@ module.exports = class Aggregation extends BaseDataview { if (this._shouldCheckColumnType()) { this._isFloatColumn = false; - this.getColumnType(psql, this.aggregationColumn, this.queries.no_filters, function (err, type) { + this.getColumnType(psql, this.aggregationColumn, this.queries.no_filters, (err, type) => { if (!err && !!type) { - self._isFloatColumn = type.float; + this._isFloatColumn = type.float; } - self.sql(psql, override, callback); + this.sql(psql, override, callback); }); return null; } @@ -295,7 +293,6 @@ module.exports = class Aggregation extends BaseDataview { } search (psql, userQuery, callback) { - const self = this; const escapedUserQuery = psql.escapeLiteral(`%${userQuery}%`); const value = this.aggregation !== 'count' && this.aggregationColumn ? `${this.aggregation}(${this.aggregationColumn})` : @@ -319,12 +316,12 @@ module.exports = class Aggregation extends BaseDataview { debug(query); - psql.query(query, function(err, result) { + psql.query(query, (err, result) => { if (err) { return callback(err, result); } - return callback(null, {type: self.getType(), categories: result.rows }); + return callback(null, {type: this.getType(), categories: result.rows }); }, true); // use read-only transaction }