From 1767b83d097b5c9201937694339b9ab6cf4b0a77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 30 Nov 2017 15:34:20 +0100 Subject: [PATCH 01/78] Aggregation query models: bootstrap hierarchy classes --- .../models/aggregation/base-aggregation.js | 9 ++++ .../models/aggregation/raster-aggregation.js | 54 +++++++++++++++++++ .../models/aggregation/vector-aggregation.js | 54 +++++++++++++++++++ 3 files changed, 117 insertions(+) create mode 100644 lib/cartodb/models/aggregation/base-aggregation.js create mode 100644 lib/cartodb/models/aggregation/raster-aggregation.js create mode 100644 lib/cartodb/models/aggregation/vector-aggregation.js diff --git a/lib/cartodb/models/aggregation/base-aggregation.js b/lib/cartodb/models/aggregation/base-aggregation.js new file mode 100644 index 00000000..d7f92f70 --- /dev/null +++ b/lib/cartodb/models/aggregation/base-aggregation.js @@ -0,0 +1,9 @@ +module.exports = class BaseAggregation { + sql () { + throw new Error('Unimplemented method'); + } +}; + +module.exports.baseQueryTemplate = ctx => ` + select ${ctx} blah.., blah, blah... +`; diff --git a/lib/cartodb/models/aggregation/raster-aggregation.js b/lib/cartodb/models/aggregation/raster-aggregation.js new file mode 100644 index 00000000..c0d65a37 --- /dev/null +++ b/lib/cartodb/models/aggregation/raster-aggregation.js @@ -0,0 +1,54 @@ +const BaseAggregation = require('./base-aggregation'); +const { baseQueryTemplate } = BaseAggregation; + +module.exports = class RasterAggregation extends BaseAggregation { + sql (options) { + return rasterAggregationQueryTemplate({ + source_query: options.sql, + res: options.resolution, + columns: options.columns + }); + } +}; + +const rasterAggregationQueryTemplate = ctx => ` + WITH + _cdb_source AS ( + -- original query + ${ctx.source_query} + ), + _cdb_resolution AS ( + SELECT ${ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) + AS _cdb_grid_size + -- equivalent to: + -- ${ctx.res}*!scale_denominator!*0.00028 + ), + _cdb_gridded AS ( + SELECT + Floor(ST_X(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gx, + Floor(ST_Y(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gy, + count(*) AS _cdb_feature_count + FROM _cdb_source, _cdb_resolution + GROUP BY _cdb_gx, _cdb_gy + ), + _cdb_webmercator AS ( + SELECT + row_number() over() AS cartodb_id, + _cdb_feature_count, + ST_SetSRID( + ST_MakePoint( + _cdb_gx*_cdb_grid_size + _cdb_grid_size/2, + _cdb_gy*_cdb_grid_size + _cdb_grid_size/2 + ), + 3857 + ) AS the_geom_webmercator + FROM _cdb_gridded, _cdb_resolution + ) + SELECT + cartodb_id, + ST_Transform(the_geom_webmercator, 4326) AS the_geom, + the_geom_webmercator, + _cdb_feature_count + FROM _cdb_webmercator + ${baseQueryTemplate(ctx)} +`; diff --git a/lib/cartodb/models/aggregation/vector-aggregation.js b/lib/cartodb/models/aggregation/vector-aggregation.js new file mode 100644 index 00000000..42f070ac --- /dev/null +++ b/lib/cartodb/models/aggregation/vector-aggregation.js @@ -0,0 +1,54 @@ +const BaseAggregation = require('./base-aggregation'); +const { baseQueryTemplate } = BaseAggregation; + +module.exports = class VectorAggregation extends BaseAggregation { + sql (options) { + return vectorAggregationQueryTemplate({ + source_query: options.sql, + res: options.resolution, + columns: options.columns + }); + } +}; + +const vectorAggregationQueryTemplate = ctx => ` + WITH + _cdb_source AS ( + -- original query + ${ctx.source_query} + ), + _cdb_resolution AS ( + SELECT ${ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) + AS _cdb_grid_size + -- equivalent to: + -- ${ctx.res}*!scale_denominator!*0.00028 + ), + _cdb_gridded AS ( + SELECT + Floor(ST_X(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gx, + Floor(ST_Y(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gy, + count(*) AS _cdb_feature_count + FROM _cdb_source, _cdb_resolution + GROUP BY _cdb_gx, _cdb_gy + ), + _cdb_webmercator AS ( + SELECT + row_number() over() AS cartodb_id, + _cdb_feature_count, + ST_SetSRID( + ST_MakePoint( + _cdb_gx*_cdb_grid_size + _cdb_grid_size/2, + _cdb_gy*_cdb_grid_size + _cdb_grid_size/2 + ), + 3857 + ) AS the_geom_webmercator + FROM _cdb_gridded, _cdb_resolution + ) + SELECT + cartodb_id, + ST_Transform(the_geom_webmercator, 4326) AS the_geom, + the_geom_webmercator, + _cdb_feature_count + FROM _cdb_webmercator + ${baseQueryTemplate(ctx)} +`; From 73ae73660367bf96b2e366841b4b1c39576878a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 30 Nov 2017 19:02:30 +0100 Subject: [PATCH 02/78] Add aggregation proxy --- .../models/aggregation/aggregation-proxy.js | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 lib/cartodb/models/aggregation/aggregation-proxy.js diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js new file mode 100644 index 00000000..0985ec8d --- /dev/null +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -0,0 +1,43 @@ +const RasterAggregation = require('./raster-aggregation'); +const VectorAggregation = require('./vector-aggregation'); +const RASTER_AGGREGATION = 'RasterAggregation'; +const VECTOR_AGGREGATION = 'VectorAggregation'; + +module.exports = class AggregationProxy { + constructor (mapconfig, resolution = 256, threshold = 10e5, placement = 'centroid') { + this.mapconfig = mapconfig; + this.resolution = resolution; + this.threshold = threshold; + this.placement = placement; + this.implementation = this._getAggregationImplementation(); + } + + _getAggregationImplementation () { + let implementation = null; + + switch (this._getAggregationType()) { + case VECTOR_AGGREGATION: + implementation = new VectorAggregation(this.resolution, this.threshold, this.placement); + break; + case RASTER_AGGREGATION: + implementation = new RasterAggregation(this.resolution, this.threshold, this.placement); + break; + default: + throw new Error('Unsupported aggregation type'); + } + + return implementation; + } + + _getAggregationType () { + if (this.mapconfig.isVetorLayergroup()) { + return VECTOR_AGGREGATION; + } + + return RASTER_AGGREGATION; + } + + sql () { + return this.implementation.sql(); + } +}; From d937ed31d5e106b0216a6007a095a92356474b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 30 Nov 2017 19:10:57 +0100 Subject: [PATCH 03/78] Add params to instantiate aggregation --- lib/cartodb/models/aggregation/aggregation-proxy.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js index 0985ec8d..7fff63f9 100644 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -4,11 +4,12 @@ const RASTER_AGGREGATION = 'RasterAggregation'; const VECTOR_AGGREGATION = 'VectorAggregation'; module.exports = class AggregationProxy { - constructor (mapconfig, resolution = 256, threshold = 10e5, placement = 'centroid') { + constructor (mapconfig, { resolution = 256, threshold = 10e5, placement = 'centroid', columns = {}} = {}) { this.mapconfig = mapconfig; this.resolution = resolution; this.threshold = threshold; this.placement = placement; + this.columns = columns; this.implementation = this._getAggregationImplementation(); } @@ -17,10 +18,10 @@ module.exports = class AggregationProxy { switch (this._getAggregationType()) { case VECTOR_AGGREGATION: - implementation = new VectorAggregation(this.resolution, this.threshold, this.placement); + implementation = new VectorAggregation(this.resolution, this.threshold, this.placement, this.columns); break; case RASTER_AGGREGATION: - implementation = new RasterAggregation(this.resolution, this.threshold, this.placement); + implementation = new RasterAggregation(this.resolution, this.threshold, this.placement, this.columns); break; default: throw new Error('Unsupported aggregation type'); From deb29f2c7774e0427ce2c8383971d360584550a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 30 Nov 2017 19:20:59 +0100 Subject: [PATCH 04/78] Implement aggregation mapconfig adapter (happy case) --- .../adapter/aggregation-mapconfig-adapter.js | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js new file mode 100644 index 00000000..1d5646ee --- /dev/null +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -0,0 +1,63 @@ +const AggregationProxy = require('../../aggregation/aggregation-proxy'); + +module.exports = class AggregationMapConfigAdapter { + getMapConfig (user, requestMapConfig, params, context, callback) { + if (!this._shouldAdaptLayers(requestMapConfig, params)) { + return callback(null, requestMapConfig); + } + + requestMapConfig.layers.forEach(layer => { + if (!this._hasLayerAggregation(layer)) { + return; + } + + const aggregation = new AggregationProxy(requestMapConfig, layer.options.aggregation); + + let aggregationSql = aggregation.sql(); + + const sqlQueryWrap = layer.options.sql_wrap; + + if (sqlQueryWrap) { + layer.options.sql_raw = aggregationSql; + aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); + } + + layer.options.sql = aggregationSql; + }); + + callback(null, requestMapConfig); + } + + _shouldAdaptLayers (requestMapConfig, params) { + if (typeof params.aggregation === 'boolean') { + return params.aggregation; + } + + if (params.aggregation === undefined) { + if (requestMapConfig.isVectorLayergroup()) { + return true; + } else if (this._hasAggregation(requestMapConfig)){ + return true; + } + } + + return false; + } + + _hasAggregation (requestMapConfig) { + for (const layer of requestMapConfig.layers) { + + + if (this._hasLayerAggregation(layer)) { + return true; + } + } + + return false; + } + + _hasLayerAggregation (layer) { + const { aggregation } = layer.options; + return aggregation !== undefined && (typeof aggregation === 'object' && typeof aggregation === 'boolean'); + } +}; From d01857923ed38e788f5ce81404e153a8e1c49dd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 30 Nov 2017 19:31:00 +0100 Subject: [PATCH 05/78] Plug aggregation mapconfig adapter --- lib/cartodb/server.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 08ec1a66..4ce7974d 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -41,6 +41,7 @@ var AnalysisMapConfigAdapter = require('./models/mapconfig/adapter/analysis-mapc var MapConfigOverviewsAdapter = require('./models/mapconfig/adapter/mapconfig-overviews-adapter'); var TurboCartoAdapter = require('./models/mapconfig/adapter/turbo-carto-adapter'); var DataviewsWidgetsAdapter = require('./models/mapconfig/adapter/dataviews-widgets-adapter'); +var AggregationMapConfigAdapter = require('./models/mapconfig/adapter/aggregation-mapconfig-adapter'); var MapConfigAdapter = require('./models/mapconfig/adapter'); var StatsBackend = require('./backends/stats'); @@ -191,6 +192,7 @@ module.exports = function(serverOptions) { new DataviewsWidgetsAdapter(), new AnalysisMapConfigAdapter(analysisBackend), new MapConfigOverviewsAdapter(overviewsMetadataApi, filterStatsApi), + new AggregationMapConfigAdapter(), new TurboCartoAdapter() ); From 0887e5d5f7d866662e1183aec3264e5637ea28a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Dec 2017 15:43:15 +0100 Subject: [PATCH 06/78] Extract method --- .../adapter/aggregation-mapconfig-adapter.js | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 1d5646ee..07089f41 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -6,24 +6,7 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, requestMapConfig); } - requestMapConfig.layers.forEach(layer => { - if (!this._hasLayerAggregation(layer)) { - return; - } - - const aggregation = new AggregationProxy(requestMapConfig, layer.options.aggregation); - - let aggregationSql = aggregation.sql(); - - const sqlQueryWrap = layer.options.sql_wrap; - - if (sqlQueryWrap) { - layer.options.sql_raw = aggregationSql; - aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); - } - - layer.options.sql = aggregationSql; - }); + requestMapConfig = this._adaptLayers(requestMapConfig); callback(null, requestMapConfig); } @@ -60,4 +43,25 @@ module.exports = class AggregationMapConfigAdapter { const { aggregation } = layer.options; return aggregation !== undefined && (typeof aggregation === 'object' && typeof aggregation === 'boolean'); } + + _adaptLayers (requestMapConfig) { + return requestMapConfig.layers.map(layer => { + if (this._hasLayerAggregation(layer)) { + const aggregation = new AggregationProxy(requestMapConfig, layer.options.aggregation); + + let aggregationSql = aggregation.sql(); + + const sqlQueryWrap = layer.options.sql_wrap; + + if (sqlQueryWrap) { + layer.options.sql_raw = aggregationSql; + aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); + } + + layer.options.sql = aggregationSql; + } + + return layer; + }); + } }; From f376a7cdd5ad8a0d221ba87c7b6c3c79d5729bfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Dec 2017 17:05:01 +0100 Subject: [PATCH 07/78] Use aggregation adapter before the overviews one --- lib/cartodb/server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 4ce7974d..b7f0e8a0 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -191,8 +191,8 @@ module.exports = function(serverOptions) { new SqlWrapMapConfigAdapter(), new DataviewsWidgetsAdapter(), new AnalysisMapConfigAdapter(analysisBackend), - new MapConfigOverviewsAdapter(overviewsMetadataApi, filterStatsApi), new AggregationMapConfigAdapter(), + new MapConfigOverviewsAdapter(overviewsMetadataApi, filterStatsApi), new TurboCartoAdapter() ); From 6f04214f5d3dc686aa81f7c6d3bc9b5c80eb4e8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Dec 2017 17:06:03 +0100 Subject: [PATCH 08/78] Simplify to pass test --- .../models/aggregation/base-aggregation.js | 4 -- .../models/aggregation/raster-aggregation.js | 45 +------------------ .../models/aggregation/vector-aggregation.js | 45 +------------------ 3 files changed, 4 insertions(+), 90 deletions(-) diff --git a/lib/cartodb/models/aggregation/base-aggregation.js b/lib/cartodb/models/aggregation/base-aggregation.js index d7f92f70..98c5a09c 100644 --- a/lib/cartodb/models/aggregation/base-aggregation.js +++ b/lib/cartodb/models/aggregation/base-aggregation.js @@ -3,7 +3,3 @@ module.exports = class BaseAggregation { throw new Error('Unimplemented method'); } }; - -module.exports.baseQueryTemplate = ctx => ` - select ${ctx} blah.., blah, blah... -`; diff --git a/lib/cartodb/models/aggregation/raster-aggregation.js b/lib/cartodb/models/aggregation/raster-aggregation.js index c0d65a37..51c30773 100644 --- a/lib/cartodb/models/aggregation/raster-aggregation.js +++ b/lib/cartodb/models/aggregation/raster-aggregation.js @@ -1,54 +1,13 @@ const BaseAggregation = require('./base-aggregation'); -const { baseQueryTemplate } = BaseAggregation; module.exports = class RasterAggregation extends BaseAggregation { sql (options) { return rasterAggregationQueryTemplate({ - source_query: options.sql, + sourceQuery: options.sql, res: options.resolution, columns: options.columns }); } }; -const rasterAggregationQueryTemplate = ctx => ` - WITH - _cdb_source AS ( - -- original query - ${ctx.source_query} - ), - _cdb_resolution AS ( - SELECT ${ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) - AS _cdb_grid_size - -- equivalent to: - -- ${ctx.res}*!scale_denominator!*0.00028 - ), - _cdb_gridded AS ( - SELECT - Floor(ST_X(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gx, - Floor(ST_Y(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gy, - count(*) AS _cdb_feature_count - FROM _cdb_source, _cdb_resolution - GROUP BY _cdb_gx, _cdb_gy - ), - _cdb_webmercator AS ( - SELECT - row_number() over() AS cartodb_id, - _cdb_feature_count, - ST_SetSRID( - ST_MakePoint( - _cdb_gx*_cdb_grid_size + _cdb_grid_size/2, - _cdb_gy*_cdb_grid_size + _cdb_grid_size/2 - ), - 3857 - ) AS the_geom_webmercator - FROM _cdb_gridded, _cdb_resolution - ) - SELECT - cartodb_id, - ST_Transform(the_geom_webmercator, 4326) AS the_geom, - the_geom_webmercator, - _cdb_feature_count - FROM _cdb_webmercator - ${baseQueryTemplate(ctx)} -`; +const rasterAggregationQueryTemplate = ctx => `${ctx.sourceQuery}`; diff --git a/lib/cartodb/models/aggregation/vector-aggregation.js b/lib/cartodb/models/aggregation/vector-aggregation.js index 42f070ac..d30f2429 100644 --- a/lib/cartodb/models/aggregation/vector-aggregation.js +++ b/lib/cartodb/models/aggregation/vector-aggregation.js @@ -1,54 +1,13 @@ const BaseAggregation = require('./base-aggregation'); -const { baseQueryTemplate } = BaseAggregation; module.exports = class VectorAggregation extends BaseAggregation { sql (options) { return vectorAggregationQueryTemplate({ - source_query: options.sql, + sourceQuery: options.sql, res: options.resolution, columns: options.columns }); } }; -const vectorAggregationQueryTemplate = ctx => ` - WITH - _cdb_source AS ( - -- original query - ${ctx.source_query} - ), - _cdb_resolution AS ( - SELECT ${ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) - AS _cdb_grid_size - -- equivalent to: - -- ${ctx.res}*!scale_denominator!*0.00028 - ), - _cdb_gridded AS ( - SELECT - Floor(ST_X(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gx, - Floor(ST_Y(_cdb_source.the_geom_webmercator)/_cdb_grid_size)::int AS _cdb_gy, - count(*) AS _cdb_feature_count - FROM _cdb_source, _cdb_resolution - GROUP BY _cdb_gx, _cdb_gy - ), - _cdb_webmercator AS ( - SELECT - row_number() over() AS cartodb_id, - _cdb_feature_count, - ST_SetSRID( - ST_MakePoint( - _cdb_gx*_cdb_grid_size + _cdb_grid_size/2, - _cdb_gy*_cdb_grid_size + _cdb_grid_size/2 - ), - 3857 - ) AS the_geom_webmercator - FROM _cdb_gridded, _cdb_resolution - ) - SELECT - cartodb_id, - ST_Transform(the_geom_webmercator, 4326) AS the_geom, - the_geom_webmercator, - _cdb_feature_count - FROM _cdb_webmercator - ${baseQueryTemplate(ctx)} -`; +const vectorAggregationQueryTemplate = ctx => `${ctx.sourceQuery}`; From 52630b80847007ee26eec385084aa6649172881b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 1 Dec 2017 17:06:42 +0100 Subject: [PATCH 09/78] Minor improvementes --- .../adapter/aggregation-mapconfig-adapter.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 07089f41..4e7d229a 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,4 +1,5 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); +const { MapConfig } = require('windshaft').model; module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { @@ -6,31 +7,33 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, requestMapConfig); } - requestMapConfig = this._adaptLayers(requestMapConfig); + requestMapConfig.layers = this._adaptLayers(requestMapConfig); callback(null, requestMapConfig); } _shouldAdaptLayers (requestMapConfig, params) { + let shouldAdapt = false; + if (typeof params.aggregation === 'boolean') { - return params.aggregation; + shouldAdapt = params.aggregation; } + const mapConfig = new MapConfig(requestMapConfig); + if (params.aggregation === undefined) { - if (requestMapConfig.isVectorLayergroup()) { - return true; + if (mapConfig.isVectorOnlyMapConfig()) { + shouldAdapt = true; } else if (this._hasAggregation(requestMapConfig)){ - return true; + shouldAdapt = true; } } - return false; + return shouldAdapt; } _hasAggregation (requestMapConfig) { for (const layer of requestMapConfig.layers) { - - if (this._hasLayerAggregation(layer)) { return true; } From 077f19d5061db24bea16e6e7340f83885c144253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 4 Dec 2017 12:40:53 +0100 Subject: [PATCH 10/78] Integrate aggregation and get metadata for layergroup --- lib/cartodb/controllers/map.js | 27 ++++- .../models/aggregation/aggregation-proxy.js | 6 +- .../adapter/aggregation-mapconfig-adapter.js | 26 +++-- test/acceptance/aggregation.js | 99 +++++++++++++++++++ 4 files changed, 145 insertions(+), 13 deletions(-) create mode 100644 test/acceptance/aggregation.js diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 6b50fc47..1689fbd1 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -87,6 +87,7 @@ MapController.prototype.composeCreateMapMiddleware = function (useTemplate = fal this.setDataviewsAndWidgetsUrlsToLayergroupMetadata(), this.setAnalysesMetadataToLayergroup(includeQuery), this.setTurboCartoMetadataToLayergroup(), + this.setAggregationMetadataToLayergroup(), this.setSurrogateKeyHeader(), this.sendResponse(), this.augmentError({ label, addContext }) @@ -540,13 +541,13 @@ MapController.prototype.setTurboCartoMetadataToLayergroup = function () { return function setTurboCartoMetadataToLayergroupMiddleware (req, res, next) { const { layergroup, mapconfig, context } = res.locals; - addContextMetadata(layergroup, mapconfig.obj(), context); + addTurboCartoContextMetadata(layergroup, mapconfig.obj(), context); next(); }; }; -function addContextMetadata(layergroup, mapConfig, context) { +function addTurboCartoContextMetadata(layergroup, mapConfig, context) { if (layergroup.metadata && Array.isArray(layergroup.metadata.layers) && Array.isArray(mapConfig.layers)) { layergroup.metadata.layers = layergroup.metadata.layers.map(function(layer, layerIndex) { if (context.turboCarto && Array.isArray(context.turboCarto.layers)) { @@ -557,6 +558,28 @@ function addContextMetadata(layergroup, mapConfig, context) { } } +// TODO: see how evolve this function, it's a good candidate to be refactored +MapController.prototype.setAggregationMetadataToLayergroup = function () { + return function setAggregationMetadataToLayergroupMiddleware (req, res, next) { + const { layergroup, mapconfig, context } = res.locals; + + addAggregationContextMetadata(layergroup, mapconfig.obj(), context); + + next(); + }; +}; + +function addAggregationContextMetadata(layergroup, mapConfig, context) { + if (layergroup.metadata && Array.isArray(layergroup.metadata.layers) && Array.isArray(mapConfig.layers)) { + layergroup.metadata.layers = layergroup.metadata.layers.map(function(layer, layerIndex) { + if (context.aggregation && Array.isArray(context.aggregation.layers)) { + layer.meta.aggregation = context.aggregation.layers[layerIndex]; + } + return layer; + }); + } +} + MapController.prototype.setSurrogateKeyHeader = function () { return function setSurrogateKeyHeaderMiddleware(req, res, next) { const { affectedTables, user, templateName } = res.locals; diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js index 7fff63f9..9de82887 100644 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -31,14 +31,14 @@ module.exports = class AggregationProxy { } _getAggregationType () { - if (this.mapconfig.isVetorLayergroup()) { + if (this.mapconfig.isVectorOnlyMapConfig()) { return VECTOR_AGGREGATION; } return RASTER_AGGREGATION; } - sql () { - return this.implementation.sql(); + sql (options) { + return this.implementation.sql(options); } }; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 4e7d229a..7b7468b7 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -3,11 +3,16 @@ const { MapConfig } = require('windshaft').model; module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { + this.mapConfig = new MapConfig(requestMapConfig); + if (!this._shouldAdaptLayers(requestMapConfig, params)) { return callback(null, requestMapConfig); } requestMapConfig.layers = this._adaptLayers(requestMapConfig); + context.aggregation = { + layers: this._getAggregationMetadata(requestMapConfig), + }; callback(null, requestMapConfig); } @@ -19,10 +24,8 @@ module.exports = class AggregationMapConfigAdapter { shouldAdapt = params.aggregation; } - const mapConfig = new MapConfig(requestMapConfig); - if (params.aggregation === undefined) { - if (mapConfig.isVectorOnlyMapConfig()) { + if (this.mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; } else if (this._hasAggregation(requestMapConfig)){ shouldAdapt = true; @@ -44,18 +47,17 @@ module.exports = class AggregationMapConfigAdapter { _hasLayerAggregation (layer) { const { aggregation } = layer.options; - return aggregation !== undefined && (typeof aggregation === 'object' && typeof aggregation === 'boolean'); + return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } _adaptLayers (requestMapConfig) { return requestMapConfig.layers.map(layer => { if (this._hasLayerAggregation(layer)) { - const aggregation = new AggregationProxy(requestMapConfig, layer.options.aggregation); - - let aggregationSql = aggregation.sql(); - + const aggregation = new AggregationProxy(this.mapConfig, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; + let aggregationSql = aggregation.sql(layer.options); + if (sqlQueryWrap) { layer.options.sql_raw = aggregationSql; aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); @@ -67,4 +69,12 @@ module.exports = class AggregationMapConfigAdapter { return layer; }); } + + _getAggregationMetadata (requestMapConfig) { + return requestMapConfig.layers.map(layer => { + return this._hasLayerAggregation(layer) ? + { aggregated: true } : + { aggregated: false }; + }); + } }; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js new file mode 100644 index 00000000..eeee18f0 --- /dev/null +++ b/test/acceptance/aggregation.js @@ -0,0 +1,99 @@ +require('../support/test_helper'); + +const assert = require('../support/assert'); +const TestClient = require('../support/test-client'); +const serverOptions = require('../../lib/cartodb/server_options'); + +const suites = [{ + desc: 'mvt (mapnik)', + usePostGIS: false +}]; + +if (process.env.POSTGIS_VERSION === '2.4') { + suites.push({ + desc: 'mvt (postgis)', + usePostGIS: true + }); +} + +describe('aggregation', function () { + + const POINTS_SQL_1 = ` + select + st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, + st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, + x as value + from generate_series(-3, 3) x + `; + + const POINTS_SQL_2 = ` + select + st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326) as the_geom, + st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, + x as value + from generate_series(-3, 3) x + `; + + function createVectorMapConfig (layers = [ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: true + } + }, + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: true + } + } + ]) { + return { + version: '1.6.0', + layers: layers + }; + } + + suites.forEach((suite) => { + const { desc, usePostGIS } = suite; + + describe(desc, function () { + const originalUsePostGIS = serverOptions.renderer.mvt.usePostGIS; + + before(function () { + serverOptions.renderer.mvt.usePostGIS = usePostGIS; + }); + + after(function (){ + serverOptions.renderer.mvt.usePostGIS = originalUsePostGIS; + }); + + + beforeEach(function () { + this.mapConfig = createVectorMapConfig(); + this.testClient = new TestClient(this.mapConfig); + }); + + afterEach(function (done) { + this.testClient.drain(done); + }); + + it('should return a layergroup indicating that was aggregated', function (done) { + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.aggregated)); + + done(); + }); + }); + }); + }); +}); From fc472e65b61b984e15c86be5ae03ad2b8733a6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 4 Dec 2017 15:31:45 +0100 Subject: [PATCH 11/78] Update yarn.lock --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 92315ec1..9a089a3d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -240,9 +240,9 @@ carto@0.16.3: semver "^5.1.0" yargs "^4.2.0" -"carto@github:cartodb/carto#0.15.1-cdb1": +carto@CartoDB/carto#0.15.1-cdb1: version "0.15.1-cdb1" - resolved "https://codeload.github.com/cartodb/carto/tar.gz/8050ec843f1f32a6469e5d1cf49602773015d398" + resolved "https://codeload.github.com/CartoDB/carto/tar.gz/8050ec843f1f32a6469e5d1cf49602773015d398" dependencies: mapnik-reference "~6.0.2" optimist "~0.6.0" @@ -2359,7 +2359,7 @@ window-size@^0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.2.0.tgz#b4315bb4214a3d7058ebeee892e13fa24d98b075" -windshaft@^4.1.0: +windshaft@4.1.0: version "4.1.0" resolved "https://registry.yarnpkg.com/windshaft/-/windshaft-4.1.0.tgz#dc17c8369570c305171d1ab5ca130369bba04d58" dependencies: From 855f47e446db37920de064460a08d1abed4bb30b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 4 Dec 2017 19:48:06 +0100 Subject: [PATCH 12/78] Detect incompatible CartoCSS or interactivity for raster aggregation --- .../adapter/aggregation-mapconfig-adapter.js | 65 +++++++++- test/acceptance/aggregation.js | 120 ++++++++++++++++-- 2 files changed, 170 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 7b7468b7..f9a9c840 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,15 +1,25 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); const { MapConfig } = require('windshaft').model; +const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; + module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { - this.mapConfig = new MapConfig(requestMapConfig); + const mapConfig = new MapConfig(requestMapConfig); - if (!this._shouldAdaptLayers(requestMapConfig, params)) { + if (!this._shouldAdaptLayers(mapConfig, requestMapConfig, params)) { return callback(null, requestMapConfig); } - requestMapConfig.layers = this._adaptLayers(requestMapConfig); + if (this._hasMissingColumns(mapConfig)) { + const error = new Error(MISSING_AGGREGATION_COLUMNS); + error.http_status = 400; + error.type = 'mapconfig'; + + return callback(error); + } + + requestMapConfig.layers = this._adaptLayers(mapConfig, requestMapConfig); context.aggregation = { layers: this._getAggregationMetadata(requestMapConfig), }; @@ -17,7 +27,48 @@ module.exports = class AggregationMapConfigAdapter { callback(null, requestMapConfig); } - _shouldAdaptLayers (requestMapConfig, params) { + _hasMissingColumns (mapConfig) { + const layers = mapConfig.getLayers(); + let missingColumns = false; + + for (let index = 0; index < layers.length; index++) { + const layer = layers[index]; + const { aggregation } = layer.options; + const hasAggregationColumns = aggregation !== undefined && + typeof aggregation !== 'boolean' && + typeof aggregation.columns === 'object'; + const aggregationColumns = hasAggregationColumns ? Object.keys(aggregation.columns) : []; + const layerColumns = mapConfig.getColumnsByLayer(index); + + if (layerColumns.length === 0) { + continue; + } + + if (aggregationColumns.length === 0) { + missingColumns = true; + break; + } + + if (!this._haveSameColumns(aggregationColumns,layerColumns)) { + missingColumns = true; + break; + } + } + + return missingColumns; + } + + _haveSameColumns (aggregationColumns, layerColumns) { + if (aggregationColumns.length !== layerColumns.length) { + return false; + } + + const diff = aggregationColumns.filter(column => !layerColumns.includes(column)); + + return !diff.length; + } + + _shouldAdaptLayers (mapConfig, requestMapConfig, params) { let shouldAdapt = false; if (typeof params.aggregation === 'boolean') { @@ -25,7 +76,7 @@ module.exports = class AggregationMapConfigAdapter { } if (params.aggregation === undefined) { - if (this.mapConfig.isVectorOnlyMapConfig()) { + if (mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; } else if (this._hasAggregation(requestMapConfig)){ shouldAdapt = true; @@ -50,10 +101,10 @@ module.exports = class AggregationMapConfigAdapter { return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } - _adaptLayers (requestMapConfig) { + _adaptLayers (mapConfig, requestMapConfig) { return requestMapConfig.layers.map(layer => { if (this._hasLayerAggregation(layer)) { - const aggregation = new AggregationProxy(this.mapConfig, layer.options.aggregation); + const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = aggregation.sql(layer.options); diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index eeee18f0..3638ff0b 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -3,6 +3,7 @@ require('../support/test_helper'); const assert = require('../support/assert'); const TestClient = require('../support/test-client'); const serverOptions = require('../../lib/cartodb/server_options'); +const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; const suites = [{ desc: 'mvt (mapnik)', @@ -30,7 +31,8 @@ describe('aggregation', function () { select st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, - x as value + x as value, + x*x as sqrt_value from generate_series(-3, 3) x `; @@ -70,17 +72,14 @@ describe('aggregation', function () { serverOptions.renderer.mvt.usePostGIS = originalUsePostGIS; }); - - beforeEach(function () { - this.mapConfig = createVectorMapConfig(); - this.testClient = new TestClient(this.mapConfig); - }); - afterEach(function (done) { this.testClient.drain(done); }); - it('should return a layergroup indicating that was aggregated', function (done) { + it('should return a layergroup indicating the mapconfig was aggregated', function (done) { + this.mapConfig = createVectorMapConfig(); + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup((err, body) => { if (err) { return done(err); @@ -94,6 +93,111 @@ describe('aggregation', function () { done(); }); }); + + it('should return a layergroup with aggregation and cartocss compatible', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + }, + cartocss: '#layer { marker-width: [value]*2; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup((err/*, body */) => { + if (err) { + return done(err); + } + + done(); + }); + }); + + it('should fail if cartocss uses "value" cloumn and it\'s not defined in the aggregation', + function (done) { + const response = { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: true, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup(response, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + + done(); + }); + }); + + it('should fail if aggregation misses a column defined in interactivity', + function (done) { + const response = { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + }, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0', + interactivity: ['sqrt_value'] + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup(response, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + + done(); + }); + }); + }); }); }); From 499e9de75d7b31fb6094c1ab6643c466558ad906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 4 Dec 2017 19:49:35 +0100 Subject: [PATCH 13/78] Use devel branch of windshaft --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0a6a9014..0fdec8bd 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "step-profiler": "~0.3.0", "turbo-carto": "0.20.2", "underscore": "~1.6.0", - "windshaft": "4.1.0", + "windshaft": "cartodb/windshaft#mapconfig-aggregation", "yargs": "~5.0.0" }, "devDependencies": { From 66b41a6ae70dca4f4de3b2822e168cb0cfea9900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 12:09:31 +0100 Subject: [PATCH 14/78] Now .getLayergroup() in test client accepts params to perform custom instantiations --- test/acceptance/aggregation.js | 4 +- test/acceptance/analysis/analysis-layers.js | 4 +- test/acceptance/analysis/error-cases.js | 16 +++--- test/acceptance/dataviews/error-cases.js | 4 +- .../ported/multilayer_error_cases.js | 2 +- test/acceptance/regressions.js | 2 +- test/acceptance/turbo-carto/error-cases.js | 12 ++--- test/acceptance/turbo-carto/regressions.js | 2 +- .../acceptance/user-database-timeout-limit.js | 28 +++++----- test/acceptance/user-render-timeout-limit.js | 5 +- test/acceptance/vector-layergroup.js | 2 +- test/support/test-client.js | 53 ++++++++++++++----- 12 files changed, 82 insertions(+), 52 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 3638ff0b..c278918a 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -146,7 +146,7 @@ describe('aggregation', function () { ]); this.testClient = new TestClient(this.mapConfig); - this.testClient.getLayergroup(response, (err, body) => { + this.testClient.getLayergroup({ response }, (err, body) => { if (err) { return done(err); } @@ -187,7 +187,7 @@ describe('aggregation', function () { ]); this.testClient = new TestClient(this.mapConfig); - this.testClient.getLayergroup(response, (err, body) => { + this.testClient.getLayergroup({ response }, (err, body) => { if (err) { return done(err); } diff --git a/test/acceptance/analysis/analysis-layers.js b/test/acceptance/analysis/analysis-layers.js index a745fb97..1eb3d4e6 100644 --- a/test/acceptance/analysis/analysis-layers.js +++ b/test/acceptance/analysis/analysis-layers.js @@ -174,7 +174,9 @@ describe('analysis-layers', function() { } }; - testClient.getLayergroup(PERMISSION_DENIED_RESPONSE, function(err, layergroupResult) { + + + testClient.getLayergroup({ response: PERMISSION_DENIED_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.deepEqual( layergroupResult.errors, diff --git a/test/acceptance/analysis/error-cases.js b/test/acceptance/analysis/error-cases.js index b79e1046..376d7606 100644 --- a/test/acceptance/analysis/error-cases.js +++ b/test/acceptance/analysis/error-cases.js @@ -62,7 +62,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -97,7 +97,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -144,7 +144,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -190,7 +190,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 11111); - testClient.getLayergroup(AUTH_ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: AUTH_ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -246,7 +246,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -298,7 +298,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -351,7 +351,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); @@ -415,7 +415,7 @@ describe('analysis-layers error cases', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); diff --git a/test/acceptance/dataviews/error-cases.js b/test/acceptance/dataviews/error-cases.js index 7e37e8ee..3e6cc2a8 100644 --- a/test/acceptance/dataviews/error-cases.js +++ b/test/acceptance/dataviews/error-cases.js @@ -51,7 +51,7 @@ describe('histogram-dataview', function() { it('should fail when invalid dataviews object is provided, string case', function(done) { var mapConfig = createMapConfig("wadus-string"); this.testClient = new TestClient(mapConfig, 1234); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, errObj) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, errObj) { assert.ok(!err, err); assert.deepEqual(errObj.errors, [ '"dataviews" must be a valid JSON object: "string" type found' ]); @@ -63,7 +63,7 @@ describe('histogram-dataview', function() { it('should fail when invalid dataviews object is provided, array case', function(done) { var mapConfig = createMapConfig([]); this.testClient = new TestClient(mapConfig, 1234); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, errObj) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, errObj) { assert.ok(!err, err); assert.deepEqual(errObj.errors, [ '"dataviews" must be a valid JSON object: "array" type found' ]); diff --git a/test/acceptance/ported/multilayer_error_cases.js b/test/acceptance/ported/multilayer_error_cases.js index 9efd7c7b..6e7f5964 100644 --- a/test/acceptance/ported/multilayer_error_cases.js +++ b/test/acceptance/ported/multilayer_error_cases.js @@ -151,7 +151,7 @@ describe('multilayer error cases', function() { }; ServerOptions.afterLayergroupCreateCalls = 0; this.client = new TestClient(layergroup); - this.client.getLayergroup({status: 400}, function(err, parsed) { + this.client.getLayergroup({ response: { status: 400 } }, function(err, parsed) { assert.ok(!err, err); // See http://github.com/CartoDB/Windshaft/issues/159 assert.equal(ServerOptions.afterLayergroupCreateCalls, 0); diff --git a/test/acceptance/regressions.js b/test/acceptance/regressions.js index 43dad070..421df02f 100644 --- a/test/acceptance/regressions.js +++ b/test/acceptance/regressions.js @@ -28,7 +28,7 @@ describe('regressions', function() { var testClient = new TestClient(mapConfig, 1234); - testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroupResult) { + testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroupResult) { assert.ok(!err, err); assert.equal(layergroupResult.errors.length, 1); diff --git a/test/acceptance/turbo-carto/error-cases.js b/test/acceptance/turbo-carto/error-cases.js index 0d25ae93..b6424c72 100644 --- a/test/acceptance/turbo-carto/error-cases.js +++ b/test/acceptance/turbo-carto/error-cases.js @@ -51,7 +51,7 @@ describe('turbo-carto error cases', function() { it('should return invalid number of ramp error', function(done) { this.testClient = new TestClient(makeMapconfig('ramp([pop_max], 8, 96, 3, (8,24,96,128))')); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); @@ -65,7 +65,7 @@ describe('turbo-carto error cases', function() { it('should return invalid column from datasource', function(done) { this.testClient = new TestClient(makeMapconfig(null, 'ramp([wadus_column], (red, green, blue))')); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); @@ -80,7 +80,7 @@ describe('turbo-carto error cases', function() { it('should return invalid method from datasource', function(done) { this.testClient = new TestClient(makeMapconfig(null, 'ramp([wadus_column], (red, green, blue), wadusmethod)')); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); @@ -95,7 +95,7 @@ describe('turbo-carto error cases', function() { it('should fail by falling back to normal carto parser', function(done) { this.testClient = new TestClient(makeMapconfig('ramp([price], (8,24,96), (8,24,96));//(red, green, blue))')); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); @@ -109,7 +109,7 @@ describe('turbo-carto error cases', function() { it('turbo-carto: should return error invalid column from datasource with some context', function(done) { this.testClient = new TestClient(makeMapconfig(null, 'ramp([wadus_column], (red, green, blue))')); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); @@ -164,7 +164,7 @@ describe('turbo-carto error cases', function() { }; this.testClient = new TestClient(multipleErrorsMapConfig); - this.testClient.getLayergroup(ERROR_RESPONSE, function(err, layergroup) { + this.testClient.getLayergroup({ response: ERROR_RESPONSE }, function(err, layergroup) { assert.ok(!err, err); assert.ok(layergroup.hasOwnProperty('errors')); diff --git a/test/acceptance/turbo-carto/regressions.js b/test/acceptance/turbo-carto/regressions.js index f7f39ccb..9e2d377f 100644 --- a/test/acceptance/turbo-carto/regressions.js +++ b/test/acceptance/turbo-carto/regressions.js @@ -79,7 +79,7 @@ describe('turbo-carto regressions', function() { ].join('\n'); this.testClient = new TestClient(makeMapconfig('SELECT * FROM test_table_private_1', cartocss)); - this.testClient.getLayergroup(TestClient.RESPONSE.ERROR, function(err, layergroup) { + this.testClient.getLayergroup({ response: TestClient.RESPONSE.ERROR }, function(err, layergroup) { assert.ok(!err, err); assert.ok(!layergroup.hasOwnProperty('layergroupid')); diff --git a/test/acceptance/user-database-timeout-limit.js b/test/acceptance/user-database-timeout-limit.js index 9893f00a..52ead026 100644 --- a/test/acceptance/user-database-timeout-limit.js +++ b/test/acceptance/user-database-timeout-limit.js @@ -138,7 +138,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], errors_with_context: [{ @@ -177,7 +177,7 @@ describe('user database timeout limit', function () { return done(err); } - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } @@ -259,7 +259,7 @@ describe('user database timeout limit', function () { return done(err); } - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } @@ -360,7 +360,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], errors_with_context: [{ @@ -387,7 +387,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } @@ -426,16 +426,16 @@ describe('user database timeout limit', function () { this.testClient.getTile(0, 0, 0, params, (err, res, tile) => { assert.ifError(err); - + var tileJSON = tile.toJSON(); assert.equal(Array.isArray(tileJSON), true); assert.equal(tileJSON.length, 2); assert.equal(tileJSON[0].name, 'errorTileSquareLayer'); assert.equal(tileJSON[1].name, 'errorTileStripesLayer'); - + done(); }); - + }); }); }); @@ -467,7 +467,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], errors_with_context: [{ @@ -494,7 +494,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } @@ -571,7 +571,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], errors_with_context: [{ @@ -601,7 +601,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } @@ -702,7 +702,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.deepEqual(timeoutError, { errors: [ 'You are over platform\'s limits. Please contact us to know more details' ], errors_with_context: [{ @@ -740,7 +740,7 @@ describe('user database timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, res) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, res) => { if (err) { return done(err); } diff --git a/test/acceptance/user-render-timeout-limit.js b/test/acceptance/user-render-timeout-limit.js index 5942d045..93770080 100644 --- a/test/acceptance/user-render-timeout-limit.js +++ b/test/acceptance/user-render-timeout-limit.js @@ -92,7 +92,7 @@ describe('user render timeout limit', function () { } }; - this.testClient.getLayergroup(expectedResponse, (err, timeoutError) => { + this.testClient.getLayergroup({ response: expectedResponse }, (err, timeoutError) => { assert.ifError(err); assert.deepEqual(timeoutError, { @@ -245,7 +245,7 @@ describe('user render timeout limit', function () { assert.equal(tileJSON.length, 2); assert.equal(tileJSON[0].name, 'errorTileSquareLayer'); assert.equal(tileJSON[1].name, 'errorTileStripesLayer'); - + done(); }); }); @@ -399,4 +399,3 @@ describe('user render timeout limit', function () { }); }); }); - diff --git a/test/acceptance/vector-layergroup.js b/test/acceptance/vector-layergroup.js index 54ec29ef..0778453e 100644 --- a/test/acceptance/vector-layergroup.js +++ b/test/acceptance/vector-layergroup.js @@ -206,7 +206,7 @@ suites.forEach((suite) => { this.testClient.mapConfig.layers[0].options.cartocss = cartocss; this.testClient.mapConfig.layers[0].options.cartocss_version = cartocssVersion; - this.testClient.getLayergroup(response, (err, body) => { + this.testClient.getLayergroup({ response }, (err, body) => { if (err) { return done(err); } diff --git a/test/support/test-client.js b/test/support/test-client.js index 3188d0fc..cda5287a 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -618,8 +618,19 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { } var data = templateId ? params.placeholders : self.mapConfig; + + const queryParams = {}; + + if (self.apiKey) { + queryParams.api_key = self.apiKey; + } + + if (typeof params.aggregation === 'boolean') { + queryParams.aggregation = params.aggregation; + } + var path = templateId ? - urlNamed + '/' + templateId + '?' + qs.stringify({api_key: self.apiKey}) : + urlNamed + '/' + templateId + '?' + qs.stringify(queryParams) : url; assert.response(self.server, @@ -647,7 +658,7 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { ); }, function getTileResult(err, layergroupId) { - // jshint maxcomplexity:12 + // jshint maxcomplexity:13 assert.ifError(err); self.keysToDelete['map_cfg|' + LayergroupToken.parse(layergroupId).token] = 0; @@ -671,8 +682,14 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { url += [z,x,y].join('/'); url += '.' + format; + const queryParams = {}; + if (self.apiKey) { - url += '?' + qs.stringify({api_key: self.apiKey}); + queryParams.api_key = self.apiKey; + } + + if (Object.keys(queryParams).length) { + url += '?' + qs.stringify(queryParams); } var request = { @@ -754,23 +771,35 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { ); }; -TestClient.prototype.getLayergroup = function(expectedResponse, callback) { +TestClient.prototype.getLayergroup = function(params, callback) { var self = this; if (!callback) { - callback = expectedResponse; - expectedResponse = { - status: 200, - headers: { - 'Content-Type': 'application/json; charset=utf-8' + callback = params; + params = { + response: { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } } }; } var url = '/api/v1/map'; - if (this.apiKey) { - url += '?' + qs.stringify({api_key: this.apiKey}); + const queryParams = {}; + + if (self.apiKey) { + queryParams.api_key = self.apiKey; + } + + if (typeof params.aggregation === 'boolean') { + queryParams.aggregation = params.aggregation; + } + + if (Object.keys(queryParams).length) { + url += '?' + qs.stringify(queryParams); } assert.response(self.server, @@ -783,7 +812,7 @@ TestClient.prototype.getLayergroup = function(expectedResponse, callback) { }, data: JSON.stringify(self.mapConfig) }, - expectedResponse, + params.response, function(res, err) { var parsedBody; // If there is a response, we are still interested in catching the created keys From 55dd0498128fbcab11055f56716b57d36150b1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 12:59:32 +0100 Subject: [PATCH 15/78] Be able to skip aggregation to create a layergroup with aggregation defined already --- lib/cartodb/controllers/map.js | 2 ++ .../adapter/aggregation-mapconfig-adapter.js | 12 ++++--- test/acceptance/aggregation.js | 34 +++++++++++++++++++ test/support/test-client.js | 21 ++++++++---- 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 1689fbd1..185a4f8d 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -6,6 +6,7 @@ var ResourceLocator = require('../models/resource-locator'); var cors = require('../middleware/cors'); var userMiddleware = require('../middleware/user'); +const allowQueryParams = require('../middleware/allow-query-params'); var MapConfig = windshaft.model.MapConfig; var Datasource = windshaft.model.Datasource; @@ -69,6 +70,7 @@ MapController.prototype.composeCreateMapMiddleware = function (useTemplate = fal return [ cors(), userMiddleware, + allowQueryParams(['aggregation']), this.prepareContext, this.initProfiler(isTemplateInstantiation), this.checkJsonContentType(), diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index f9a9c840..7f1dce9d 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -69,13 +69,15 @@ module.exports = class AggregationMapConfigAdapter { } _shouldAdaptLayers (mapConfig, requestMapConfig, params) { + const { aggregation } = params; + let shouldAdapt = false; - if (typeof params.aggregation === 'boolean') { - shouldAdapt = params.aggregation; - } - - if (params.aggregation === undefined) { + if (aggregation === 'false') { + shouldAdapt = false; + } else if (aggregation === 'true') { + shouldAdapt = true; + } else if (aggregation === undefined) { if (mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; } else if (this._hasAggregation(requestMapConfig)){ diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index c278918a..282f0c78 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -198,6 +198,40 @@ describe('aggregation', function () { }); }); + it('should skip aggregation to create a layergroup with aggregation defined already', function (done) { + const mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + } + } + } + ]); + + this.testClient = new TestClient(mapConfig); + const options = { aggregation: false }; + + this.testClient.getLayergroup(options, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation === undefined)); + + done(); + }); + }); }); }); }); diff --git a/test/support/test-client.js b/test/support/test-client.js index cda5287a..54581f99 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -771,17 +771,24 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { ); }; -TestClient.prototype.getLayergroup = function(params, callback) { +TestClient.prototype.getLayergroup = function (params, callback) { + // jshint maxcomplexity: 7 var self = this; if (!callback) { callback = params; - params = { - response: { - status: 200, - headers: { - 'Content-Type': 'application/json; charset=utf-8' - } + params = null; + } + + if (!params) { + params = {}; + } + + if (!params.response) { + params.response = { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' } }; } From 4f8b541010a35135a6a8ee4f0b10c8266256a479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 13:12:25 +0100 Subject: [PATCH 16/78] Mark aggregation queries --- lib/cartodb/models/aggregation/raster-aggregation.js | 2 +- lib/cartodb/models/aggregation/vector-aggregation.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/aggregation/raster-aggregation.js b/lib/cartodb/models/aggregation/raster-aggregation.js index 51c30773..f06ee380 100644 --- a/lib/cartodb/models/aggregation/raster-aggregation.js +++ b/lib/cartodb/models/aggregation/raster-aggregation.js @@ -10,4 +10,4 @@ module.exports = class RasterAggregation extends BaseAggregation { } }; -const rasterAggregationQueryTemplate = ctx => `${ctx.sourceQuery}`; +const rasterAggregationQueryTemplate = ctx => `/** aggregated query (raster) **/ ${ctx.sourceQuery}`; diff --git a/lib/cartodb/models/aggregation/vector-aggregation.js b/lib/cartodb/models/aggregation/vector-aggregation.js index d30f2429..c1845f63 100644 --- a/lib/cartodb/models/aggregation/vector-aggregation.js +++ b/lib/cartodb/models/aggregation/vector-aggregation.js @@ -10,4 +10,4 @@ module.exports = class VectorAggregation extends BaseAggregation { } }; -const vectorAggregationQueryTemplate = ctx => `${ctx.sourceQuery}`; +const vectorAggregationQueryTemplate = ctx => `/** aggregated query (vector) **/ ${ctx.sourceQuery}`; From 7b35701fa8b4b0753576970e1c932a17bc6c4974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 16:50:18 +0100 Subject: [PATCH 17/78] Extract method --- .../adapter/aggregation-mapconfig-adapter.js | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 7f1dce9d..b674a435 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -34,10 +34,7 @@ module.exports = class AggregationMapConfigAdapter { for (let index = 0; index < layers.length; index++) { const layer = layers[index]; const { aggregation } = layer.options; - const hasAggregationColumns = aggregation !== undefined && - typeof aggregation !== 'boolean' && - typeof aggregation.columns === 'object'; - const aggregationColumns = hasAggregationColumns ? Object.keys(aggregation.columns) : []; + const aggregationColumns = this._getAggregationColumns(aggregation); const layerColumns = mapConfig.getColumnsByLayer(index); if (layerColumns.length === 0) { @@ -126,8 +123,23 @@ module.exports = class AggregationMapConfigAdapter { _getAggregationMetadata (requestMapConfig) { return requestMapConfig.layers.map(layer => { return this._hasLayerAggregation(layer) ? - { aggregated: true } : - { aggregated: false }; + { png: false, mvt: true } : + { png: false, mvt: false }; }); } + + _getAggregationColumns (aggregation) { + const hasAggregationColumns = aggregation !== undefined && + typeof aggregation !== 'boolean' && + typeof aggregation.columns === 'object'; + let aggregationColumns = []; + + if (hasAggregationColumns) { + aggregationColumns = Object.keys(aggregation.columns).map(key => { + return aggregation.columns[key].aggregated_column; + }); + } + + return aggregationColumns; + } }; From 81d99ca655cf7191cba8cb18ee20ea910c4ab6a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 16:52:15 +0100 Subject: [PATCH 18/78] Make test to pass --- test/acceptance/aggregation.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 282f0c78..6aa93dd7 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -61,7 +61,7 @@ describe('aggregation', function () { suites.forEach((suite) => { const { desc, usePostGIS } = suite; - describe(desc, function () { + describe.only(desc, function () { const originalUsePostGIS = serverOptions.renderer.mvt.usePostGIS; before(function () { @@ -88,7 +88,7 @@ describe('aggregation', function () { assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); - body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.aggregated)); + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); done(); }); @@ -108,23 +108,28 @@ describe('aggregation', function () { } } }, - cartocss: '#layer { marker-width: [value]*2; }', + cartocss: '#layer { marker-width: [value]; }', cartocss_version: '2.3.0' } } ]); this.testClient = new TestClient(this.mapConfig); - this.testClient.getLayergroup((err/*, body */) => { + this.testClient.getLayergroup((err, body) => { if (err) { return done(err); } + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + done(); }); }); - it('should fail if cartocss uses "value" cloumn and it\'s not defined in the aggregation', + it('should fail if cartocss uses "value" column and it\'s not defined in the aggregation', function (done) { const response = { status: 400, From e7592ee570b92e72f00e3d885c087f224d9992b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 17:44:52 +0100 Subject: [PATCH 19/78] Improve error message --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 3 ++- test/acceptance/aggregation.js | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index b674a435..310610d3 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,7 +1,8 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); const { MapConfig } = require('windshaft').model; -const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; +const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ +' interactivity fields or attributes that are not present in the aggregation'; module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 6aa93dd7..8939358b 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -3,7 +3,8 @@ require('../support/test_helper'); const assert = require('../support/assert'); const TestClient = require('../support/test-client'); const serverOptions = require('../../lib/cartodb/server_options'); -const MISSING_AGGREGATION_COLUMNS = 'There are missing columns to perform aggregation'; +const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ + ' interactivity fields or attributes that are not present in the aggregation'; const suites = [{ desc: 'mvt (mapnik)', @@ -61,7 +62,7 @@ describe('aggregation', function () { suites.forEach((suite) => { const { desc, usePostGIS } = suite; - describe.only(desc, function () { + describe(desc, function () { const originalUsePostGIS = serverOptions.renderer.mvt.usePostGIS; before(function () { From 9118e2dc5ed633db324ddbb870f73f4ee5f52038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 20:21:20 +0100 Subject: [PATCH 20/78] Add tests --- test/acceptance/aggregation.js | 70 ++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 8939358b..178478db 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -37,6 +37,15 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; + + const POINTS_SQL_NO_THE_GEOM = ` + select + st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, + x as value, + x*x as sqrt_value + from generate_series(-3, 3) x + `; + function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -90,6 +99,7 @@ describe('aggregation', function () { assert.ok(Array.isArray(body.metadata.layers)); body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); done(); }); @@ -125,6 +135,7 @@ describe('aggregation', function () { assert.ok(Array.isArray(body.metadata.layers)); body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.png)); done(); }); @@ -238,6 +249,65 @@ describe('aggregation', function () { done(); }); }); + + it('without the_geom defined in the sql should return a layergroup ', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_NO_THE_GEOM, + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + console.log(require('util').inspect(body, { depth: null })); + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); + + done(); + }); + }); + + it('without the_geom defined in the sql should get a vector tile', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_NO_THE_GEOM, + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getTile(0, 0, 0, { format: 'mvt' }, (err, res, tile) => { + if (err) { + return done(err); + } + + assert.equal(tile.tileSize, 4096); + assert.equal(tile.z, 0); + assert.equal(tile.x, 0); + assert.equal(tile.y, 0); + + const layer0 = JSON.parse(tile.toGeoJSONSync(0)); + + assert.equal(layer0.name, 'layer0'); + assert.equal(layer0.features[0].type, 'Feature'); + assert.equal(layer0.features[0].geometry.type, 'Point'); + + done(); + }); + }); + }); }); }); From 214d684fcc9922ae63f037ab7f95c2dc771a3d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 5 Dec 2017 20:39:30 +0100 Subject: [PATCH 21/78] Adapt layer when is vector only map-caonfig --- .../adapter/aggregation-mapconfig-adapter.js | 15 +++++++++++---- test/acceptance/aggregation.js | 1 - 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 310610d3..eb78ce65 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -22,7 +22,7 @@ module.exports = class AggregationMapConfigAdapter { requestMapConfig.layers = this._adaptLayers(mapConfig, requestMapConfig); context.aggregation = { - layers: this._getAggregationMetadata(requestMapConfig), + layers: this._getAggregationMetadata(mapConfig, requestMapConfig), }; callback(null, requestMapConfig); @@ -102,8 +102,9 @@ module.exports = class AggregationMapConfigAdapter { } _adaptLayers (mapConfig, requestMapConfig) { + const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); return requestMapConfig.layers.map(layer => { - if (this._hasLayerAggregation(layer)) { + if (isVectorOnlyMapConfig || this._hasLayerAggregation(layer)) { const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; @@ -121,10 +122,16 @@ module.exports = class AggregationMapConfigAdapter { }); } - _getAggregationMetadata (requestMapConfig) { + _getAggregationMetadata (mapConfig, requestMapConfig) { + if (mapConfig.isVectorOnlyMapConfig()) { + return requestMapConfig.layers.map((/* layer */) => { + return { png: false, mvt: true }; + }); + } + return requestMapConfig.layers.map(layer => { return this._hasLayerAggregation(layer) ? - { png: false, mvt: true } : + { png: true, mvt: true } : { png: false, mvt: false }; }); } diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 178478db..cf6b3444 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -266,7 +266,6 @@ describe('aggregation', function () { return done(err); } - console.log(require('util').inspect(body, { depth: null })); assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); From dab204ea71baee73006774e26e64e9919542776a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 17:32:06 +0100 Subject: [PATCH 22/78] Do not aggregate if rows cout is lower than threshold or the layer's sql has geometries distinct of points --- .../adapter/aggregation-mapconfig-adapter.js | 119 +++++++++++++----- lib/cartodb/server.js | 2 +- lib/cartodb/utils/query-utils.js | 20 ++- test/acceptance/aggregation.js | 103 ++++++++++----- 4 files changed, 182 insertions(+), 62 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index eb78ce65..bc401542 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,10 +1,17 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); const { MapConfig } = require('windshaft').model; +const queryUtils = require('../../../utils/query-utils'); const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ ' interactivity fields or attributes that are not present in the aggregation'; +const unsupportedGeometryTypeErrorMessage = ctx => +`Unsupported geometry type (${ctx.geometryType}) for aggregation. Aggregation is available only for points.`; module.exports = class AggregationMapConfigAdapter { + constructor (pgConnection) { + this.pgConnection = pgConnection; + } + getMapConfig (user, requestMapConfig, params, context, callback) { const mapConfig = new MapConfig(requestMapConfig); @@ -20,12 +27,13 @@ module.exports = class AggregationMapConfigAdapter { return callback(error); } - requestMapConfig.layers = this._adaptLayers(mapConfig, requestMapConfig); - context.aggregation = { - layers: this._getAggregationMetadata(mapConfig, requestMapConfig), - }; + this._adaptLayers(user, mapConfig, requestMapConfig, context, (err, requestMapConfig) => { + if (err) { + return callback(err); + } - callback(null, requestMapConfig); + callback(null, requestMapConfig); + }); } _hasMissingColumns (mapConfig) { @@ -101,39 +109,90 @@ module.exports = class AggregationMapConfigAdapter { return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } - _adaptLayers (mapConfig, requestMapConfig) { - const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); - return requestMapConfig.layers.map(layer => { - if (isVectorOnlyMapConfig || this._hasLayerAggregation(layer)) { - const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); - const sqlQueryWrap = layer.options.sql_wrap; - - let aggregationSql = aggregation.sql(layer.options); - - if (sqlQueryWrap) { - layer.options.sql_raw = aggregationSql; - aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); - } - - layer.options.sql = aggregationSql; + _adaptLayers (user, mapConfig, requestMapConfig, context, callback) { + this.pgConnection.getConnection(user, (err, connection) => { + if (err) { + return callback(err); } - return layer; + const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); + + const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { + return new Promise((resolve, reject) => { + if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { + return resolve({ layer, index, adapted: false }); + } + + const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? + layer.options.aggregation.threshold : + 1e5; + + const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); + + connection.query(aggregationMetadata, (err, res) => { + if (err) { + return resolve({ layer, index, adapted: false }); + } + + const estimatedFeatureCount = res.rows[0].count; + const geometryType = res.rows[0].type; + + if (estimatedFeatureCount < threshold) { + return resolve({ layer, index, adapted: false }); + } + + if (geometryType !== 'ST_Point') { + return reject(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); + } + + const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); + const sqlQueryWrap = layer.options.sql_wrap; + + let aggregationSql = aggregation.sql(layer.options); + + if (sqlQueryWrap) { + layer.options.sql_raw = aggregationSql; + aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); + } + + layer.options.sql = aggregationSql; + + return resolve({ layer, index, adapted: true }); + }); + }); + }); + + Promise.all(adaptLayerPromises) + .then(results => { + context.aggregation = { + layers: [] + }; + + results.forEach(({ layer, index, adapted }) => { + if (adapted) { + requestMapConfig.layers[index] = layer; + } + const aggregatedFormats = this._getAggregationMetadata(isVectorOnlyMapConfig, layer, adapted); + context.aggregation.layers.push(aggregatedFormats); + }); + + return requestMapConfig; + }) + .then(requestMapConfig => callback(null, requestMapConfig)) + .catch(err => callback(err)); }); } - _getAggregationMetadata (mapConfig, requestMapConfig) { - if (mapConfig.isVectorOnlyMapConfig()) { - return requestMapConfig.layers.map((/* layer */) => { + _getAggregationMetadata (isVectorOnlyMapConfig, layer, adapted) { + if (adapted) { + if (isVectorOnlyMapConfig) { return { png: false, mvt: true }; - }); + } + + return { png: true, mvt: true }; } - return requestMapConfig.layers.map(layer => { - return this._hasLayerAggregation(layer) ? - { png: true, mvt: true } : - { png: false, mvt: false }; - }); + return { png: false, mvt: false }; } _getAggregationColumns (aggregation) { diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index b7f0e8a0..46ff7122 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -191,7 +191,7 @@ module.exports = function(serverOptions) { new SqlWrapMapConfigAdapter(), new DataviewsWidgetsAdapter(), new AnalysisMapConfigAdapter(analysisBackend), - new AggregationMapConfigAdapter(), + new AggregationMapConfigAdapter(pgConnection), new MapConfigOverviewsAdapter(overviewsMetadataApi, filterStatsApi), new TurboCartoAdapter() ); diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 47d730f4..873151d8 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -21,6 +21,22 @@ module.exports.extractTableNames = function extractTableNames(query) { ].join(''); }; -module.exports.getQueryRowCount = function getQueryRowEstimation(query) { +function getQueryRowEstimation(query) { return 'select CDB_EstimateRowCount(\'' + query + '\') as rows'; -}; +} +module.exports.getQueryRowCount = getQueryRowEstimation; + +module.exports.getAggregationMetadata = ctx => ` + WITH + rowEstimation AS ( + ${getQueryRowEstimation(ctx.query)} + ), + geometryType AS ( + SELECT ST_GeometryType(the_geom) as geom_type + FROM (${ctx.query}) AS __cdb_query WHERE the_geom IS NOT NULL LIMIT 1 + ) + SELECT + rows AS count, + geom_type AS type + FROM rowEstimation, geometryType; +`; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index cf6b3444..2576a09c 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -38,14 +38,18 @@ describe('aggregation', function () { `; - const POINTS_SQL_NO_THE_GEOM = ` + const POLYGONS_SQL_1 = ` select - st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, - x as value, - x*x as sqrt_value + st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry as the_geom, + st_transform( + st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry, + 3857 + ) as the_geom_webmercator, + x as value from generate_series(-3, 3) x `; + function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -87,7 +91,26 @@ describe('aggregation', function () { }); it('should return a layergroup indicating the mapconfig was aggregated', function (done) { - this.mapConfig = createVectorMapConfig(); + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1 + } + } + }, + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + threshold: 1 + } + } + } + ]); this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -117,7 +140,8 @@ describe('aggregation', function () { aggregate_function: 'sum', aggregated_column: 'value' } - } + }, + threshold: 1 }, cartocss: '#layer { marker-width: [value]; }', cartocss_version: '2.3.0' @@ -250,18 +274,29 @@ describe('aggregation', function () { }); }); - it('without the_geom defined in the sql should return a layergroup ', function (done) { - this.mapConfig = createVectorMapConfig([ + it('when the layer\'s row count is lower than threshold should skip aggregation', function (done) { + const mapConfig = createVectorMapConfig([ { type: 'cartodb', options: { - sql: POINTS_SQL_NO_THE_GEOM, + sql: POINTS_SQL_1, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + threshold: 1001 + } } } ]); - this.testClient = new TestClient(this.mapConfig); - this.testClient.getLayergroup((err, body) => { + this.testClient = new TestClient(mapConfig); + const options = {}; + + this.testClient.getLayergroup(options, (err, body) => { if (err) { return done(err); } @@ -269,44 +304,54 @@ describe('aggregation', function () { assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); - body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); - body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); + body.metadata.layers.forEach(layer =>{ + assert.deepEqual(layer.meta.aggregation, { png: false, mvt: false }); + }); done(); }); }); - it('without the_geom defined in the sql should get a vector tile', function (done) { - this.mapConfig = createVectorMapConfig([ + it('when the layer\'s geometry type is not point should responds with error', function (done) { + const mapConfig = createVectorMapConfig([ { type: 'cartodb', options: { - sql: POINTS_SQL_NO_THE_GEOM, + sql: POLYGONS_SQL_1, + aggregation: { + threshold: 1 + } } } ]); - this.testClient = new TestClient(this.mapConfig); - this.testClient.getTile(0, 0, 0, { format: 'mvt' }, (err, res, tile) => { + this.testClient = new TestClient(mapConfig); + const options = { + response: { + status: 400 + } + }; + + this.testClient.getLayergroup(options, (err, body) => { if (err) { return done(err); } - assert.equal(tile.tileSize, 4096); - assert.equal(tile.z, 0); - assert.equal(tile.x, 0); - assert.equal(tile.y, 0); - - const layer0 = JSON.parse(tile.toGeoJSONSync(0)); - - assert.equal(layer0.name, 'layer0'); - assert.equal(layer0.features[0].type, 'Feature'); - assert.equal(layer0.features[0].geometry.type, 'Point'); + assert.deepEqual(body, { + errors: [ + 'Unsupported geometry type (ST_Polygon) for aggregation.' + + ' Aggregation is available only for points.' + ], + errors_with_context:[{ + type: 'unknown', + message: 'Unsupported geometry type (ST_Polygon) for aggregation.' + + ' Aggregation is available only for points.' + }] + }); done(); }); }); - }); }); }); From 2edcbb4724d79dcb5b0240372b148fcfb5880f5c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 11 Dec 2017 18:33:06 +0100 Subject: [PATCH 23/78] Implement aggregation queries. Implmented for placements: centroid, point-gird, point-sample. Aggregated columns not yet implemented (only count). Aggregation could be made more efficient by using quadkeys --- .../models/aggregation/aggregation-proxy.js | 11 +-- .../aggregation/aggregation-templates.js | 82 +++++++++++++++++++ .../models/aggregation/base-aggregation.js | 7 ++ .../models/aggregation/raster-aggregation.js | 16 ++-- .../models/aggregation/vector-aggregation.js | 16 ++-- .../adapter/aggregation-mapconfig-adapter.js | 2 +- 6 files changed, 114 insertions(+), 20 deletions(-) create mode 100644 lib/cartodb/models/aggregation/aggregation-templates.js diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js index 9de82887..aa52e05f 100644 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -4,8 +4,9 @@ const RASTER_AGGREGATION = 'RasterAggregation'; const VECTOR_AGGREGATION = 'VectorAggregation'; module.exports = class AggregationProxy { - constructor (mapconfig, { resolution = 256, threshold = 10e5, placement = 'centroid', columns = {}} = {}) { + constructor (mapconfig, query, { resolution = 256, threshold = 1e5, placement = 'centroid', columns = {}} = {}) { this.mapconfig = mapconfig; + this.query = query; this.resolution = resolution; this.threshold = threshold; this.placement = placement; @@ -18,10 +19,10 @@ module.exports = class AggregationProxy { switch (this._getAggregationType()) { case VECTOR_AGGREGATION: - implementation = new VectorAggregation(this.resolution, this.threshold, this.placement, this.columns); + implementation = new VectorAggregation(this.query, this.resolution, this.threshold, this.placement, this.columns); break; case RASTER_AGGREGATION: - implementation = new RasterAggregation(this.resolution, this.threshold, this.placement, this.columns); + implementation = new RasterAggregation(this.query, this.resolution, this.threshold, this.placement, this.columns); break; default: throw new Error('Unsupported aggregation type'); @@ -38,7 +39,7 @@ module.exports = class AggregationProxy { return RASTER_AGGREGATION; } - sql (options) { - return this.implementation.sql(options); + sql () { + return this.implementation.sql(); } }; diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js new file mode 100644 index 00000000..0597ed43 --- /dev/null +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -0,0 +1,82 @@ +/** + * Returns template function (function that accepts template parameters and returns a string) + */ +module.exports = (options) => { + let templateFn = aggregationQueryTemplates[options.placement]; + console.log(options); + if (!templateFn) { + throw new Error("Invalid Aggregation placement: '" + options.placement + "'"); + } + return templateFn; +}; + +// Notes: +// * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) +// * We need to filter spatially using !bbox! to make the queries efficient because the filter added by Mapnik (wrapping the query) +// is only applied after the aggregation. +// * This queries are used for rendering and the_geom is omitted in the results for better performance + +const aggregationQueryTemplates = { + + 'centroid': ctx => ` + WITH _cdb_params AS ( + SELECT + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ) + SELECT + row_number() over() AS cartodb_id, + ST_SetSRID( + ST_MakePoint( + AVG(ST_X(_cdb_query.the_geom_webmercator)), + AVG(ST_Y(_cdb_query.the_geom_webmercator)) + ), 3857 + ) AS the_geom_webmercator, + count(*) AS _cdb_feature_count + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox + GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + `, + + 'point-grid': ctx => ` + WITH _cdb_params AS ( + SELECT + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ), + _cdb_clusters AS ( + SELECT + ST_SetSRID(ST_MakePoint(AVG(ST_X(_cdb_query.the_geom_webmercator)), AVG(ST_Y(_cdb_query.the_geom_webmercator))), 3857) AS the_geom_webmercator, + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, + count(*) AS _cdb_feature_count + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE the_geom_webmercator && _cdb_params.bbox + GROUP BY _cdb_gx, _cdb_gy + ) + SELECT + ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, + _cdb_feature_count + FROM _cdb_clusters, _cdb_params + `, + + 'point-sample-': ctx => ` + WITH _cdb_params AS ( + SELECT + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ), _cdb_clusters AS ( + SELECT + MIN(cartodb_id) AS cartodb_id, + count(*) AS _cdb_feature_count + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox + GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + ) SELECT + _cdb_clusters.cartodb_id, + the_geom, the_geom_webmercator, + _cdb_feature_count + FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query on (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) + ` + + }; diff --git a/lib/cartodb/models/aggregation/base-aggregation.js b/lib/cartodb/models/aggregation/base-aggregation.js index 98c5a09c..0e54689d 100644 --- a/lib/cartodb/models/aggregation/base-aggregation.js +++ b/lib/cartodb/models/aggregation/base-aggregation.js @@ -1,4 +1,11 @@ module.exports = class BaseAggregation { + constructor(query, resolution, threshold, placement, columns) { + this.query = query; + this.resolution = resolution; + this.threshold = threshold; + this.placement = placement; + this.columns = columns; + } sql () { throw new Error('Unimplemented method'); } diff --git a/lib/cartodb/models/aggregation/raster-aggregation.js b/lib/cartodb/models/aggregation/raster-aggregation.js index f06ee380..7314b805 100644 --- a/lib/cartodb/models/aggregation/raster-aggregation.js +++ b/lib/cartodb/models/aggregation/raster-aggregation.js @@ -1,13 +1,15 @@ const BaseAggregation = require('./base-aggregation'); +const aggregationTemplate = require('./aggregation-templates'); module.exports = class RasterAggregation extends BaseAggregation { - sql (options) { - return rasterAggregationQueryTemplate({ - sourceQuery: options.sql, - res: options.resolution, - columns: options.columns + constructor () { + super(...arguments); + } + sql () { + return aggregationTemplate(this)({ + sourceQuery: this.query, + res: this.resolution, + columns: this.columns }); } }; - -const rasterAggregationQueryTemplate = ctx => `/** aggregated query (raster) **/ ${ctx.sourceQuery}`; diff --git a/lib/cartodb/models/aggregation/vector-aggregation.js b/lib/cartodb/models/aggregation/vector-aggregation.js index c1845f63..8cd4c859 100644 --- a/lib/cartodb/models/aggregation/vector-aggregation.js +++ b/lib/cartodb/models/aggregation/vector-aggregation.js @@ -1,13 +1,15 @@ const BaseAggregation = require('./base-aggregation'); +const aggregationTemplate = require('./aggregation-templates'); module.exports = class VectorAggregation extends BaseAggregation { - sql (options) { - return vectorAggregationQueryTemplate({ - sourceQuery: options.sql, - res: options.resolution, - columns: options.columns + constructor () { + super(...arguments); + } + sql () { + return aggregationTemplate(this)({ + sourceQuery: this.query, + res: this.resolution, + columns: this.columns }); } }; - -const vectorAggregationQueryTemplate = ctx => `/** aggregated query (vector) **/ ${ctx.sourceQuery}`; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index bc401542..b20a1e34 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -145,7 +145,7 @@ module.exports = class AggregationMapConfigAdapter { return reject(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); } - const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); + const aggregation = new AggregationProxy(mapConfig, layer.options.sql, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = aggregation.sql(layer.options); From 68f967e582877670371c58d29600fffdb07889c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 18:34:22 +0100 Subject: [PATCH 24/78] Extract adaptLayer method --- .../adapter/aggregation-mapconfig-adapter.js | 88 ++++++++++--------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index bc401542..25677ca3 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -118,48 +118,7 @@ module.exports = class AggregationMapConfigAdapter { const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { - return new Promise((resolve, reject) => { - if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { - return resolve({ layer, index, adapted: false }); - } - - const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? - layer.options.aggregation.threshold : - 1e5; - - const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); - - connection.query(aggregationMetadata, (err, res) => { - if (err) { - return resolve({ layer, index, adapted: false }); - } - - const estimatedFeatureCount = res.rows[0].count; - const geometryType = res.rows[0].type; - - if (estimatedFeatureCount < threshold) { - return resolve({ layer, index, adapted: false }); - } - - if (geometryType !== 'ST_Point') { - return reject(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); - } - - const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); - const sqlQueryWrap = layer.options.sql_wrap; - - let aggregationSql = aggregation.sql(layer.options); - - if (sqlQueryWrap) { - layer.options.sql_raw = aggregationSql; - aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); - } - - layer.options.sql = aggregationSql; - - return resolve({ layer, index, adapted: true }); - }); - }); + return this._adaptLayer(connection, layer, index, isVectorOnlyMapConfig, mapConfig); }); Promise.all(adaptLayerPromises) @@ -183,6 +142,51 @@ module.exports = class AggregationMapConfigAdapter { }); } + _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { + return new Promise((resolve, reject) => { + if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { + return resolve({ layer, index, adapted: false }); + } + + const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? + layer.options.aggregation.threshold : + 1e5; + + const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); + + connection.query(aggregationMetadata, (err, res) => { + if (err) { + return resolve({ layer, index, adapted: false }); + } + + const estimatedFeatureCount = res.rows[0].count; + const geometryType = res.rows[0].type; + + if (estimatedFeatureCount < threshold) { + return resolve({ layer, index, adapted: false }); + } + + if (geometryType !== 'ST_Point') { + return reject(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); + } + + const aggregation = new AggregationProxy(mapConfig, layer.options.aggregation); + const sqlQueryWrap = layer.options.sql_wrap; + + let aggregationSql = aggregation.sql(layer.options); + + if (sqlQueryWrap) { + layer.options.sql_raw = aggregationSql; + aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); + } + + layer.options.sql = aggregationSql; + + return resolve({ layer, index, adapted: true }); + }); + }); + } + _getAggregationMetadata (isVectorOnlyMapConfig, layer, adapted) { if (adapted) { if (isVectorOnlyMapConfig) { From b1f788fb5798f63a07f21ead3c60a6cf9030205d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 18:42:03 +0100 Subject: [PATCH 25/78] Remove unuseful callback --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 0a7b62d8..94092bda 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -27,13 +27,7 @@ module.exports = class AggregationMapConfigAdapter { return callback(error); } - this._adaptLayers(user, mapConfig, requestMapConfig, context, (err, requestMapConfig) => { - if (err) { - return callback(err); - } - - callback(null, requestMapConfig); - }); + this._adaptLayers(user, mapConfig, requestMapConfig, context, callback); } _hasMissingColumns (mapConfig) { From 446449bbde84ef5270c8659c2672cc30338274db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 18:47:20 +0100 Subject: [PATCH 26/78] Move variable declaration close to the place that it's used --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 94092bda..3ef4e391 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -142,10 +142,6 @@ module.exports = class AggregationMapConfigAdapter { return resolve({ layer, index, adapted: false }); } - const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? - layer.options.aggregation.threshold : - 1e5; - const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); connection.query(aggregationMetadata, (err, res) => { @@ -154,6 +150,11 @@ module.exports = class AggregationMapConfigAdapter { } const estimatedFeatureCount = res.rows[0].count; + + const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? + layer.options.aggregation.threshold : + 1e5; + const geometryType = res.rows[0].type; if (estimatedFeatureCount < threshold) { From cc68b8421242f0382a78c0f945174d30cad7ba57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:06:53 +0100 Subject: [PATCH 27/78] Extract checkLayerAggregationMetadata method --- .../adapter/aggregation-mapconfig-adapter.js | 81 +++++++++++-------- 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 3ef4e391..39900012 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -138,47 +138,64 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { return new Promise((resolve, reject) => { - if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { - return resolve({ layer, index, adapted: false }); + this._checkLayerAggregationMetadata(connection, isVectorOnlyMapConfig, layer, index, (err, shouldAdapt) => { + if (err) { + return reject(err); + } + + if (shouldAdapt) { + const aggregation = new AggregationProxy(mapConfig, layer.options.sql, layer.options.aggregation); + const sqlQueryWrap = layer.options.sql_wrap; + + let aggregationSql = aggregation.sql(layer.options); + + if (sqlQueryWrap) { + layer.options.sql_raw = aggregationSql; + aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); + } + + layer.options.sql = aggregationSql; + } + + return resolve({ layer, index, adapted: shouldAdapt }); + }); + }); + } + + _checkLayerAggregationMetadata (connection, isVectorOnlyMapConfig, layer, index, callback) { + let shouldAdapt = false; + + if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { + return callback(null, shouldAdapt); + } + + const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); + + connection.query(aggregationMetadata, (err, res) => { + if (err) { + return callback(null, shouldAdapt); } - const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); + const estimatedFeatureCount = res.rows[0].count; - connection.query(aggregationMetadata, (err, res) => { - if (err) { - return resolve({ layer, index, adapted: false }); - } + const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? + layer.options.aggregation.threshold : + 1e5; - const estimatedFeatureCount = res.rows[0].count; - const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? - layer.options.aggregation.threshold : - 1e5; + if (estimatedFeatureCount < threshold) { + return callback(null, shouldAdapt); + } - const geometryType = res.rows[0].type; + const geometryType = res.rows[0].type; - if (estimatedFeatureCount < threshold) { - return resolve({ layer, index, adapted: false }); - } + if (geometryType !== 'ST_Point') { + return callback(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); + } - if (geometryType !== 'ST_Point') { - return reject(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); - } + shouldAdapt = true; - const aggregation = new AggregationProxy(mapConfig, layer.options.sql, layer.options.aggregation); - const sqlQueryWrap = layer.options.sql_wrap; - - let aggregationSql = aggregation.sql(layer.options); - - if (sqlQueryWrap) { - layer.options.sql_raw = aggregationSql; - aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); - } - - layer.options.sql = aggregationSql; - - return resolve({ layer, index, adapted: true }); - }); + callback(null, shouldAdapt); }); } From d9477006463262de463b39cb7208aa955019ab2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:12:10 +0100 Subject: [PATCH 28/78] Get connection at the begining of adapt layers functionality --- .../adapter/aggregation-mapconfig-adapter.js | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 39900012..4f2382ae 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -27,7 +27,13 @@ module.exports = class AggregationMapConfigAdapter { return callback(error); } - this._adaptLayers(user, mapConfig, requestMapConfig, context, callback); + this.pgConnection.getConnection(user, (err, connection) => { + if (err) { + return callback(err); + } + + this._adaptLayers(connection, mapConfig, requestMapConfig, context, callback); + }); } _hasMissingColumns (mapConfig) { @@ -103,37 +109,31 @@ module.exports = class AggregationMapConfigAdapter { return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } - _adaptLayers (user, mapConfig, requestMapConfig, context, callback) { - this.pgConnection.getConnection(user, (err, connection) => { - if (err) { - return callback(err); - } + _adaptLayers (connection, mapConfig, requestMapConfig, context, callback) { + const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); - const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); - - const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { - return this._adaptLayer(connection, layer, index, isVectorOnlyMapConfig, mapConfig); - }); - - Promise.all(adaptLayerPromises) - .then(results => { - context.aggregation = { - layers: [] - }; - - results.forEach(({ layer, index, adapted }) => { - if (adapted) { - requestMapConfig.layers[index] = layer; - } - const aggregatedFormats = this._getAggregationMetadata(isVectorOnlyMapConfig, layer, adapted); - context.aggregation.layers.push(aggregatedFormats); - }); - - return requestMapConfig; - }) - .then(requestMapConfig => callback(null, requestMapConfig)) - .catch(err => callback(err)); + const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { + return this._adaptLayer(connection, layer, index, isVectorOnlyMapConfig, mapConfig); }); + + Promise.all(adaptLayerPromises) + .then(results => { + context.aggregation = { + layers: [] + }; + + results.forEach(({ layer, index, adapted }) => { + if (adapted) { + requestMapConfig.layers[index] = layer; + } + const aggregatedFormats = this._getAggregationMetadata(isVectorOnlyMapConfig, layer, adapted); + context.aggregation.layers.push(aggregatedFormats); + }); + + return requestMapConfig; + }) + .then(requestMapConfig => callback(null, requestMapConfig)) + .catch(err => callback(err)); } _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { From 8d42909eabfaa0346ccc0c7563983b5d2f214bdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:14:16 +0100 Subject: [PATCH 29/78] Change argument order to be more consistent --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 4f2382ae..144759af 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -138,7 +138,7 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { return new Promise((resolve, reject) => { - this._checkLayerAggregationMetadata(connection, isVectorOnlyMapConfig, layer, index, (err, shouldAdapt) => { + this._checkLayerAggregationMetadata(connection, layer, index, isVectorOnlyMapConfig, (err, shouldAdapt) => { if (err) { return reject(err); } @@ -162,7 +162,7 @@ module.exports = class AggregationMapConfigAdapter { }); } - _checkLayerAggregationMetadata (connection, isVectorOnlyMapConfig, layer, index, callback) { + _checkLayerAggregationMetadata (connection, layer, index, isVectorOnlyMapConfig, callback) { let shouldAdapt = false; if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { From 6725025e1ab3cfebad19f773283d77b701526119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:17:32 +0100 Subject: [PATCH 30/78] Improve naming for a method --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 144759af..227866e4 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -138,7 +138,7 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { return new Promise((resolve, reject) => { - this._checkLayerAggregationMetadata(connection, layer, index, isVectorOnlyMapConfig, (err, shouldAdapt) => { + this._shouldAdaptLayer(connection, layer, index, isVectorOnlyMapConfig, (err, shouldAdapt) => { if (err) { return reject(err); } @@ -162,7 +162,7 @@ module.exports = class AggregationMapConfigAdapter { }); } - _checkLayerAggregationMetadata (connection, layer, index, isVectorOnlyMapConfig, callback) { + _shouldAdaptLayer (connection, layer, index, isVectorOnlyMapConfig, callback) { let shouldAdapt = false; if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { From 3f075ca432f0784a6e6e3525ff434c1d3d96ed56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:18:29 +0100 Subject: [PATCH 31/78] Remove unused argument --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 227866e4..5850e16a 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -138,7 +138,7 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { return new Promise((resolve, reject) => { - this._shouldAdaptLayer(connection, layer, index, isVectorOnlyMapConfig, (err, shouldAdapt) => { + this._shouldAdaptLayer(connection, layer, isVectorOnlyMapConfig, (err, shouldAdapt) => { if (err) { return reject(err); } @@ -162,7 +162,7 @@ module.exports = class AggregationMapConfigAdapter { }); } - _shouldAdaptLayer (connection, layer, index, isVectorOnlyMapConfig, callback) { + _shouldAdaptLayer (connection, layer, isVectorOnlyMapConfig, callback) { let shouldAdapt = false; if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { @@ -182,7 +182,6 @@ module.exports = class AggregationMapConfigAdapter { layer.options.aggregation.threshold : 1e5; - if (estimatedFeatureCount < threshold) { return callback(null, shouldAdapt); } From 87c4848e1920d7d54bc152c9623d83ca3f07b016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:22:15 +0100 Subject: [PATCH 32/78] Improve namig --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 5850e16a..91fcb5c1 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -86,7 +86,7 @@ module.exports = class AggregationMapConfigAdapter { } else if (aggregation === undefined) { if (mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; - } else if (this._hasAggregation(requestMapConfig)){ + } else if (this._hasAnyLayerAggregation(requestMapConfig)){ shouldAdapt = true; } } @@ -94,7 +94,7 @@ module.exports = class AggregationMapConfigAdapter { return shouldAdapt; } - _hasAggregation (requestMapConfig) { + _hasAnyLayerAggregation (requestMapConfig) { for (const layer of requestMapConfig.layers) { if (this._hasLayerAggregation(layer)) { return true; @@ -111,7 +111,6 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayers (connection, mapConfig, requestMapConfig, context, callback) { const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); - const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { return this._adaptLayer(connection, layer, index, isVectorOnlyMapConfig, mapConfig); }); From 5bf4eba215b0a2d1a129a69d0ac5289749b79adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 11 Dec 2017 19:35:59 +0100 Subject: [PATCH 33/78] Remove unused thenable --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 91fcb5c1..b5bac9d6 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -129,9 +129,8 @@ module.exports = class AggregationMapConfigAdapter { context.aggregation.layers.push(aggregatedFormats); }); - return requestMapConfig; + callback(null, requestMapConfig); }) - .then(requestMapConfig => callback(null, requestMapConfig)) .catch(err => callback(err)); } From 06efe410ef3c7227f5745d1c0086779e325fbba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 10:43:49 +0100 Subject: [PATCH 34/78] Replace nested conditional with guard clause (early return) --- .../adapter/aggregation-mapconfig-adapter.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index b5bac9d6..ac7841fb 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -197,15 +197,15 @@ module.exports = class AggregationMapConfigAdapter { } _getAggregationMetadata (isVectorOnlyMapConfig, layer, adapted) { - if (adapted) { - if (isVectorOnlyMapConfig) { - return { png: false, mvt: true }; - } - - return { png: true, mvt: true }; + if (!adapted) { + return { png: false, mvt: false }; } - return { png: false, mvt: false }; + if (isVectorOnlyMapConfig) { + return { png: false, mvt: true }; + } + + return { png: true, mvt: true }; } _getAggregationColumns (aggregation) { From d405987a96ecf43d44ca5dc1ce79ff2a88ea6496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 10:49:05 +0100 Subject: [PATCH 35/78] Replace nested conditional with guard clause --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index ac7841fb..115632df 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -81,9 +81,15 @@ module.exports = class AggregationMapConfigAdapter { if (aggregation === 'false') { shouldAdapt = false; - } else if (aggregation === 'true') { + return shouldAdapt; + } + + if (aggregation === 'true') { shouldAdapt = true; - } else if (aggregation === undefined) { + return shouldAdapt; + } + + if (aggregation === undefined) { if (mapConfig.isVectorOnlyMapConfig()) { shouldAdapt = true; } else if (this._hasAnyLayerAggregation(requestMapConfig)){ From c637caf9c9bc0ca57bcc58063a84e35d9b7e1214 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 10:56:23 +0100 Subject: [PATCH 36/78] Replace nested conditional with guard clause --- .../adapter/aggregation-mapconfig-adapter.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 115632df..eed51dbf 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -89,12 +89,14 @@ module.exports = class AggregationMapConfigAdapter { return shouldAdapt; } - if (aggregation === undefined) { - if (mapConfig.isVectorOnlyMapConfig()) { - shouldAdapt = true; - } else if (this._hasAnyLayerAggregation(requestMapConfig)){ - shouldAdapt = true; - } + if (aggregation === undefined && mapConfig.isVectorOnlyMapConfig()) { + shouldAdapt = true; + return shouldAdapt; + } + + if (aggregation === undefined && this._hasAnyLayerAggregation(requestMapConfig)){ + shouldAdapt = true; + return shouldAdapt; } return shouldAdapt; From f52cc276bee52527c6ed0f6361c7ee019e449418 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 10:57:50 +0100 Subject: [PATCH 37/78] Remove control flag --- .../adapter/aggregation-mapconfig-adapter.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index eed51dbf..6538c050 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -77,29 +77,23 @@ module.exports = class AggregationMapConfigAdapter { _shouldAdaptLayers (mapConfig, requestMapConfig, params) { const { aggregation } = params; - let shouldAdapt = false; - if (aggregation === 'false') { - shouldAdapt = false; - return shouldAdapt; + return false; } if (aggregation === 'true') { - shouldAdapt = true; - return shouldAdapt; + return true; } if (aggregation === undefined && mapConfig.isVectorOnlyMapConfig()) { - shouldAdapt = true; - return shouldAdapt; + return true; } if (aggregation === undefined && this._hasAnyLayerAggregation(requestMapConfig)){ - shouldAdapt = true; - return shouldAdapt; + return true; } - return shouldAdapt; + return false; } _hasAnyLayerAggregation (requestMapConfig) { From 85e7245a33290b6f149c0d38f323bf32220939b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 11:07:04 +0100 Subject: [PATCH 38/78] Remove control flag --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 6538c050..1b3db044 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -38,7 +38,6 @@ module.exports = class AggregationMapConfigAdapter { _hasMissingColumns (mapConfig) { const layers = mapConfig.getLayers(); - let missingColumns = false; for (let index = 0; index < layers.length; index++) { const layer = layers[index]; @@ -51,17 +50,15 @@ module.exports = class AggregationMapConfigAdapter { } if (aggregationColumns.length === 0) { - missingColumns = true; - break; + return true; } if (!this._haveSameColumns(aggregationColumns,layerColumns)) { - missingColumns = true; - break; + return true; } } - return missingColumns; + return false; } _haveSameColumns (aggregationColumns, layerColumns) { From 2f68d658f0fbda0e2cbd17f87e6e06b3c5012ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 11:10:12 +0100 Subject: [PATCH 39/78] Remove local variable --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 1b3db044..ddb3a34e 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -40,8 +40,7 @@ module.exports = class AggregationMapConfigAdapter { const layers = mapConfig.getLayers(); for (let index = 0; index < layers.length; index++) { - const layer = layers[index]; - const { aggregation } = layer.options; + const { aggregation } = layers[index].options; const aggregationColumns = this._getAggregationColumns(aggregation); const layerColumns = mapConfig.getColumnsByLayer(index); @@ -53,7 +52,7 @@ module.exports = class AggregationMapConfigAdapter { return true; } - if (!this._haveSameColumns(aggregationColumns,layerColumns)) { + if (!this._haveSameColumns(aggregationColumns, layerColumns)) { return true; } } From b2fcbdd8a3b6f370c126ff33f1432385545d5460 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 12 Dec 2017 11:18:18 +0100 Subject: [PATCH 40/78] Implement aggregation columns --- .../aggregation/aggregation-templates.js | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index 0597ed43..6b864240 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -3,13 +3,33 @@ */ module.exports = (options) => { let templateFn = aggregationQueryTemplates[options.placement]; - console.log(options); if (!templateFn) { throw new Error("Invalid Aggregation placement: '" + options.placement + "'"); } return templateFn; }; +const aggregate_columns = ctx => { + let columns = ctx.columns || {}; + if (Object.keys(columns).length == 0) { + // default aggregation + columns = { + _cdb_feature_count: { + aggregate_function: 'count' + } + } + } + return Object.keys(columns).map(column_name => { + let aggregate_expression = columns[column_name].aggregate_expression; + if (!aggregate_expression) { + const aggregate_function = columns[column_name].aggregate_function || 'count'; + const aggregated_column = columns[column_name].aggregated_column || '*'; + aggregate_expression = `${aggregate_function}(${aggregated_column})`; + } + return `${aggregate_expression} AS ${column_name}`; + }).join(', '); +}; + // Notes: // * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) // * We need to filter spatially using !bbox! to make the queries efficient because the filter added by Mapnik (wrapping the query) @@ -32,7 +52,7 @@ const aggregationQueryTemplates = { AVG(ST_Y(_cdb_query.the_geom_webmercator)) ), 3857 ) AS the_geom_webmercator, - count(*) AS _cdb_feature_count + ${aggregate_columns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) @@ -49,7 +69,7 @@ const aggregationQueryTemplates = { ST_SetSRID(ST_MakePoint(AVG(ST_X(_cdb_query.the_geom_webmercator)), AVG(ST_Y(_cdb_query.the_geom_webmercator))), 3857) AS the_geom_webmercator, Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, - count(*) AS _cdb_feature_count + ${aggregate_columns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE the_geom_webmercator && _cdb_params.bbox GROUP BY _cdb_gx, _cdb_gy @@ -68,7 +88,7 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT MIN(cartodb_id) AS cartodb_id, - count(*) AS _cdb_feature_count + ${aggregate_columns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) From e37682403c5347cedc513e037e228265be18d229 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 12 Dec 2017 11:19:05 +0100 Subject: [PATCH 41/78] Fix test Note that the CartoCSS should reference columns of the aggregated table --- test/acceptance/aggregation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 2576a09c..804610ba 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -143,7 +143,7 @@ describe('aggregation', function () { }, threshold: 1 }, - cartocss: '#layer { marker-width: [value]; }', + cartocss: '#layer { marker-width: [total]; }', cartocss_version: '2.3.0' } } From e93fe13b41d44c93789545101328256f1d53d7a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 11:57:38 +0100 Subject: [PATCH 42/78] Get the right columns from aggregation --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index ddb3a34e..aafbc367 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -44,6 +44,8 @@ module.exports = class AggregationMapConfigAdapter { const aggregationColumns = this._getAggregationColumns(aggregation); const layerColumns = mapConfig.getColumnsByLayer(index); + console.log(aggregationColumns, layerColumns); + if (layerColumns.length === 0) { continue; } @@ -210,12 +212,11 @@ module.exports = class AggregationMapConfigAdapter { const hasAggregationColumns = aggregation !== undefined && typeof aggregation !== 'boolean' && typeof aggregation.columns === 'object'; + let aggregationColumns = []; if (hasAggregationColumns) { - aggregationColumns = Object.keys(aggregation.columns).map(key => { - return aggregation.columns[key].aggregated_column; - }); + aggregationColumns = Object.keys(aggregation.columns); } return aggregationColumns; From eceffda87f91058e8330f53b7cb729607c4dab34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 12:01:25 +0100 Subject: [PATCH 43/78] Do not use control flag --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index aafbc367..27f4b613 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -213,12 +213,6 @@ module.exports = class AggregationMapConfigAdapter { typeof aggregation !== 'boolean' && typeof aggregation.columns === 'object'; - let aggregationColumns = []; - - if (hasAggregationColumns) { - aggregationColumns = Object.keys(aggregation.columns); - } - - return aggregationColumns; + return hasAggregationColumns ? Object.keys(aggregation.columns) : []; } }; From faaebaa07d6157a78cc6c9f3393dd4485095ab20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 12:02:10 +0100 Subject: [PATCH 44/78] Remove console.log --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 27f4b613..e661042c 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -44,8 +44,6 @@ module.exports = class AggregationMapConfigAdapter { const aggregationColumns = this._getAggregationColumns(aggregation); const layerColumns = mapConfig.getColumnsByLayer(index); - console.log(aggregationColumns, layerColumns); - if (layerColumns.length === 0) { continue; } From 5bc1903677edae6e5c8edcfb8eb5aed2e41f5c8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 12:15:13 +0100 Subject: [PATCH 45/78] Add test to check if cartoccs and aggregation definition are fully compatible --- test/acceptance/aggregation.js | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 804610ba..ef06b1bd 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -165,6 +165,46 @@ describe('aggregation', function () { }); }); + it('should fail when aggregation and cartocss are not compatible', function (done) { + const response = { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + columns: { + total: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + }, + threshold: 1 + }, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0' + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + this.testClient.getLayergroup({ response }, (err, body) => { + if (err) { + return done(err); + } + + assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + + done(); + }); + }); + it('should fail if cartocss uses "value" column and it\'s not defined in the aggregation', function (done) { const response = { From 869f2ac3227b270371517746f641d083f7b19cde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 12:39:12 +0100 Subject: [PATCH 46/78] Improve error message --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 2 +- test/acceptance/aggregation.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index e661042c..4217b2cf 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -5,7 +5,7 @@ const queryUtils = require('../../../utils/query-utils'); const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ ' interactivity fields or attributes that are not present in the aggregation'; const unsupportedGeometryTypeErrorMessage = ctx => -`Unsupported geometry type (${ctx.geometryType}) for aggregation. Aggregation is available only for points.`; +`Unsupported geometry type: ${ctx.geometryType}. Aggregation is available only for geometry type: ST_Point`; module.exports = class AggregationMapConfigAdapter { constructor (pgConnection) { diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index ef06b1bd..9c1fed45 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -379,13 +379,13 @@ describe('aggregation', function () { assert.deepEqual(body, { errors: [ - 'Unsupported geometry type (ST_Polygon) for aggregation.' + - ' Aggregation is available only for points.' + 'Unsupported geometry type: ST_Polygon.' + + ' Aggregation is available only for geometry type: ST_Point' ], errors_with_context:[{ type: 'unknown', - message: 'Unsupported geometry type (ST_Polygon) for aggregation.' + - ' Aggregation is available only for points.' + message: 'Unsupported geometry type: ST_Polygon.' + + ' Aggregation is available only for geometry type: ST_Point' }] }); From 0b27d174ef1748641712e9417a51db2e48f0c9a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 12:53:29 +0100 Subject: [PATCH 47/78] Check if query retrieves results --- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 4217b2cf..c067d17f 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -172,7 +172,8 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, shouldAdapt); } - const estimatedFeatureCount = res.rows[0].count; + const result = res.rows[0] || {}; + const estimatedFeatureCount = result.count; const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? layer.options.aggregation.threshold : @@ -182,7 +183,7 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, shouldAdapt); } - const geometryType = res.rows[0].type; + const geometryType = result.type; if (geometryType !== 'ST_Point') { return callback(new Error(unsupportedGeometryTypeErrorMessage({ geometryType }))); From e26cfb2efb96f3fd912277150b5e89e51dbfabbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 13:32:01 +0100 Subject: [PATCH 48/78] Remove magic number --- lib/cartodb/models/aggregation/aggregation-proxy.js | 6 +++++- .../mapconfig/adapter/aggregation-mapconfig-adapter.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js index aa52e05f..58da647c 100644 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -4,7 +4,11 @@ const RASTER_AGGREGATION = 'RasterAggregation'; const VECTOR_AGGREGATION = 'VectorAggregation'; module.exports = class AggregationProxy { - constructor (mapconfig, query, { resolution = 256, threshold = 1e5, placement = 'centroid', columns = {}} = {}) { + static get THRESHOLD() { + return 1e5; // 100K + } + + constructor (mapconfig, query, { resolution = 256, threshold = AggregationProxy.THRESHOLD, placement = 'centroid', columns = {}} = {}) { this.mapconfig = mapconfig; this.query = query; this.resolution = resolution; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index c067d17f..7c315f86 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -177,7 +177,7 @@ module.exports = class AggregationMapConfigAdapter { const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? layer.options.aggregation.threshold : - 1e5; + AggregationProxy.THRESHOLD; if (estimatedFeatureCount < threshold) { return callback(null, shouldAdapt); From 3b7db0b08f3d65d07c2deb9b98967c7f438b5612 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 12 Dec 2017 15:48:25 +0100 Subject: [PATCH 49/78] Fix typo --- lib/cartodb/models/aggregation/aggregation-templates.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index 6b864240..516f0d78 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -80,7 +80,7 @@ const aggregationQueryTemplates = { FROM _cdb_clusters, _cdb_params `, - 'point-sample-': ctx => ` + 'point-sample': ctx => ` WITH _cdb_params AS ( SELECT (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, From d4d32bdfa3cca39fc8981497f77f0c4ce0accba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 15:53:35 +0100 Subject: [PATCH 50/78] Make jshint more happy --- .../models/aggregation/aggregation-proxy.js | 23 ++- .../aggregation/aggregation-templates.js | 141 ++++++++++-------- 2 files changed, 96 insertions(+), 68 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js index 58da647c..d5c64c18 100644 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ b/lib/cartodb/models/aggregation/aggregation-proxy.js @@ -8,7 +8,12 @@ module.exports = class AggregationProxy { return 1e5; // 100K } - constructor (mapconfig, query, { resolution = 256, threshold = AggregationProxy.THRESHOLD, placement = 'centroid', columns = {}} = {}) { + constructor (mapconfig, query, { + resolution = 256, + threshold = AggregationProxy.THRESHOLD, + placement = 'centroid', + columns = {} + } = {}) { this.mapconfig = mapconfig; this.query = query; this.resolution = resolution; @@ -23,10 +28,22 @@ module.exports = class AggregationProxy { switch (this._getAggregationType()) { case VECTOR_AGGREGATION: - implementation = new VectorAggregation(this.query, this.resolution, this.threshold, this.placement, this.columns); + implementation = new VectorAggregation( + this.query, + this.resolution, + this.threshold, + this.placement, + this.columns + ); break; case RASTER_AGGREGATION: - implementation = new RasterAggregation(this.query, this.resolution, this.threshold, this.placement, this.columns); + implementation = new RasterAggregation( + this.query, + this.resolution, + this.threshold, + this.placement, + this.columns + ); break; default: throw new Error('Unsupported aggregation type'); diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index 6b864240..40e63650 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -11,13 +11,13 @@ module.exports = (options) => { const aggregate_columns = ctx => { let columns = ctx.columns || {}; - if (Object.keys(columns).length == 0) { + if (Object.keys(columns).length === 0) { // default aggregation columns = { _cdb_feature_count: { aggregate_function: 'count' } - } + }; } return Object.keys(columns).map(column_name => { let aggregate_expression = columns[column_name].aggregate_expression; @@ -31,72 +31,83 @@ const aggregate_columns = ctx => { }; // Notes: -// * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) -// * We need to filter spatially using !bbox! to make the queries efficient because the filter added by Mapnik (wrapping the query) -// is only applied after the aggregation. +// * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to +// * ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) +// * We need to filter spatially using !bbox! to make the queries efficient because the filter added by Mapnik +// * (wrapping the query) is only applied after the aggregation. // * This queries are used for rendering and the_geom is omitted in the results for better performance const aggregationQueryTemplates = { - - 'centroid': ctx => ` - WITH _cdb_params AS ( - SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, - !bbox! AS bbox - ) + 'centroid': ctx => ` + WITH _cdb_params AS ( SELECT - row_number() over() AS cartodb_id, - ST_SetSRID( - ST_MakePoint( - AVG(ST_X(_cdb_query.the_geom_webmercator)), - AVG(ST_Y(_cdb_query.the_geom_webmercator)) - ), 3857 - ) AS the_geom_webmercator, - ${aggregate_columns(ctx)} + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ) + SELECT + row_number() over() AS cartodb_id, + ST_SetSRID( + ST_MakePoint( + AVG(ST_X(_cdb_query.the_geom_webmercator)), + AVG(ST_Y(_cdb_query.the_geom_webmercator)) + ), 3857 + ) AS the_geom_webmercator, + ${aggregate_columns(ctx)} + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox + GROUP BY + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + `, + + 'point-grid': ctx => ` + WITH _cdb_params AS ( + SELECT + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ), + _cdb_clusters AS ( + SELECT + ST_SetSRID( + ST_MakePoint( + AVG(ST_X(_cdb_query.the_geom_webmercator)), + AVG(ST_Y(_cdb_query.the_geom_webmercator)) + ), + 3857 + ) AS the_geom_webmercator, + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, + ${aggregate_columns(ctx)} + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE the_geom_webmercator && _cdb_params.bbox + GROUP BY _cdb_gx, _cdb_gy + ) + SELECT + ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, + _cdb_feature_count + FROM _cdb_clusters, _cdb_params + `, + + 'point-sample-': ctx => ` + WITH _cdb_params AS ( + SELECT + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ), _cdb_clusters AS ( + SELECT + MIN(cartodb_id) AS cartodb_id, + ${aggregate_columns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox - GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) - `, - - 'point-grid': ctx => ` - WITH _cdb_params AS ( - SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, - !bbox! AS bbox - ), - _cdb_clusters AS ( - SELECT - ST_SetSRID(ST_MakePoint(AVG(ST_X(_cdb_query.the_geom_webmercator)), AVG(ST_Y(_cdb_query.the_geom_webmercator))), 3857) AS the_geom_webmercator, - Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, - Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, - ${aggregate_columns(ctx)} - FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params - WHERE the_geom_webmercator && _cdb_params.bbox - GROUP BY _cdb_gx, _cdb_gy - ) - SELECT - ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, - _cdb_feature_count - FROM _cdb_clusters, _cdb_params - `, - - 'point-sample-': ctx => ` - WITH _cdb_params AS ( - SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, - !bbox! AS bbox - ), _cdb_clusters AS ( - SELECT - MIN(cartodb_id) AS cartodb_id, - ${aggregate_columns(ctx)} - FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params - WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox - GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) - ) SELECT - _cdb_clusters.cartodb_id, - the_geom, the_geom_webmercator, - _cdb_feature_count - FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query on (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) - ` - - }; + GROUP BY + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + ) SELECT + _cdb_clusters.cartodb_id, + the_geom, the_geom_webmercator, + _cdb_feature_count + FROM + _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query + ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) + ` +}; From ae35acd21dd43575ca32f272b7eb5b3e90b446f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 15:54:36 +0100 Subject: [PATCH 51/78] typo --- lib/cartodb/models/aggregation/aggregation-templates.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index 40e63650..0b3e9e3d 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -88,7 +88,7 @@ const aggregationQueryTemplates = { FROM _cdb_clusters, _cdb_params `, - 'point-sample-': ctx => ` + 'point-sample': ctx => ` WITH _cdb_params AS ( SELECT (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, From 4405d618459ebef1e8036b07202ebdf04ca9d23e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 12 Dec 2017 16:17:42 +0100 Subject: [PATCH 52/78] Remove support for arbitrary aggregation SQL expressions. Only the supported aggregate functions can be used now, currently count, sum, avg, min & max. --- .../aggregation/aggregation-templates.js | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index d7e92f04..109d0275 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -9,7 +9,25 @@ module.exports = (options) => { return templateFn; }; -const aggregate_columns = ctx => { +const SUPPORTED_AGGREGATE_FUNCTIONS = { + 'count': { + sql: (column_name, params) => `count(${params.aggregated_column || '*'})` + }, + 'avg': { + sql: (column_name, params) => `avg(${params.aggregated_column || column_name})` + }, + 'sum': { + sql: (column_name, params) => `sum(${params.aggregated_column || column_name})` + }, + 'min': { + sql: (column_name, params) => `min(${params.aggregated_column || column_name})` + }, + 'max': { + sql: (column_name, params) => `max(${params.aggregated_column || column_name})` + } +}; + +const aggregateColumns = ctx => { let columns = ctx.columns || {}; if (Object.keys(columns).length === 0) { // default aggregation @@ -20,21 +38,22 @@ const aggregate_columns = ctx => { }; } return Object.keys(columns).map(column_name => { - let aggregate_expression = columns[column_name].aggregate_expression; - if (!aggregate_expression) { - const aggregate_function = columns[column_name].aggregate_function || 'count'; - const aggregated_column = columns[column_name].aggregated_column || '*'; - aggregate_expression = `${aggregate_function}(${aggregated_column})`; + const aggregate_function = columns[column_name].aggregate_function || 'count'; + const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; + if (!aggregate_definition) { + throw new Error("Invalid Aggregate function: '" + aggregate_function + "'"); } + const aggregate_expression = aggregate_definition.sql(column_name, columns[column_name]); return `${aggregate_expression} AS ${column_name}`; }).join(', '); }; // Notes: // * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to -// * ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) -// * We need to filter spatially using !bbox! to make the queries efficient because the filter added by Mapnik -// * (wrapping the query) is only applied after the aggregation. +// ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) +// * We need to filter spatially using !bbox! to make the queries efficient because +// the filter added by Mapnik (wrapping the query) +// is only applied after the aggregation. // * This queries are used for rendering and the_geom is omitted in the results for better performance const aggregationQueryTemplates = { @@ -52,7 +71,7 @@ const aggregationQueryTemplates = { AVG(ST_Y(_cdb_query.the_geom_webmercator)) ), 3857 ) AS the_geom_webmercator, - ${aggregate_columns(ctx)} + ${aggregateColumns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY @@ -77,7 +96,7 @@ const aggregationQueryTemplates = { ) AS the_geom_webmercator, Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, - ${aggregate_columns(ctx)} + ${aggregateColumns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE the_geom_webmercator && _cdb_params.bbox GROUP BY _cdb_gx, _cdb_gy @@ -97,7 +116,7 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT MIN(cartodb_id) AS cartodb_id, - ${aggregate_columns(ctx)} + ${aggregateColumns(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY From 4193f96c03ca08d2652e5346f7d0fff387c84d4e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 12 Dec 2017 17:38:39 +0100 Subject: [PATCH 53/78] Fix point-grid aggregation query --- .../aggregation/aggregation-templates.js | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-templates.js index 109d0275..eb7d6019 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-templates.js @@ -80,31 +80,24 @@ const aggregationQueryTemplates = { `, 'point-grid': ctx => ` - WITH _cdb_params AS ( - SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, - !bbox! AS bbox - ), - _cdb_clusters AS ( - SELECT - ST_SetSRID( - ST_MakePoint( - AVG(ST_X(_cdb_query.the_geom_webmercator)), - AVG(ST_Y(_cdb_query.the_geom_webmercator)) - ), - 3857 - ) AS the_geom_webmercator, - Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, - Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, - ${aggregateColumns(ctx)} - FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params - WHERE the_geom_webmercator && _cdb_params.bbox - GROUP BY _cdb_gx, _cdb_gy - ) + WITH _cdb_params AS ( SELECT - ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, - _cdb_feature_count - FROM _cdb_clusters, _cdb_params + (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + !bbox! AS bbox + ), + _cdb_clusters AS ( + SELECT + Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, + ${aggregateColumns(ctx)} + FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + WHERE the_geom_webmercator && _cdb_params.bbox + GROUP BY _cdb_gx, _cdb_gy + ) + SELECT + ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, + _cdb_feature_count + FROM _cdb_clusters, _cdb_params `, 'point-sample': ctx => ` From f390a108304e5d843443a969436869efb0d2c383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 17:58:42 +0100 Subject: [PATCH 54/78] Remove methods that check map-config aggregation and use the ones that MapConfig model provides --- .../adapter/aggregation-mapconfig-adapter.js | 42 +++++-------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 7c315f86..440b21a2 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -15,7 +15,7 @@ module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { const mapConfig = new MapConfig(requestMapConfig); - if (!this._shouldAdaptLayers(mapConfig, requestMapConfig, params)) { + if (!this._shouldAdapt(mapConfig, params)) { return callback(null, requestMapConfig); } @@ -70,7 +70,7 @@ module.exports = class AggregationMapConfigAdapter { return !diff.length; } - _shouldAdaptLayers (mapConfig, requestMapConfig, params) { + _shouldAdapt (mapConfig, params) { const { aggregation } = params; if (aggregation === 'false') { @@ -81,36 +81,16 @@ module.exports = class AggregationMapConfigAdapter { return true; } - if (aggregation === undefined && mapConfig.isVectorOnlyMapConfig()) { - return true; - } - - if (aggregation === undefined && this._hasAnyLayerAggregation(requestMapConfig)){ + if (mapConfig.isAggregationMapConfig()) { return true; } return false; } - _hasAnyLayerAggregation (requestMapConfig) { - for (const layer of requestMapConfig.layers) { - if (this._hasLayerAggregation(layer)) { - return true; - } - } - - return false; - } - - _hasLayerAggregation (layer) { - const { aggregation } = layer.options; - return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); - } - _adaptLayers (connection, mapConfig, requestMapConfig, context, callback) { - const isVectorOnlyMapConfig = mapConfig.isVectorOnlyMapConfig(); const adaptLayerPromises = requestMapConfig.layers.map((layer, index) => { - return this._adaptLayer(connection, layer, index, isVectorOnlyMapConfig, mapConfig); + return this._adaptLayer(connection, mapConfig, layer, index); }); Promise.all(adaptLayerPromises) @@ -123,7 +103,7 @@ module.exports = class AggregationMapConfigAdapter { if (adapted) { requestMapConfig.layers[index] = layer; } - const aggregatedFormats = this._getAggregationMetadata(isVectorOnlyMapConfig, layer, adapted); + const aggregatedFormats = this._getAggregationMetadata(mapConfig, layer, adapted); context.aggregation.layers.push(aggregatedFormats); }); @@ -132,9 +112,9 @@ module.exports = class AggregationMapConfigAdapter { .catch(err => callback(err)); } - _adaptLayer (connection, layer, index, isVectorOnlyMapConfig, mapConfig) { + _adaptLayer (connection, mapConfig, layer, index) { return new Promise((resolve, reject) => { - this._shouldAdaptLayer(connection, layer, isVectorOnlyMapConfig, (err, shouldAdapt) => { + this._shouldAdaptLayer(connection, mapConfig, layer, index, (err, shouldAdapt) => { if (err) { return reject(err); } @@ -158,10 +138,10 @@ module.exports = class AggregationMapConfigAdapter { }); } - _shouldAdaptLayer (connection, layer, isVectorOnlyMapConfig, callback) { + _shouldAdaptLayer (connection, mapConfig, layer, index, callback) { let shouldAdapt = false; - if (!isVectorOnlyMapConfig && !this._hasLayerAggregation(layer)) { + if (!mapConfig.isAggregationLayer(index)) { return callback(null, shouldAdapt); } @@ -195,12 +175,12 @@ module.exports = class AggregationMapConfigAdapter { }); } - _getAggregationMetadata (isVectorOnlyMapConfig, layer, adapted) { + _getAggregationMetadata (mapConfig, layer, adapted) { if (!adapted) { return { png: false, mvt: false }; } - if (isVectorOnlyMapConfig) { + if (mapConfig.isVectorOnlyMapConfig()) { return { png: false, mvt: true }; } From 6d46a21005a73f8cbb067f349a5197fc306fadf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 19:23:21 +0100 Subject: [PATCH 55/78] Validate aggregation query param --- .../adapter/aggregation-mapconfig-adapter.js | 18 +++++-- test/acceptance/aggregation.js | 49 +++++++++++++++++-- test/support/test-client.js | 4 +- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 440b21a2..8df069c0 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -7,6 +7,9 @@ const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map const unsupportedGeometryTypeErrorMessage = ctx => `Unsupported geometry type: ${ctx.geometryType}. Aggregation is available only for geometry type: ST_Point`; +const invalidAggregationParamValueErrorMessage = ctx => +`Invalid value for 'aggregation' query param: ${ctx.value}. Valid ones are 'true' or 'false'`; + module.exports = class AggregationMapConfigAdapter { constructor (pgConnection) { this.pgConnection = pgConnection; @@ -15,6 +18,10 @@ module.exports = class AggregationMapConfigAdapter { getMapConfig (user, requestMapConfig, params, context, callback) { const mapConfig = new MapConfig(requestMapConfig); + if (!this._isValidAggregationParam(params)) { + return callback(new Error(invalidAggregationParamValueErrorMessage({ value: params.aggregation }))); + } + if (!this._shouldAdapt(mapConfig, params)) { return callback(null, requestMapConfig); } @@ -36,6 +43,11 @@ module.exports = class AggregationMapConfigAdapter { }); } + _isValidAggregationParam (params) { + const { aggregation } = params; + return aggregation === undefined || aggregation === 'true' || aggregation === 'false'; + } + _hasMissingColumns (mapConfig) { const layers = mapConfig.getLayers(); @@ -77,11 +89,7 @@ module.exports = class AggregationMapConfigAdapter { return false; } - if (aggregation === 'true') { - return true; - } - - if (mapConfig.isAggregationMapConfig()) { + if (aggregation === 'true' || mapConfig.isAggregationMapConfig()) { return true; } diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 9c1fed45..56815ccc 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -291,7 +291,8 @@ describe('aggregation', function () { aggregate_function: 'sum', aggregated_column: 'value' } - } + }, + threshold: 1 } } } @@ -308,7 +309,49 @@ describe('aggregation', function () { assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); - body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation === undefined)); + body.metadata.layers.forEach(layer => assert.equal(layer.meta.aggregation, undefined)); + + done(); + }); + }); + + it('when the aggregation param is not valid should respond with error', function (done) { + const mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + threshold: 1 + } + } + } + ]); + + this.testClient = new TestClient(mapConfig); + const options = { + response: { + status: 400 + }, + aggregation: 'wadus' + }; + + this.testClient.getLayergroup(options, (err, body) => { + if (err) { + return done(err); + } + + assert.deepEqual(body, { + errors: [ + "Invalid value for 'aggregation' query param: wadus." + + " Valid ones are 'true' or 'false'" + ], + errors_with_context:[{ + type: 'unknown', + message: "Invalid value for 'aggregation' query param: wadus." + + " Valid ones are 'true' or 'false'" + }] + }); done(); }); @@ -352,7 +395,7 @@ describe('aggregation', function () { }); }); - it('when the layer\'s geometry type is not point should responds with error', function (done) { + it('when the layer\'s geometry type is not point should respond with error', function (done) { const mapConfig = createVectorMapConfig([ { type: 'cartodb', diff --git a/test/support/test-client.js b/test/support/test-client.js index 54581f99..f4437a39 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -625,7 +625,7 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) { queryParams.api_key = self.apiKey; } - if (typeof params.aggregation === 'boolean') { + if (params.aggregation !== undefined) { queryParams.aggregation = params.aggregation; } @@ -801,7 +801,7 @@ TestClient.prototype.getLayergroup = function (params, callback) { queryParams.api_key = self.apiKey; } - if (typeof params.aggregation === 'boolean') { + if (params.aggregation !== undefined) { queryParams.aggregation = params.aggregation; } From aa43eb8953a8ab833dad60ad7e54e52fdd1b5865 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 12 Dec 2017 20:10:42 +0100 Subject: [PATCH 56/78] Remove aggregation validation and use MapConfig validation --- .../adapter/aggregation-mapconfig-adapter.js | 59 +++---------------- 1 file changed, 8 insertions(+), 51 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 8df069c0..fdd70bcf 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -2,8 +2,6 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); const { MapConfig } = require('windshaft').model; const queryUtils = require('../../../utils/query-utils'); -const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ -' interactivity fields or attributes that are not present in the aggregation'; const unsupportedGeometryTypeErrorMessage = ctx => `Unsupported geometry type: ${ctx.geometryType}. Aggregation is available only for geometry type: ST_Point`; @@ -16,24 +14,25 @@ module.exports = class AggregationMapConfigAdapter { } getMapConfig (user, requestMapConfig, params, context, callback) { - const mapConfig = new MapConfig(requestMapConfig); - if (!this._isValidAggregationParam(params)) { return callback(new Error(invalidAggregationParamValueErrorMessage({ value: params.aggregation }))); } - if (!this._shouldAdapt(mapConfig, params)) { - return callback(null, requestMapConfig); - } + const mapConfig = new MapConfig(requestMapConfig); - if (this._hasMissingColumns(mapConfig)) { - const error = new Error(MISSING_AGGREGATION_COLUMNS); + try { + mapConfig.validateAggregation(); + } catch (error) { error.http_status = 400; error.type = 'mapconfig'; return callback(error); } + if (!this._shouldAdapt(mapConfig, params)) { + return callback(null, requestMapConfig); + } + this.pgConnection.getConnection(user, (err, connection) => { if (err) { return callback(err); @@ -48,40 +47,6 @@ module.exports = class AggregationMapConfigAdapter { return aggregation === undefined || aggregation === 'true' || aggregation === 'false'; } - _hasMissingColumns (mapConfig) { - const layers = mapConfig.getLayers(); - - for (let index = 0; index < layers.length; index++) { - const { aggregation } = layers[index].options; - const aggregationColumns = this._getAggregationColumns(aggregation); - const layerColumns = mapConfig.getColumnsByLayer(index); - - if (layerColumns.length === 0) { - continue; - } - - if (aggregationColumns.length === 0) { - return true; - } - - if (!this._haveSameColumns(aggregationColumns, layerColumns)) { - return true; - } - } - - return false; - } - - _haveSameColumns (aggregationColumns, layerColumns) { - if (aggregationColumns.length !== layerColumns.length) { - return false; - } - - const diff = aggregationColumns.filter(column => !layerColumns.includes(column)); - - return !diff.length; - } - _shouldAdapt (mapConfig, params) { const { aggregation } = params; @@ -194,12 +159,4 @@ module.exports = class AggregationMapConfigAdapter { return { png: true, mvt: true }; } - - _getAggregationColumns (aggregation) { - const hasAggregationColumns = aggregation !== undefined && - typeof aggregation !== 'boolean' && - typeof aggregation.columns === 'object'; - - return hasAggregationColumns ? Object.keys(aggregation.columns) : []; - } }; From 6fe73862f3af761704cc578d27abe8ed2d1107be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 11:42:51 +0100 Subject: [PATCH 57/78] Create a MapConfig's subclass to delegate aggregation --- .../aggregation/aggregation-map-config.js | 89 +++++++++++++++++++ .../adapter/aggregation-mapconfig-adapter.js | 6 +- .../ported/multilayer_error_cases.js | 2 +- 3 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 lib/cartodb/models/aggregation/aggregation-map-config.js diff --git a/lib/cartodb/models/aggregation/aggregation-map-config.js b/lib/cartodb/models/aggregation/aggregation-map-config.js new file mode 100644 index 00000000..dfc52f60 --- /dev/null +++ b/lib/cartodb/models/aggregation/aggregation-map-config.js @@ -0,0 +1,89 @@ +const MapConfig = require('windshaft').model.MapConfig; +const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ +' interactivity fields or attributes that are not present in the aggregation'; + + +module.exports = class AggregationMapConfig extends MapConfig { + constructor (config, datasource) { + super(config, datasource); + + if (this._hasAggregationMissingColumns()) { + throw new Error(MISSING_AGGREGATION_COLUMNS); + } + } + + isAggregationMapConfig () { + return this.isVectorOnlyMapConfig() || this.hasAnyLayerAggregation(); + } + + isAggregationLayer (index) { + return this.isVectorOnlyMapConfig() || this.hasLayerAggregation(index); + } + + hasAnyLayerAggregation () { + const layers = this.getLayers(); + + for (let index = 0; index < layers.length; index++) { + if (this.hasLayerAggregation(index)) { + return true; + } + } + + return false; + } + + hasLayerAggregation (index) { + const layer = this.getLayer(index); + const { aggregation } = layer.options; + + return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); + } + + validateAggregation () { + if (this._hasAggregationMissingColumns()) { + throw new Error(MISSING_AGGREGATION_COLUMNS); + } + } + + _hasAggregationMissingColumns () { + const layers = this.getLayers(); + + if (!this.isAggregationMapConfig()) { + return false; + } + + for (let index = 0; index < layers.length; index++) { + const aggregationColumns = this._getAggregationColumnsByLayer(index); + const layerColumns = this.getColumnsByLayer(index); + + if (layerColumns.length === 0) { + continue; + } + + if (aggregationColumns.length !== layerColumns.length) { + return true; + } + + const missingColumns = this._getMissingColumns(aggregationColumns, layerColumns); + + if (missingColumns.length > 0) { + return true; + } + } + + return false; + } + + _getMissingColumns (aggregationColumns, layerColumns) { + return aggregationColumns.filter(column => !layerColumns.includes(column)); + } + + _getAggregationColumnsByLayer (index) { + const { aggregation } = this.getLayer(index).options; + const hasAggregationColumns = aggregation !== undefined && + typeof aggregation !== 'boolean' && + typeof aggregation.columns === 'object'; + + return hasAggregationColumns ? Object.keys(aggregation.columns) : []; + } +}; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index fdd70bcf..2f738842 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,5 +1,5 @@ const AggregationProxy = require('../../aggregation/aggregation-proxy'); -const { MapConfig } = require('windshaft').model; +const AggregationMapConfig = require('../../aggregation/aggregation-map-config'); const queryUtils = require('../../../utils/query-utils'); const unsupportedGeometryTypeErrorMessage = ctx => @@ -18,10 +18,10 @@ module.exports = class AggregationMapConfigAdapter { return callback(new Error(invalidAggregationParamValueErrorMessage({ value: params.aggregation }))); } - const mapConfig = new MapConfig(requestMapConfig); + let mapConfig; try { - mapConfig.validateAggregation(); + mapConfig = new AggregationMapConfig(requestMapConfig); } catch (error) { error.http_status = 400; error.type = 'mapconfig'; diff --git a/test/acceptance/ported/multilayer_error_cases.js b/test/acceptance/ported/multilayer_error_cases.js index 6e7f5964..936446f2 100644 --- a/test/acceptance/ported/multilayer_error_cases.js +++ b/test/acceptance/ported/multilayer_error_cases.js @@ -57,7 +57,7 @@ describe('multilayer error cases', function() { res.body, '/**/ typeof test === \'function\' && ' + 'test({"errors":["Missing layers array from layergroup config"],' + - '"errors_with_context":[{"type":"unknown",' + + '"errors_with_context":[{"type":"mapconfig",' + '"message":"Missing layers array from layergroup config"}]});' ); done(); From 4a63fed943b76518fca84f8f4e0bd7970f636088 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 13 Dec 2017 12:35:17 +0100 Subject: [PATCH 58/78] Simplify Aggregation classes We're using the same aggregation queries for the Raster and Vector cases, so we don't need the class hierarchies used to handled them differently. AggregationProxy has been renamed to Aggregation --- .../models/aggregation/aggregation-proxy.js | 66 ------------------- ...tion-templates.js => aggregation-query.js} | 26 +++++++- lib/cartodb/models/aggregation/aggregation.js | 24 +++++++ .../models/aggregation/base-aggregation.js | 12 ---- .../models/aggregation/raster-aggregation.js | 15 ----- .../models/aggregation/vector-aggregation.js | 15 ----- .../adapter/aggregation-mapconfig-adapter.js | 6 +- 7 files changed, 51 insertions(+), 113 deletions(-) delete mode 100644 lib/cartodb/models/aggregation/aggregation-proxy.js rename lib/cartodb/models/aggregation/{aggregation-templates.js => aggregation-query.js} (87%) create mode 100644 lib/cartodb/models/aggregation/aggregation.js delete mode 100644 lib/cartodb/models/aggregation/base-aggregation.js delete mode 100644 lib/cartodb/models/aggregation/raster-aggregation.js delete mode 100644 lib/cartodb/models/aggregation/vector-aggregation.js diff --git a/lib/cartodb/models/aggregation/aggregation-proxy.js b/lib/cartodb/models/aggregation/aggregation-proxy.js deleted file mode 100644 index d5c64c18..00000000 --- a/lib/cartodb/models/aggregation/aggregation-proxy.js +++ /dev/null @@ -1,66 +0,0 @@ -const RasterAggregation = require('./raster-aggregation'); -const VectorAggregation = require('./vector-aggregation'); -const RASTER_AGGREGATION = 'RasterAggregation'; -const VECTOR_AGGREGATION = 'VectorAggregation'; - -module.exports = class AggregationProxy { - static get THRESHOLD() { - return 1e5; // 100K - } - - constructor (mapconfig, query, { - resolution = 256, - threshold = AggregationProxy.THRESHOLD, - placement = 'centroid', - columns = {} - } = {}) { - this.mapconfig = mapconfig; - this.query = query; - this.resolution = resolution; - this.threshold = threshold; - this.placement = placement; - this.columns = columns; - this.implementation = this._getAggregationImplementation(); - } - - _getAggregationImplementation () { - let implementation = null; - - switch (this._getAggregationType()) { - case VECTOR_AGGREGATION: - implementation = new VectorAggregation( - this.query, - this.resolution, - this.threshold, - this.placement, - this.columns - ); - break; - case RASTER_AGGREGATION: - implementation = new RasterAggregation( - this.query, - this.resolution, - this.threshold, - this.placement, - this.columns - ); - break; - default: - throw new Error('Unsupported aggregation type'); - } - - return implementation; - } - - _getAggregationType () { - if (this.mapconfig.isVectorOnlyMapConfig()) { - return VECTOR_AGGREGATION; - } - - return RASTER_AGGREGATION; - } - - sql () { - return this.implementation.sql(); - } -}; diff --git a/lib/cartodb/models/aggregation/aggregation-templates.js b/lib/cartodb/models/aggregation/aggregation-query.js similarity index 87% rename from lib/cartodb/models/aggregation/aggregation-templates.js rename to lib/cartodb/models/aggregation/aggregation-query.js index eb7d6019..7dd55b3c 100644 --- a/lib/cartodb/models/aggregation/aggregation-templates.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -1,7 +1,14 @@ /** - * Returns template function (function that accepts template parameters and returns a string) + * Returns a template function (function that accepts template parameters and returns a string) + * to generate an aggregation query. + * Valid options to define the query template are: + * - placement + * The query template parameters taken by the result template function are: + * - sourceQuery + * - res + * - columns */ -module.exports = (options) => { +const templateForOptions = (options) => { let templateFn = aggregationQueryTemplates[options.placement]; if (!templateFn) { throw new Error("Invalid Aggregation placement: '" + options.placement + "'"); @@ -9,6 +16,21 @@ module.exports = (options) => { return templateFn; }; +/** + * Generates an aggregation query given the aggregation options: + * - query + * - resolution + * - columns + * - placement + */ +const queryForOptions = (options) => templateForOptions(options)({ + sourceQuery: options.query, + res: options.resolution, + columns: options.columns +}); + +module.exports = queryForOptions; + const SUPPORTED_AGGREGATE_FUNCTIONS = { 'count': { sql: (column_name, params) => `count(${params.aggregated_column || '*'})` diff --git a/lib/cartodb/models/aggregation/aggregation.js b/lib/cartodb/models/aggregation/aggregation.js new file mode 100644 index 00000000..447740b4 --- /dev/null +++ b/lib/cartodb/models/aggregation/aggregation.js @@ -0,0 +1,24 @@ +const aggregationQuery = require('./aggregation-query'); + +module.exports = class Aggregation { + static get THRESHOLD() { + return 1e5; // 100K + } + + constructor (mapconfig, query, { + resolution = 256, + threshold = Aggregation.THRESHOLD, + placement = 'centroid', + columns = {} + } = {}) { + this.mapconfig = mapconfig; + this.query = query; + this.resolution = resolution; + this.threshold = threshold; + this.placement = placement; + this.columns = columns; + } + sql () { + return aggregationQuery(this); + } +}; diff --git a/lib/cartodb/models/aggregation/base-aggregation.js b/lib/cartodb/models/aggregation/base-aggregation.js deleted file mode 100644 index 0e54689d..00000000 --- a/lib/cartodb/models/aggregation/base-aggregation.js +++ /dev/null @@ -1,12 +0,0 @@ -module.exports = class BaseAggregation { - constructor(query, resolution, threshold, placement, columns) { - this.query = query; - this.resolution = resolution; - this.threshold = threshold; - this.placement = placement; - this.columns = columns; - } - sql () { - throw new Error('Unimplemented method'); - } -}; diff --git a/lib/cartodb/models/aggregation/raster-aggregation.js b/lib/cartodb/models/aggregation/raster-aggregation.js deleted file mode 100644 index 7314b805..00000000 --- a/lib/cartodb/models/aggregation/raster-aggregation.js +++ /dev/null @@ -1,15 +0,0 @@ -const BaseAggregation = require('./base-aggregation'); -const aggregationTemplate = require('./aggregation-templates'); - -module.exports = class RasterAggregation extends BaseAggregation { - constructor () { - super(...arguments); - } - sql () { - return aggregationTemplate(this)({ - sourceQuery: this.query, - res: this.resolution, - columns: this.columns - }); - } -}; diff --git a/lib/cartodb/models/aggregation/vector-aggregation.js b/lib/cartodb/models/aggregation/vector-aggregation.js deleted file mode 100644 index 8cd4c859..00000000 --- a/lib/cartodb/models/aggregation/vector-aggregation.js +++ /dev/null @@ -1,15 +0,0 @@ -const BaseAggregation = require('./base-aggregation'); -const aggregationTemplate = require('./aggregation-templates'); - -module.exports = class VectorAggregation extends BaseAggregation { - constructor () { - super(...arguments); - } - sql () { - return aggregationTemplate(this)({ - sourceQuery: this.query, - res: this.resolution, - columns: this.columns - }); - } -}; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index fdd70bcf..b92c0563 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -1,4 +1,4 @@ -const AggregationProxy = require('../../aggregation/aggregation-proxy'); +const Aggregation = require('../../aggregation/aggregation'); const { MapConfig } = require('windshaft').model; const queryUtils = require('../../../utils/query-utils'); @@ -93,7 +93,7 @@ module.exports = class AggregationMapConfigAdapter { } if (shouldAdapt) { - const aggregation = new AggregationProxy(mapConfig, layer.options.sql, layer.options.aggregation); + const aggregation = new Aggregation(mapConfig, layer.options.sql, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = aggregation.sql(layer.options); @@ -130,7 +130,7 @@ module.exports = class AggregationMapConfigAdapter { const threshold = layer.options.aggregation && layer.options.aggregation.threshold ? layer.options.aggregation.threshold : - AggregationProxy.THRESHOLD; + Aggregation.THRESHOLD; if (estimatedFeatureCount < threshold) { return callback(null, shouldAdapt); From e8740af6efe1838737265aa27bd82d5f9d30f238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 16:34:36 +0100 Subject: [PATCH 59/78] Fix issue when sql_wrap is provided and aggregation metadata query fails --- .../adapter/aggregation-mapconfig-adapter.js | 4 +- test/acceptance/aggregation.js | 47 +++++++++++++++++++ test/support/prepare_db.sh | 4 +- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 1578dded..a663632f 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -118,7 +118,9 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, shouldAdapt); } - const aggregationMetadata = queryUtils.getAggregationMetadata({ query: layer.options.sql }); + const aggregationMetadata = queryUtils.getAggregationMetadata({ + query: layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql + }); connection.query(aggregationMetadata, (err, res) => { if (err) { diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 56815ccc..04772f25 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -49,6 +49,23 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; + const SQL_WRAP = ` + WITH hgrid AS ( + SELECT + CDB_RectangleGrid ( + ST_Expand(!bbox!, CDB_XYZ_Resolution(1) * 12), + CDB_XYZ_Resolution(1) * 12, + CDB_XYZ_Resolution(1) * 12 + ) as cell + ) + SELECT + hgrid.cell as the_geom_webmercator, + count(1) as agg_value, + count(1) /power( 12 * CDB_XYZ_Resolution(1), 2 ) as agg_value_density, + row_number() over () as cartodb_id + FROM hgrid, (<%= sql %>) i + WHERE ST_Intersects(i.the_geom_webmercator, hgrid.cell) GROUP BY hgrid.cell + `; function createVectorMapConfig (layers = [ { @@ -435,6 +452,36 @@ describe('aggregation', function () { done(); }); }); + + it('when sql_wrap is provided should return a layergroup', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql_wrap: SQL_WRAP, + sql: POINTS_SQL_1, + aggregation: { + threshold: 1 + } + } + } + ]); + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); + + done(); + }); + }); }); }); }); diff --git a/test/support/prepare_db.sh b/test/support/prepare_db.sh index b31b2c9b..03d97fc7 100755 --- a/test/support/prepare_db.sh +++ b/test/support/prepare_db.sh @@ -77,7 +77,7 @@ if test x"$PREPARE_PGSQL" = xyes; then createdb -Ttemplate_postgis -EUTF8 "${TEST_DB}" || die "Could not create test database" LOCAL_SQL_SCRIPTS='analysis_catalog windshaft.test gadm4 ported/populated_places_simple_reduced cdb_analysis_check cdb_invalidate_varnish' - REMOTE_SQL_SCRIPTS='CDB_QueryStatements CDB_QueryTables CDB_CartodbfyTable CDB_TableMetadata CDB_ForeignTable CDB_UserTables CDB_ColumnNames CDB_ZoomFromScale CDB_OverviewsSupport CDB_Overviews CDB_QuantileBins CDB_JenksBins CDB_HeadsTailsBins CDB_EqualIntervalBins CDB_Hexagon CDB_XYZ CDB_EstimateRowCount' + REMOTE_SQL_SCRIPTS='CDB_QueryStatements CDB_QueryTables CDB_CartodbfyTable CDB_TableMetadata CDB_ForeignTable CDB_UserTables CDB_ColumnNames CDB_ZoomFromScale CDB_OverviewsSupport CDB_Overviews CDB_QuantileBins CDB_JenksBins CDB_HeadsTailsBins CDB_EqualIntervalBins CDB_Hexagon CDB_XYZ CDB_EstimateRowCount CDB_RectangleGrid' CURL_ARGS="" for i in ${REMOTE_SQL_SCRIPTS} @@ -99,7 +99,7 @@ if test x"$PREPARE_PGSQL" = xyes; then sed -e 's/PARALLEL \= [A-Z]*,/''/g' \ -e 's/PARALLEL [A-Z]*/''/g' sql/$i.sql > $TMPFILE mv $TMPFILE sql/$i.sql - fi + fi cat sql/${i}.sql | sed -e 's/cartodb\./public./g' -e "s/''cartodb''/''public''/g" | sed "s/:PUBLICUSER/${PUBLICUSER}/" | From 98e8d745b11686e3d64fe5918b964fa9a5f02d54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 17:01:43 +0100 Subject: [PATCH 60/78] Support sql_wrap for aggregation --- .../adapter/aggregation-mapconfig-adapter.js | 5 ++-- test/acceptance/aggregation.js | 26 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index a663632f..a9a24c0e 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -93,10 +93,11 @@ module.exports = class AggregationMapConfigAdapter { } if (shouldAdapt) { - const aggregation = new Aggregation(mapConfig, layer.options.sql, layer.options.aggregation); + const sql = layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql; + const aggregation = new Aggregation(mapConfig, sql, layer.options.aggregation); const sqlQueryWrap = layer.options.sql_wrap; - let aggregationSql = aggregation.sql(layer.options); + let aggregationSql = aggregation.sql(); if (sqlQueryWrap) { layer.options.sql_raw = aggregationSql; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 04772f25..30f55888 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -482,6 +482,32 @@ describe('aggregation', function () { done(); }); }); + + it('when sql_wrap is provided should return a tile', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql_wrap: SQL_WRAP, + sql: POINTS_SQL_1, + aggregation: { + threshold: 1 + }, + cartocss: '#layer { marker-width: 1; }', + cartocss_version: '2.3.0' + } + } + ]); + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(0, 0, 0, {}, (err) => { + if (err) { + return done(err); + } + + done(); + }); + }); }); }); }); From 52d1cd47db14b962103eca02cc505481cda32373 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 19:24:17 +0100 Subject: [PATCH 61/78] Do not validate aggregation missing columns. It will fail afterwards in map validation --- .../aggregation/aggregation-map-config.js | 55 ------------------- .../adapter/aggregation-mapconfig-adapter.js | 11 +--- 2 files changed, 1 insertion(+), 65 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-map-config.js b/lib/cartodb/models/aggregation/aggregation-map-config.js index dfc52f60..e6524e3d 100644 --- a/lib/cartodb/models/aggregation/aggregation-map-config.js +++ b/lib/cartodb/models/aggregation/aggregation-map-config.js @@ -1,15 +1,8 @@ const MapConfig = require('windshaft').model.MapConfig; -const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ -' interactivity fields or attributes that are not present in the aggregation'; - module.exports = class AggregationMapConfig extends MapConfig { constructor (config, datasource) { super(config, datasource); - - if (this._hasAggregationMissingColumns()) { - throw new Error(MISSING_AGGREGATION_COLUMNS); - } } isAggregationMapConfig () { @@ -38,52 +31,4 @@ module.exports = class AggregationMapConfig extends MapConfig { return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } - - validateAggregation () { - if (this._hasAggregationMissingColumns()) { - throw new Error(MISSING_AGGREGATION_COLUMNS); - } - } - - _hasAggregationMissingColumns () { - const layers = this.getLayers(); - - if (!this.isAggregationMapConfig()) { - return false; - } - - for (let index = 0; index < layers.length; index++) { - const aggregationColumns = this._getAggregationColumnsByLayer(index); - const layerColumns = this.getColumnsByLayer(index); - - if (layerColumns.length === 0) { - continue; - } - - if (aggregationColumns.length !== layerColumns.length) { - return true; - } - - const missingColumns = this._getMissingColumns(aggregationColumns, layerColumns); - - if (missingColumns.length > 0) { - return true; - } - } - - return false; - } - - _getMissingColumns (aggregationColumns, layerColumns) { - return aggregationColumns.filter(column => !layerColumns.includes(column)); - } - - _getAggregationColumnsByLayer (index) { - const { aggregation } = this.getLayer(index).options; - const hasAggregationColumns = aggregation !== undefined && - typeof aggregation !== 'boolean' && - typeof aggregation.columns === 'object'; - - return hasAggregationColumns ? Object.keys(aggregation.columns) : []; - } }; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index a9a24c0e..a94ba21e 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -18,16 +18,7 @@ module.exports = class AggregationMapConfigAdapter { return callback(new Error(invalidAggregationParamValueErrorMessage({ value: params.aggregation }))); } - let mapConfig; - - try { - mapConfig = new AggregationMapConfig(requestMapConfig); - } catch (error) { - error.http_status = 400; - error.type = 'mapconfig'; - - return callback(error); - } + const mapConfig = new AggregationMapConfig(requestMapConfig); if (!this._shouldAdapt(mapConfig, params)) { return callback(null, requestMapConfig); From 0bc68d7144f3eb7d87d64e799937e66bbbc3e3df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 19:46:25 +0100 Subject: [PATCH 62/78] Do not override sql_raw --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index a94ba21e..cbbcd50f 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -91,7 +91,6 @@ module.exports = class AggregationMapConfigAdapter { let aggregationSql = aggregation.sql(); if (sqlQueryWrap) { - layer.options.sql_raw = aggregationSql; aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); } From 1edf684475cdd2ade6fa277861165ad3b47fc324 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 19:46:35 +0100 Subject: [PATCH 63/78] Fix test --- test/acceptance/aggregation.js | 71 ++++++------------- .../ported/multilayer_error_cases.js | 2 +- 2 files changed, 23 insertions(+), 50 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 30f55888..18533f60 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -37,7 +37,6 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; - const POLYGONS_SQL_1 = ` select st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry as the_geom, @@ -67,6 +66,17 @@ describe('aggregation', function () { WHERE ST_Intersects(i.the_geom_webmercator, hgrid.cell) GROUP BY hgrid.cell `; + const TURBO_CARTOCSS_SQL_WRAP = ` + #layer { + polygon-fill: ramp([agg_value], (#245668, #04817E, #39AB7E, #8BD16D, #EDEF5D), quantiles); + } + #layer::outline { + line-width: 1; + line-color: #FFFFFF; + line-opacity: 1; + } + `; + function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -216,7 +226,7 @@ describe('aggregation', function () { return done(err); } - assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + assert.ok(body.errors[0].match(/column "value" does not exist/)); done(); }); @@ -236,7 +246,9 @@ describe('aggregation', function () { type: 'cartodb', options: { sql: POINTS_SQL_2, - aggregation: true, + aggregation: { + threshold: 1 + }, cartocss: '#layer { marker-width: [value]; }', cartocss_version: '2.3.0' } @@ -249,48 +261,7 @@ describe('aggregation', function () { return done(err); } - assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); - - done(); - }); - }); - - it('should fail if aggregation misses a column defined in interactivity', - function (done) { - const response = { - status: 400, - headers: { - 'Content-Type': 'application/json; charset=utf-8' - } - }; - - this.mapConfig = createVectorMapConfig([ - { - type: 'cartodb', - options: { - sql: POINTS_SQL_2, - aggregation: { - columns: { - total: { - aggregate_function: 'sum', - aggregated_column: 'value' - } - } - }, - cartocss: '#layer { marker-width: [value]; }', - cartocss_version: '2.3.0', - interactivity: ['sqrt_value'] - } - } - ]); - - this.testClient = new TestClient(this.mapConfig); - this.testClient.getLayergroup({ response }, (err, body) => { - if (err) { - return done(err); - } - - assert.equal(body.errors[0], MISSING_AGGREGATION_COLUMNS); + assert.ok(body.errors[0].match(/column "value" does not exist/)); done(); }); @@ -462,7 +433,9 @@ describe('aggregation', function () { sql: POINTS_SQL_1, aggregation: { threshold: 1 - } + }, + cartocss: TURBO_CARTOCSS_SQL_WRAP, + cartocss_version: '3.0.12' } } ]); @@ -477,7 +450,7 @@ describe('aggregation', function () { assert.ok(Array.isArray(body.metadata.layers)); body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); - body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.png)); done(); }); @@ -493,8 +466,8 @@ describe('aggregation', function () { aggregation: { threshold: 1 }, - cartocss: '#layer { marker-width: 1; }', - cartocss_version: '2.3.0' + cartocss: TURBO_CARTOCSS_SQL_WRAP, + cartocss_version: '3.0.12' } } ]); diff --git a/test/acceptance/ported/multilayer_error_cases.js b/test/acceptance/ported/multilayer_error_cases.js index 936446f2..6e7f5964 100644 --- a/test/acceptance/ported/multilayer_error_cases.js +++ b/test/acceptance/ported/multilayer_error_cases.js @@ -57,7 +57,7 @@ describe('multilayer error cases', function() { res.body, '/**/ typeof test === \'function\' && ' + 'test({"errors":["Missing layers array from layergroup config"],' + - '"errors_with_context":[{"type":"mapconfig",' + + '"errors_with_context":[{"type":"unknown",' + '"message":"Missing layers array from layergroup config"}]});' ); done(); From b83351a5045c88dc875ab0827900d3acbf0780b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 13 Dec 2017 20:07:23 +0100 Subject: [PATCH 64/78] Use last release of windshaft --- package.json | 2 +- yarn.lock | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 9c0e4273..26bb0405 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "step-profiler": "~0.3.0", "turbo-carto": "0.20.2", "underscore": "~1.6.0", - "windshaft": "cartodb/windshaft#mapconfig-aggregation", + "windshaft": "4.1.0", "yargs": "~5.0.0" }, "devDependencies": { diff --git a/yarn.lock b/yarn.lock index a7aed387..78e663f0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2,7 +2,7 @@ # yarn lockfile v1 -"abaculus@github:cartodb/abaculus#2.0.3-cdb1": +abaculus@cartodb/abaculus#2.0.3-cdb1: version "2.0.3-cdb1" resolved "https://codeload.github.com/cartodb/abaculus/tar.gz/f5f34e1c80cdd8d49edd1d6fe3b2220ab2e23aaf" dependencies: @@ -226,7 +226,7 @@ camshaft@0.60.0: dot "^1.0.3" request "^2.69.0" -"canvas@github:cartodb/node-canvas#1.6.2-cdb2": +canvas@cartodb/node-canvas#1.6.2-cdb2: version "1.6.2-cdb2" resolved "https://codeload.github.com/cartodb/node-canvas/tar.gz/8acf04557005c633f9e68524488a2657c04f3766" dependencies: @@ -252,7 +252,7 @@ carto@CartoDB/carto#0.15.1-cdb1: optimist "~0.6.0" underscore "~1.6.0" -"carto@github:cartodb/carto#0.15.1-cdb3": +carto@cartodb/carto#0.15.1-cdb3: version "0.15.1-cdb3" resolved "https://codeload.github.com/cartodb/carto/tar.gz/945f5efb74fd1af1f5e1f69f409f9567f94fb5a7" dependencies: @@ -1633,7 +1633,7 @@ pg-types@1.*: postgres-date "~1.0.0" postgres-interval "^1.1.0" -"pg@github:cartodb/node-postgres#6.1.6-cdb1": +pg@cartodb/node-postgres#6.1.6-cdb1: version "6.1.6" resolved "https://codeload.github.com/cartodb/node-postgres/tar.gz/3eef52dd1e655f658a4ee8ac5697688b3ecfed44" dependencies: @@ -2223,7 +2223,7 @@ through@2: version "2.3.8" resolved "https://registry.yarnpkg.com/through/-/through-2.3.8.tgz#0dd4c9ffaabc357960b1b724115d7e0e86a2e1f5" -"tilelive-bridge@github:cartodb/tilelive-bridge#2.3.1-cdb4": +tilelive-bridge@cartodb/tilelive-bridge#2.3.1-cdb4: version "2.3.1-cdb4" resolved "https://codeload.github.com/cartodb/tilelive-bridge/tar.gz/faa2b638da2d119b78281575d40255cb523f6ca6" dependencies: @@ -2231,7 +2231,7 @@ through@2: mapnik-pool "~0.1.3" sphericalmercator "1.0.x" -"tilelive-mapnik@github:cartodb/tilelive-mapnik#0.6.18-cdb3": +tilelive-mapnik@cartodb/tilelive-mapnik#0.6.18-cdb3: version "0.6.18-cdb3" resolved "https://codeload.github.com/cartodb/tilelive-mapnik/tar.gz/23bd1c31dd57d0b76c86b9f1eaf62462b3c17d01" dependencies: From bcfc43a517854858b306a166a3062e95d977e1cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Dec 2017 11:22:00 +0100 Subject: [PATCH 65/78] jshint, my old friend --- test/acceptance/aggregation.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 18533f60..8e1e8f6d 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -3,8 +3,6 @@ require('../support/test_helper'); const assert = require('../support/assert'); const TestClient = require('../support/test-client'); const serverOptions = require('../../lib/cartodb/server_options'); -const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map-config defines cartocss expressions,'+ - ' interactivity fields or attributes that are not present in the aggregation'; const suites = [{ desc: 'mvt (mapnik)', From daa3fdca119b5702ea4eb7452104361d93be8c4f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 12:12:43 +0100 Subject: [PATCH 66/78] Fix bug in point-grid aggregation --- lib/cartodb/models/aggregation/aggregation-query.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 7dd55b3c..55ee53ea 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -50,6 +50,7 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { }; const aggregateColumns = ctx => { + // TODO: always add count let columns = ctx.columns || {}; if (Object.keys(columns).length === 0) { // default aggregation @@ -117,7 +118,7 @@ const aggregationQueryTemplates = { GROUP BY _cdb_gx, _cdb_gy ) SELECT - ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res*0.5)), 3857) AS the_geom_webmercator, + ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res+0.5)), 3857) AS the_geom_webmercator, _cdb_feature_count FROM _cdb_clusters, _cdb_params `, From b0e47ecc6201643cc0821317bc6963a935af12f2 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 12:23:02 +0100 Subject: [PATCH 67/78] Fix aggregation resolution parameter It was implemented as the inverse of the intended value --- lib/cartodb/models/aggregation/aggregation-query.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 55ee53ea..bf995c82 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -72,8 +72,8 @@ const aggregateColumns = ctx => { }; // Notes: -// * ${ctx.res*0.00028/256}*!scale_denominator! is equivalent to -// ${ctx.res/256}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) +// * ${256*0.00028/ctx.res}*!scale_denominator! is equivalent to +// ${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) // * We need to filter spatially using !bbox! to make the queries efficient because // the filter added by Mapnik (wrapping the query) // is only applied after the aggregation. @@ -83,7 +83,7 @@ const aggregationQueryTemplates = { 'centroid': ctx => ` WITH _cdb_params AS ( SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, !bbox! AS bbox ) SELECT @@ -105,7 +105,7 @@ const aggregationQueryTemplates = { 'point-grid': ctx => ` WITH _cdb_params AS ( SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, !bbox! AS bbox ), _cdb_clusters AS ( @@ -127,7 +127,7 @@ const aggregationQueryTemplates = { WITH _cdb_params AS ( SELECT - (${ctx.res*0.00028/256}*!scale_denominator!)::double precision AS res, + (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, !bbox! AS bbox ), _cdb_clusters AS ( SELECT From a987f6ac05d8c9e4fe221741d5873b735497d10f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Dec 2017 14:14:55 +0100 Subject: [PATCH 68/78] Fix issue when the sql has single quotes defined and the aggregation metadata query was not able to estimate row count --- lib/cartodb/utils/query-utils.js | 2 +- test/acceptance/aggregation.js | 50 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 6c80485a..c0d0aec2 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -22,7 +22,7 @@ module.exports.extractTableNames = function extractTableNames(query) { }; function getQueryRowEstimation(query) { - return 'select CDB_EstimateRowCount(\'' + query + '\') as rows'; + return 'select CDB_EstimateRowCount($windshaft$' + query + '$windshaft$) as rows'; } module.exports.getQueryRowCount = getQueryRowEstimation; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 8e1e8f6d..d08a0ea0 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -26,6 +26,19 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; + const POINTS_SQL_TIMESTAMP_1 = ` + select + st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, + st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, + x as value, + date + from + generate_series(-3, 3) x, + generate_series( + '2007-02-15 01:00:00'::timestamp, '2007-02-18 01:00:00'::timestamp, '1 day'::interval + ) date + `; + const POINTS_SQL_2 = ` select st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326) as the_geom, @@ -479,6 +492,43 @@ describe('aggregation', function () { done(); }); }); + + it('should work when the sql has single quotes', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: ` + SELECT + the_geom_webmercator, + the_geom, + value, + DATE_PART('day', date::timestamp - '1912-12-31 01:00:00'::timestamp )::numeric AS day + FROM (${POINTS_SQL_TIMESTAMP_1}) _query + `, + aggregation: { + threshold: 1 + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation.mvt)); + body.metadata.layers.forEach(layer => assert.ok(!layer.meta.aggregation.png)); + + done(); + }); + }); }); }); }); From b81cfe418ad9bb8e505f0f8e80cb109648366230 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 15:02:03 +0100 Subject: [PATCH 69/78] Always add a _cdb_feature_count to aggregated queries --- .../models/aggregation/aggregation-query.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index bf995c82..f088e6b3 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -50,16 +50,11 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { }; const aggregateColumns = ctx => { - // TODO: always add count - let columns = ctx.columns || {}; - if (Object.keys(columns).length === 0) { - // default aggregation - columns = { - _cdb_feature_count: { - aggregate_function: 'count' - } - }; - } + let columns = Object.assign({ + _cdb_feature_count: { + aggregate_function: 'count' + } + }, ctx.columns || {}); return Object.keys(columns).map(column_name => { const aggregate_function = columns[column_name].aggregate_function || 'count'; const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; From d311dccce8845219336e75ab953b78aac40641a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Dec 2017 16:35:09 +0100 Subject: [PATCH 70/78] Add test to check tangram compatibility --- test/acceptance/aggregation.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index d08a0ea0..46ea56b0 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -166,6 +166,33 @@ describe('aggregation', function () { }); }); + it('should return a NOT aggregated layergroup', function (done) { + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + cartocss: '#layer { marker-width: [value]; }', + cartocss_version: '2.3.0' + } + } + ]); + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + body.metadata.layers.forEach(layer => assert.equal(layer.meta.aggregation, undefined)); + + done(); + }); + }); + it('should return a layergroup with aggregation and cartocss compatible', function (done) { this.mapConfig = createVectorMapConfig([ { From 753ada0e76a1090f805790608e3f233ce6d7af57 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 16:36:24 +0100 Subject: [PATCH 71/78] Add cartodb_id to test datasets --- test/acceptance/aggregation.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index d08a0ea0..7bd1d328 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -20,6 +20,7 @@ describe('aggregation', function () { const POINTS_SQL_1 = ` select + x + 4 as cartodb_id, st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, x as value @@ -28,6 +29,7 @@ describe('aggregation', function () { const POINTS_SQL_TIMESTAMP_1 = ` select + row_number() over() AS cartodb_id, st_setsrid(st_makepoint(x*10, x*10), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10), 4326), 3857) as the_geom_webmercator, x as value, @@ -41,6 +43,7 @@ describe('aggregation', function () { const POINTS_SQL_2 = ` select + x + 4 as cartodb_id, st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326) as the_geom, st_transform(st_setsrid(st_makepoint(x*10, x*10*(-1)), 4326), 3857) as the_geom_webmercator, x as value, @@ -50,6 +53,7 @@ describe('aggregation', function () { const POLYGONS_SQL_1 = ` select + x + 4 as cartodb_id, st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry as the_geom, st_transform( st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry, From ba6cca46a171f3f5f24526748056381a573d64dc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 16:37:15 +0100 Subject: [PATCH 72/78] Fix aggregation queries --- .../models/aggregation/aggregation-query.js | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index f088e6b3..b987e24d 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -50,11 +50,20 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { }; const aggregateColumns = ctx => { - let columns = Object.assign({ + return Object.assign({ _cdb_feature_count: { aggregate_function: 'count' } }, ctx.columns || {}); +}; + +const aggregateColumnNames = ctx => { + let columns = aggregateColumns(ctx); + return Object.keys(columns).join(', '); +}; + +const aggregateColumnDefs = ctx => { + let columns = aggregateColumns(ctx); return Object.keys(columns).map(column_name => { const aggregate_function = columns[column_name].aggregate_function || 'count'; const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; @@ -89,7 +98,7 @@ const aggregationQueryTemplates = { AVG(ST_Y(_cdb_query.the_geom_webmercator)) ), 3857 ) AS the_geom_webmercator, - ${aggregateColumns(ctx)} + ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY @@ -107,14 +116,14 @@ const aggregationQueryTemplates = { SELECT Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, - ${aggregateColumns(ctx)} + ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE the_geom_webmercator && _cdb_params.bbox GROUP BY _cdb_gx, _cdb_gy ) SELECT ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res+0.5)), 3857) AS the_geom_webmercator, - _cdb_feature_count + ${aggregateColumnNames(ctx)} FROM _cdb_clusters, _cdb_params `, @@ -127,7 +136,7 @@ const aggregationQueryTemplates = { ), _cdb_clusters AS ( SELECT MIN(cartodb_id) AS cartodb_id, - ${aggregateColumns(ctx)} + ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY @@ -136,7 +145,7 @@ const aggregationQueryTemplates = { ) SELECT _cdb_clusters.cartodb_id, the_geom, the_geom_webmercator, - _cdb_feature_count + ${aggregateColumnNames(ctx)} FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) From 507d105ab27a4debdc34a22860624a7fc4fe0068 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 16:37:40 +0100 Subject: [PATCH 73/78] Add mode aggregation --- lib/cartodb/models/aggregation/aggregation-query.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index b987e24d..ca6d3431 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -46,6 +46,9 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { }, 'max': { sql: (column_name, params) => `max(${params.aggregated_column || column_name})` + }, + 'mode': { + sql: (column_name, params) => `cdb_mode(${params.aggregated_column || column_name})` } }; From 9d8ce6bc44055c58f5f465d45f2dfdff5e627827 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 16:51:55 +0100 Subject: [PATCH 74/78] Refactor aggregation resolution --- .../models/aggregation/aggregation-query.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index ca6d3431..332241d8 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -78,9 +78,14 @@ const aggregateColumnDefs = ctx => { }).join(', '); }; + +// SQL expression to compute the aggregation resolution (grid cell size). +// This is equivalent to `${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))` +// This is defined by the ctx.res parameter, which is the number of grid cells per tile linear dimension +// (i.e. each tile is divided into ctx.res*ctx.res cells). +const gridResolution = ctx => `(${256*0.00028/ctx.res}*!scale_denominator!)::double precision`; + // Notes: -// * ${256*0.00028/ctx.res}*!scale_denominator! is equivalent to -// ${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)) // * We need to filter spatially using !bbox! to make the queries efficient because // the filter added by Mapnik (wrapping the query) // is only applied after the aggregation. @@ -90,7 +95,7 @@ const aggregationQueryTemplates = { 'centroid': ctx => ` WITH _cdb_params AS ( SELECT - (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, + ${gridResolution(ctx)} AS res, !bbox! AS bbox ) SELECT @@ -112,7 +117,7 @@ const aggregationQueryTemplates = { 'point-grid': ctx => ` WITH _cdb_params AS ( SELECT - (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, + ${gridResolution(ctx)} AS res, !bbox! AS bbox ), _cdb_clusters AS ( @@ -133,8 +138,7 @@ const aggregationQueryTemplates = { 'point-sample': ctx => ` WITH _cdb_params AS ( SELECT - - (${256*0.00028/ctx.res}*!scale_denominator!)::double precision AS res, + ${gridResolution(ctx)} AS res, !bbox! AS bbox ), _cdb_clusters AS ( SELECT From f95c310462a56e44e5761b6ae56cc60231aef4c5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 17:03:49 +0100 Subject: [PATCH 75/78] Redefine aggregation torque to match Torque Now the resolution aggregation parameter has the same meaning as in Torque (-torque-resolution in CartoCSS) --- lib/cartodb/models/aggregation/aggregation-query.js | 5 +++-- lib/cartodb/models/aggregation/aggregation.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 332241d8..6dd5a165 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -19,13 +19,14 @@ const templateForOptions = (options) => { /** * Generates an aggregation query given the aggregation options: * - query - * - resolution + * - resolution - defined as in torque: + * aggregation cell is resolution*resolution pixels, where tiles are always 256x256 pixels * - columns * - placement */ const queryForOptions = (options) => templateForOptions(options)({ sourceQuery: options.query, - res: options.resolution, + res: 256/options.resolution, columns: options.columns }); diff --git a/lib/cartodb/models/aggregation/aggregation.js b/lib/cartodb/models/aggregation/aggregation.js index 447740b4..d95b74cd 100644 --- a/lib/cartodb/models/aggregation/aggregation.js +++ b/lib/cartodb/models/aggregation/aggregation.js @@ -6,7 +6,7 @@ module.exports = class Aggregation { } constructor (mapconfig, query, { - resolution = 256, + resolution = 1, threshold = Aggregation.THRESHOLD, placement = 'centroid', columns = {} From 0c044636ef372e20c99aea46281bf323f5c4883a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 17:22:50 +0100 Subject: [PATCH 76/78] Fix mode aggregation --- lib/cartodb/models/aggregation/aggregation-query.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 6dd5a165..89ab47ad 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -49,7 +49,7 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { sql: (column_name, params) => `max(${params.aggregated_column || column_name})` }, 'mode': { - sql: (column_name, params) => `cdb_mode(${params.aggregated_column || column_name})` + sql: (column_name, params) => `_cdb_mode(${params.aggregated_column || column_name})` } }; From 6b472c0b2072721d93cae6f8cc7d1e039dca347b Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 14 Dec 2017 17:51:49 +0100 Subject: [PATCH 77/78] Experimental aggregation dimensions This is not meant por public consumption (exposing SQL expressions is undesiderable) --- .../models/aggregation/aggregation-query.js | 55 ++++++++++++++----- lib/cartodb/models/aggregation/aggregation.js | 4 +- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 89ab47ad..b42b1c57 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -7,6 +7,7 @@ * - sourceQuery * - res * - columns + * - dimensions */ const templateForOptions = (options) => { let templateFn = aggregationQueryTemplates[options.placement]; @@ -23,11 +24,13 @@ const templateForOptions = (options) => { * aggregation cell is resolution*resolution pixels, where tiles are always 256x256 pixels * - columns * - placement + * - dimensions */ const queryForOptions = (options) => templateForOptions(options)({ sourceQuery: options.query, res: 256/options.resolution, - columns: options.columns + columns: options.columns, + dimensions: options.dimensions }); module.exports = queryForOptions; @@ -53,6 +56,11 @@ const SUPPORTED_AGGREGATE_FUNCTIONS = { } }; +const sep = (list) => { + let expr = list.join(', '); + return expr ? ', ' + expr : expr; +}; + const aggregateColumns = ctx => { return Object.assign({ _cdb_feature_count: { @@ -63,12 +71,12 @@ const aggregateColumns = ctx => { const aggregateColumnNames = ctx => { let columns = aggregateColumns(ctx); - return Object.keys(columns).join(', '); + return sep(Object.keys(columns)); }; const aggregateColumnDefs = ctx => { let columns = aggregateColumns(ctx); - return Object.keys(columns).map(column_name => { + return sep(Object.keys(columns).map(column_name => { const aggregate_function = columns[column_name].aggregate_function || 'count'; const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function]; if (!aggregate_definition) { @@ -76,10 +84,24 @@ const aggregateColumnDefs = ctx => { } const aggregate_expression = aggregate_definition.sql(column_name, columns[column_name]); return `${aggregate_expression} AS ${column_name}`; - }).join(', '); + })); }; +const aggregateDimensions = ctx => ctx.dimensions || {}; + +const dimensionNames = ctx => { + return sep(Object.keys(aggregateDimensions(ctx))); +}; + +const dimensionDefs = ctx => { + let dimensions = aggregateDimensions(ctx); + return sep(Object.keys(dimensions).map(dimension_name => { + const expression = dimensions[dimension_name]; + return `${expression} AS ${dimension_name}`; + })); +}; + // SQL expression to compute the aggregation resolution (grid cell size). // This is equivalent to `${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))` // This is defined by the ctx.res parameter, which is the number of grid cells per tile linear dimension @@ -106,13 +128,15 @@ const aggregationQueryTemplates = { AVG(ST_X(_cdb_query.the_geom_webmercator)), AVG(ST_Y(_cdb_query.the_geom_webmercator)) ), 3857 - ) AS the_geom_webmercator, + ) AS the_geom_webmercator + ${dimensionDefs(ctx)} ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + ${dimensionNames(ctx)} `, 'point-grid': ctx => ` @@ -124,14 +148,16 @@ const aggregationQueryTemplates = { _cdb_clusters AS ( SELECT Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gx, - Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy, + Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res)::int AS _cdb_gy + ${dimensionDefs(ctx)} ${aggregateColumnDefs(ctx)} - FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + FROM (${ctx.sourceQuery(ctx)}) _cdb_query, _cdb_params WHERE the_geom_webmercator && _cdb_params.bbox - GROUP BY _cdb_gx, _cdb_gy + GROUP BY _cdb_gx, _cdb_gy ${dimensionNames} ) SELECT - ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res+0.5)), 3857) AS the_geom_webmercator, + ST_SetSRID(ST_MakePoint(_cdb_gx*(res+0.5), _cdb_gy*(res+0.5)), 3857) AS the_geom_webmercator + ${dimensionNames(ctx)} ${aggregateColumnNames(ctx)} FROM _cdb_clusters, _cdb_params `, @@ -143,19 +169,22 @@ const aggregationQueryTemplates = { !bbox! AS bbox ), _cdb_clusters AS ( SELECT - MIN(cartodb_id) AS cartodb_id, + MIN(cartodb_id) AS cartodb_id + ${dimensionDefs(ctx)} ${aggregateColumnDefs(ctx)} - FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params + FROM (${ctx.sourceQuery(ctx)}) _cdb_query, _cdb_params WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) + ${dimensionNames(ctx)} ) SELECT _cdb_clusters.cartodb_id, - the_geom, the_geom_webmercator, + the_geom, the_geom_webmercator + ${dimensionNames(ctx)} ${aggregateColumnNames(ctx)} FROM - _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query + _cdb_clusters INNER JOIN (${ctx.sourceQuery(ctx)}) _cdb_query ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) ` }; diff --git a/lib/cartodb/models/aggregation/aggregation.js b/lib/cartodb/models/aggregation/aggregation.js index d95b74cd..2642898c 100644 --- a/lib/cartodb/models/aggregation/aggregation.js +++ b/lib/cartodb/models/aggregation/aggregation.js @@ -9,7 +9,8 @@ module.exports = class Aggregation { resolution = 1, threshold = Aggregation.THRESHOLD, placement = 'centroid', - columns = {} + columns = {}, + dimensions = {} } = {}) { this.mapconfig = mapconfig; this.query = query; @@ -17,6 +18,7 @@ module.exports = class Aggregation { this.threshold = threshold; this.placement = placement; this.columns = columns; + this.dimensions = dimensions; } sql () { return aggregationQuery(this); From 434de7786c5e4815a5138776bcfe653ba2ab04c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 14 Dec 2017 18:26:15 +0100 Subject: [PATCH 78/78] Fix test from merge --- test/acceptance/error-middleware.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/acceptance/error-middleware.js b/test/acceptance/error-middleware.js index 3ad22774..4df5b7e6 100644 --- a/test/acceptance/error-middleware.js +++ b/test/acceptance/error-middleware.js @@ -17,22 +17,24 @@ describe('error middleware', function () { message: "Missing cartocss for layer 0 options", name: "Error", label: "ANONYMOUS LAYERGROUP", - type: "layer", + type: "layer", }, moreErrors: [] }; this.testClient = new TestClient(mapConfig, 1234); - - const expectedResponse = { - status: 400, - headers: { - 'Content-Type': 'application/json; charset=utf-8', - 'X-Tiler-Errors': JSON.stringify(errorHeader) + + const params = { + response: { + status: 400, + headers: { + 'Content-Type': 'application/json; charset=utf-8', + 'X-Tiler-Errors': JSON.stringify(errorHeader) + } } }; - this.testClient.getLayergroup(expectedResponse, (err) => { + this.testClient.getLayergroup(params, (err) => { assert.ifError(err); done(); });