From e9a4fc4b2c1188e3262a820de95ac38e31e36d4b Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Dec 2017 11:31:33 +0100 Subject: [PATCH 1/5] Use full-sample aggregation only as default Sampling is performed only when placement, columns or dimensions are specified; otherwise the regular centroid/grid-point/grid-center is used without sampling. --- .../aggregation/aggregation-mapconfig.js | 8 +- .../models/aggregation/aggregation-query.js | 98 ++++++--- test/acceptance/aggregation.js | 194 ++++++++++++++++-- 3 files changed, 249 insertions(+), 51 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index bf334338..320239bf 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -16,10 +16,6 @@ module.exports = class AggregationMapConfig extends MapConfig { return aggregationQuery.SUPPORTED_PLACEMENTS; } - static get PLACEMENT () { - return AggregationMapConfig.PLACEMENTS.find(placement => placement === 'centroid'); - } - static get THRESHOLD () { return 1e5; // 100K } @@ -55,9 +51,11 @@ module.exports = class AggregationMapConfig extends MapConfig { getAggregatedQuery (index) { const { sql_raw, sql } = this.getLayer(index).options; const { + // The default aggregation has no placement, columns or dimensions; + // this enables the special "full-sample" aggregation. resolution = AggregationMapConfig.RESOLUTION, threshold = AggregationMapConfig.THRESHOLD, - placement = AggregationMapConfig.PLACEMENT, + placement = null, columns = {}, dimensions = {} } = this.getAggregation(index); diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index fd860318..ab56e9b5 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -1,8 +1,12 @@ +const DEFAULT_PLACEMENT = 'centroid'; + /** * 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 + * - columns + * - dimensions* * The query template parameters taken by the result template function are: * - sourceQuery * - res @@ -10,13 +14,28 @@ * - dimensions */ const templateForOptions = (options) => { - let templateFn = aggregationQueryTemplates[options.placement]; - if (!templateFn) { - throw new Error("Invalid Aggregation placement: '" + options.placement + "'"); + let templateFn = defaultAggregationQueryTemplate; + if (!isDefaultAggregation(options)) { + templateFn = aggregationQueryTemplates[options.placement || DEFAULT_PLACEMENT]; + if (!templateFn) { + throw new Error("Invalid Aggregation placement: '" + options.placement + "'"); + } } return templateFn; }; +const isEmptyParameter = (parameter) => !parameter || Object.keys(parameter).length === 0; + +/** + * When no placement, columns or dimensions are specified in the aggregation + * a special default aggregation is used. + */ +const isDefaultAggregation = (options) => { + return !options || ( + !options.placement && isEmptyParameter(options.columns) && isEmptyParameter(options.dimensions) + ); +}; + /** * Generates an aggregation query given the aggregation options: * - query @@ -25,6 +44,11 @@ const templateForOptions = (options) => { * - columns * - placement * - dimensions + * + * The default aggregation (when no explicit placement, columns or dimensions are present) returns + * a sample record (with all the original columns and _cdb_feature_count) for each aggregation group. + * When placement, columns or dimensions are specified, columns are aggregated as requested + * (by default only _cdb_feature_count) and with the_geom_webmercator as defined by placement. */ const queryForOptions = (options) => templateForOptions(options)({ sourceQuery: options.query, @@ -35,6 +59,9 @@ const queryForOptions = (options) => templateForOptions(options)({ module.exports = queryForOptions; +// Checks if the aggregation parameters represent the default (full-sample) aggregation +module.exports.isDefaultAggregation = isDefaultAggregation; + const SUPPORTED_AGGREGATE_FUNCTIONS = { 'count': { sql: (column_name, params) => `count(${params.aggregated_column || '*'})` @@ -92,7 +119,13 @@ const aggregateColumnDefs = ctx => { const aggregateDimensions = ctx => ctx.dimensions || {}; -const dimensionNames = ctx => { +const dimensionNames = (ctx, table) => { + if (table) { + let dimensions = aggregateDimensions(ctx); + return sep(Object.keys(dimensions).map( + dimension_name => `${table}.${dimension_name}` + )); + } return sep(Object.keys(aggregateDimensions(ctx))); }; @@ -116,6 +149,34 @@ const gridResolution = ctx => `(${256*0.00028/ctx.res}*!scale_denominator!)::dou // is only applied after the aggregation. // * This queries are used for rendering and the_geom is omitted in the results for better performance +// The special default aggregation includes all the columns of a sample row per grid cell and +// the count (_cdb_feature_count) of the aggregated rows. +const defaultAggregationQueryTemplate = ctx => ` + WITH + _cdb_params AS ( + SELECT + ${gridResolution(ctx)} AS res, + !bbox! AS bbox + ), + _cdb_clusters AS ( + SELECT + MIN(cartodb_id) AS cartodb_id + ${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)} + ) SELECT + _cdb_query.* + ${aggregateColumnNames(ctx)} + FROM + _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query + ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) +`; + const aggregationQueryTemplates = { 'centroid': ctx => ` WITH @@ -188,38 +249,13 @@ const aggregationQueryTemplates = { SELECT _cdb_clusters.cartodb_id, the_geom, the_geom_webmercator - ${dimensionNames(ctx)} - ${aggregateColumnNames(ctx)} - FROM - _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query - ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) - `, - - 'full-sample': ctx => ` - WITH - _cdb_params AS ( - SELECT - ${gridResolution(ctx)} AS res, - !bbox! AS bbox - ), - _cdb_clusters AS ( - SELECT - MIN(cartodb_id) AS cartodb_id - ${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)} - ) SELECT - _cdb_query.* + ${dimensionNames(ctx, '_cdb_query')} ${aggregateColumnNames(ctx)} FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id) ` + }; module.exports.SUPPORTED_PLACEMENTS = Object.keys(aggregationQueryTemplates); diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 14040592..2d31fe98 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -289,7 +289,8 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_2, aggregation: { - threshold: 1 + threshold: 1, + placement: 'centroid' }, cartocss: '#layer { marker-width: [value]; }', cartocss_version: '2.3.0' @@ -309,6 +310,45 @@ describe('aggregation', function () { }); }); + it('should provide all columns in the default aggregation ', + function (done) { + const response = { + status: 200, + headers: { + 'Content-Type': 'application/json; charset=utf-8' + } + }; + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + 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(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('should skip aggregation to create a layergroup with aggregation defined already', function (done) { const mapConfig = createVectorMapConfig([ { @@ -569,6 +609,140 @@ describe('aggregation', function () { }); }); + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`dimensions should work for ${placement} placement`, function(done) { + + // FIXME: skip until pg-mvt renderer is able to return all columns + if (process.env.POSTGIS_VERSION === '2.4') { + return done(); + } + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + placement: placement , + threshold: 1, + dimensions: { + value: "value" + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.value, 'number') + ); + + done(); + }); + }); + }); + + it(`dimensions should trigger non-default aggregation`, function(done) { + + // FIXME: skip until pg-mvt renderer is able to return all columns + if (process.env.POSTGIS_VERSION === '2.4') { + return done(); + } + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + threshold: 1, + dimensions: { + value: "value" + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.value, 'number') + ); + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.sqrt_value, 'undefined') + ); + + done(); + }); + }); + + it(`aggregations should trigger non-default aggregation`, function(done) { + + // FIXME: skip until pg-mvt renderer is able to return all columns + if (process.env.POSTGIS_VERSION === '2.4') { + return done(); + } + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + aggregation: { + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.value, 'number') + ); + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.sqrt_value, 'undefined') + ); + + done(); + }); + }); it('should work when the sql has single quotes', function (done) { this.mapConfig = createVectorMapConfig([ { @@ -576,6 +750,7 @@ describe('aggregation', function () { options: { sql: ` SELECT + cartodb_id, the_geom_webmercator, the_geom, value, @@ -732,7 +907,7 @@ describe('aggregation', function () { }); }); - it('aggregates with full-sample placement', function (done) { + it('aggregates with full-sample placement by default', function (done) { this.mapConfig = createVectorMapConfig([ { type: 'cartodb', @@ -740,17 +915,6 @@ describe('aggregation', function () { sql: POINTS_SQL_1, resolution: 256, aggregation: { - placement: 'point-grid', - columns: { - total: { - aggregate_function: 'sum', - aggregated_column: 'value' - }, - v_avg: { - aggregate_function: 'avg', - aggregated_column: 'value' - } - }, threshold: 1 } } @@ -847,10 +1011,10 @@ describe('aggregation', function () { } assert.deepEqual(body, { - errors: [ 'Invalid placement. Valid values: centroid, point-grid, point-sample, full-sample'], + errors: [ 'Invalid placement. Valid values: centroid, point-grid, point-sample'], errors_with_context:[{ type: 'layer', - message: 'Invalid placement. Valid values: centroid, point-grid, point-sample, full-sample', + message: 'Invalid placement. Valid values: centroid, point-grid, point-sample', layer: { id: "layer0", index: 0, From 19bf079f2dcd2cfbf9443335eb760d2ddb6794b1 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Dec 2017 15:45:14 +0100 Subject: [PATCH 2/5] Exclude test from PostGIS 2.4 --- test/acceptance/aggregation.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 2d31fe98..1809c8a0 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -908,6 +908,12 @@ describe('aggregation', function () { }); it('aggregates with full-sample placement by default', function (done) { + + // FIXME: skip until pg-mvt renderer is able to return all columns + if (process.env.POSTGIS_VERSION === '2.4') { + return done(); + } + this.mapConfig = createVectorMapConfig([ { type: 'cartodb', From 54f32113f348d7fa0d0deb84b27f7d21928ba968 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Dec 2017 15:45:34 +0100 Subject: [PATCH 3/5] Add some aggregation tests --- test/acceptance/aggregation.js | 50 +++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 1809c8a0..afc27282 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -697,7 +697,7 @@ describe('aggregation', function () { }); }); - it(`aggregations should trigger non-default aggregation`, function(done) { + it(`aggregation columns should trigger non-default aggregation`, function(done) { // FIXME: skip until pg-mvt renderer is able to return all columns if (process.env.POSTGIS_VERSION === '2.4') { @@ -743,6 +743,54 @@ describe('aggregation', function () { done(); }); }); + + ['centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`aggregations with base column names should work for ${placement} placement`, function(done) { + + // FIXME: skip until pg-mvt renderer is able to return all columns + if (process.env.POSTGIS_VERSION === '2.4') { + return done(); + } + + this.mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_1, + aggregation: { + placement: placement , + threshold: 1, + columns: { + value: { + aggregate_function: 'sum', + aggregated_column: 'value' + } + } + } + } + } + ]); + + this.testClient = new TestClient(this.mapConfig); + const options = { + format: 'mvt' + }; + this.testClient.getTile(0, 0, 0, options, (err, res, tile) => { + if (err) { + return done(err); + } + + const tileJSON = tile.toJSON(); + + tileJSON[0].features.forEach( + feature => assert.equal(typeof feature.properties.value, 'number') + ); + + done(); + }); + }); + }); + it('should work when the sql has single quotes', function (done) { this.mapConfig = createVectorMapConfig([ { From 1ce80766998b4e1d4f7c3620962f9d88103f5fb0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Dec 2017 15:46:29 +0100 Subject: [PATCH 4/5] Change default aggregation placement to point-sample For consistency with the default 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 ab56e9b5..5ecf09aa 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -1,4 +1,4 @@ -const DEFAULT_PLACEMENT = 'centroid'; +const DEFAULT_PLACEMENT = 'point-sample'; /** * Returns a template function (function that accepts template parameters and returns a string) From d726c9ad013d072b03c6eea0d42ed68a7c0deaff Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 22 Dec 2017 15:48:30 +0100 Subject: [PATCH 5/5] Fix point-sample aggregation it failed in the case of aggregate columns with the name of base columns --- lib/cartodb/models/aggregation/aggregation-query.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 5ecf09aa..965d201f 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -98,8 +98,13 @@ const aggregateColumns = ctx => { }, ctx.columns || {}); }; -const aggregateColumnNames = ctx => { +const aggregateColumnNames = (ctx, table) => { let columns = aggregateColumns(ctx); + if (table) { + return sep(Object.keys(columns).map( + column_name => `${table}.${column_name}` + )); + } return sep(Object.keys(columns)); }; @@ -120,13 +125,13 @@ const aggregateColumnDefs = ctx => { const aggregateDimensions = ctx => ctx.dimensions || {}; const dimensionNames = (ctx, table) => { + let dimensions = aggregateDimensions(ctx); if (table) { - let dimensions = aggregateDimensions(ctx); return sep(Object.keys(dimensions).map( dimension_name => `${table}.${dimension_name}` )); } - return sep(Object.keys(aggregateDimensions(ctx))); + return sep(Object.keys(dimensions)); }; const dimensionDefs = ctx => { @@ -250,7 +255,7 @@ const aggregationQueryTemplates = { _cdb_clusters.cartodb_id, the_geom, the_geom_webmercator ${dimensionNames(ctx, '_cdb_query')} - ${aggregateColumnNames(ctx)} + ${aggregateColumnNames(ctx, '_cdb_clusters')} FROM _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id)