From e21ab12e4c9fcae42b1040f36f261cd0282b82e6 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 15 Jul 2018 22:15:57 +0200 Subject: [PATCH 01/11] Test that tiles do not contain partially aggregated clusters This tests #889 --- test/acceptance/aggregation.js | 131 +++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index ca350110..6b9cfcb2 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -134,6 +134,57 @@ describe('aggregation', function () { from generate_series(0, 13) x `; + // Some points at corners and centers of aggregation grids (zoom=1, resolution=1) + // @ = point location + // --+---+---+---+---+---+---+-- + // | | | | | | | + // --+---+---@---@---@---+---+-- + // | | | @ | @ | | | + // --+---+---@---@---@---+---+--- Y = 0 + // | | | @ | @ | | | + // --+---+---@---@---@---+---+-- + // | | | | | | | + // --+---+---+---+---+---+---+-- + // | + // X = 0 + // Point identifiers (cartodb_id) + // --+---+---+---+---+---+---+-- + // | | | | | | | + // --+---+---7---8---9---+---+-- + // | | |12 |13 | | | + // --+---+---4---5---6---+---+--- + // | | |10 |11 | | | + // --+---+---1---2---3---+---+-- + // | | | | | | | + // --+---+---+---+---+---+---+-- + // Point count per aggregation cell and Z=1 tiles + // + // Tile 0,0 -+---+---+---+- Tile 1,0 + // | | | 1 | 1 | 1 | | + // --+---+---@---@---@---+---+-- + // | | | 2 | 2 | 1 | | + // --+---+---@---@---@---+---+--- + // | | | 2 | 2 | 1 | | + // --+---+---@---@---@---+---+-- + // | | | | | | | + // Tile 0, 1 -+---+---+---+- Tile 1,1 + // + const POINTS_SQL_GRID = ` + WITH params AS ( + SELECT CDB_XYZ_Resolution(1) AS l -- cell size for Z=1 res=1 + ) + SELECT + row_number() OVER () AS cartodb_id, + ST_SetSRID(ST_MakePoint(x*l, y*l), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(x*l, y*l), 3857), 4326) AS the_geom + FROM params, generate_series(-1,1) x, generate_series(-1,1) y + UNION + SELECT + row_number() OVER () + 9 AS cartodb_id, + ST_SetSRID(ST_MakePoint(x*l-l/2, y*l-l/2), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(x*l-l/2, y*l-l/2), 3857), 4326) AS the_geom + FROM params, generate_series(0,1) x, generate_series(0,1) y + `; function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -2280,6 +2331,86 @@ describe('aggregation', function () { }); }); + + it(`for ${placement} each aggr. cell is in a single tile`, function (done) { + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_GRID, + resolution: 1, + aggregation: { + threshold: 1 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(1, 0, 0, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile00 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 0, 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile01 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 1, 0, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile10 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 1, 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile11 = JSON.parse(mvt.toGeoJSONSync(0)); + + const tile00Expected = [ + { cartodb_id: 4, _cdb_feature_count: 2 }, + { cartodb_id: 7, _cdb_feature_count: 1 } + ]; + const tile10Expected = [ + { cartodb_id: 5, _cdb_feature_count: 2 }, + { cartodb_id: 6, _cdb_feature_count: 1 }, + { cartodb_id: 8, _cdb_feature_count: 1 }, + { cartodb_id: 9, _cdb_feature_count: 1 } + ]; + const tile01Expected = [ + { cartodb_id: 1, _cdb_feature_count: 2 } + ]; + const tile11Expected = [ + { cartodb_id: 2, _cdb_feature_count: 2 }, + { cartodb_id: 3, _cdb_feature_count: 1 } + ]; + const tile00Actual = tile00.features.map(f => f.properties); + const tile10Actual = tile10.features.map(f => f.properties); + const tile01Actual = tile01.features.map(f => f.properties); + const tile11Actual = tile11.features.map(f => f.properties); + const orderById = (a, b) => a.cartodb_id - b.cartodb_id; + assert.deepEqual(tile00Actual.sort(orderById), tile00Expected); + assert.deepEqual(tile10Actual.sort(orderById), tile10Expected); + assert.deepEqual(tile01Actual.sort(orderById), tile01Expected); + assert.deepEqual(tile11Actual.sort(orderById), tile11Expected); + + done(); + }); + }); + }); + + }); + }); }); ['default', 'centroid', 'point-sample', 'point-grid'].forEach(placement => { From 92ef0a60fc3c94d1003e3a7db40110c17cc780e3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 15 Jul 2018 22:17:00 +0200 Subject: [PATCH 02/11] Test aggregation accuracy This tests #1001 --- test/acceptance/aggregation.js | 124 +++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 6b9cfcb2..7e8cb54d 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -185,6 +185,49 @@ describe('aggregation', function () { ST_Transform(ST_SetSRID(ST_MakePoint(x*l-l/2, y*l-l/2), 3857), 4326) AS the_geom FROM params, generate_series(0,1) x, generate_series(0,1) y `; + + // Points positioned inside one cell of Z=20, X=1000000, X=1000000 (the SW corner) + // The center of the cell is x = 18181005.874444414, y = -18181043.94366749 + const POINTS_SQL_CELL = ` + SELECT + 1 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857), 4326) AS the_geom + UNION SELECT + 2 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181005.9, -18181044.0), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181005.9, -18181044.0), 3857), 4326) AS the_geom + UNION SELECT + 3 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181005.87, -18181043.94), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181005.87, -18181043.94), 3857), 4326) AS the_geom + UNION SELECT + 4 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857), 4326) AS the_geom + `; + + // Points positioned inside one cell of Z=20, X=1000000, X=1000000 (inner cell not on border) + // The center of the cell is x = 18181006.023735486, y = -18181043.794376418 + const POINTS_SQL_CELL_INNER = ` + SELECT + 1 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181005.95, -18181043.8), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181005.95, -18181043.8), 3857), 4326) AS the_geom + UNION SELECT + 2 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181006.09, -18181043.72), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181006.09, -18181043.72), 3857), 4326) AS the_geom + UNION SELECT + 3 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181006.02, -18181043.79), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181006.02, -18181043.79), 3857), 4326) AS the_geom + UNION SELECT + 4 AS cartodb_id, + ST_SetSRID(ST_MakePoint(18181006.01, -18181043.75), 3857) AS the_geom_webmercator, + ST_Transform(ST_SetSRID(ST_MakePoint(18181006.01, -18181043.75), 3857), 4326) AS the_geom + `; + function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -2411,6 +2454,87 @@ describe('aggregation', function () { }); }); + + it.skip(`for ${placement} points aggregated into corner cluster`, function (done) { + // this test will fail due to !bbox! lack of accuracy if strict cell filtering is in place + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_CELL, + resolution: 1, + aggregation: { + threshold: 1 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(20, 1000000, 1000000, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile = JSON.parse(mvt.toGeoJSONSync(0)); + assert.equal(tile.features.length, 1); + assert.equal(tile.features[0].properties._cdb_feature_count, 4); + console.log(tile.features[0].properties); + if (placement === 'point-grid') { + // check geometry x = 18181005.874444414, y = -18181043.94366749 + assert.deepEqual(tile.features[0].geometry.coordinates, [ 163.322754576802, -83.3823797469878 ]); + } + done(); + }); + }); + + it(`for ${placement} points aggregated into correct cluster`, function (done) { + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_CELL_INNER, + resolution: 1, + aggregation: { + threshold: 1 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(20, 1000000, 1000000, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile = JSON.parse(mvt.toGeoJSONSync(0)); + assert.equal(tile.features.length, 1); + assert.equal(tile.features[0].properties._cdb_feature_count, 4); + console.log(tile.features[0].properties); + if (placement === 'point-grid') { + // check geometry x = 18181006.023735486, y = -18181043.794376418 + assert.deepEqual(tile.features[0].geometry.coordinates, [ 163.322755917907, -83.3823795924354 ]); + } + done(); + }); + }); }); ['default', 'centroid', 'point-sample', 'point-grid'].forEach(placement => { From 31feb58e1ffc14ff1d5ada726b314958bb729f26 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 15 Jul 2018 22:25:16 +0200 Subject: [PATCH 03/11] Compute aggregation cell size accurately This fixes #1001 --- lib/cartodb/models/aggregation/aggregation-query.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index cfb9855a..23028f1b 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -237,14 +237,15 @@ const havingClause = ctx => { }; // 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). // We limit the the minimum resolution to avoid division by zero problems. The limit used is // the pixel size of zoom level 30 (i.e. 1/2*(30+8) of the full earth web-mercator extent), which is about 0.15 mm. +// Computing this using !scale_denominator!, !pixel_width! or !pixel_height! produces +// inaccurate results due to rounding present in those values. const gridResolution = ctx => { const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38); - return `GREATEST(${256*0.00028/ctx.res}*!scale_denominator!, ${minimumResolution})::double precision`; + return `GREATEST(${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)), ${minimumResolution})::double precision`; }; // Notes: From 716f983e7193950a70334c01d368dded4e5ebf05 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 15 Jul 2018 22:26:37 +0200 Subject: [PATCH 04/11] Filter out partially aggregated clusters Fixes #889 --- .../models/aggregation/aggregation-query.js | 71 +++++++++++-------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 23028f1b..4c2c34bf 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -248,28 +248,58 @@ const gridResolution = ctx => { return `GREATEST(${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)), ${minimumResolution})::double precision`; }; +// Each aggregation cell is defined by the cell coordinates Floor(x/res), Floor(y/res), +// i.e. they include the West and South borders but not the East and North ones. +// So, to avoid picking points that don't belong to cells in the tile, given the tile +// limits Xmin, Ymin, Xmax, Ymax (bbox), we should select points that satisfy +// Xmin <= x < Xmax and Ymin <= y < Ymax (with x, y from the_geom_webmercator) +// On the other hand we can efficiently filter spatially (relying on spatial indexing) +// with `the_geom_webmercator && bbox` which is equivalent to +// Xmin <= x <= Xmax and Ymin <= y <= Ymax +// So, in order to be both efficient and accurate we will need to use both +// conditions for spatial filtering. +const spatialFilter = ` + (_cdb_query.the_geom_webmercator && _cdb_params.bbox) AND + ST_X(_cdb_query.the_geom_webmercator) >= _cdb_params.xmin AND ST_X(_cdb_query.the_geom_webmercator) < _cdb_params.xmax AND + ST_Y(_cdb_query.the_geom_webmercator) >= _cdb_params.ymin AND ST_Y(_cdb_query.the_geom_webmercator) < _cdb_params.ymax +`; + // Notes: // * 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 +// * If the MVT extent or tile buffer was 0 or a multiple of the resolution we could use directly +// the bbox for them, but in general we need to find the nearest cell limits inside the bbox. +const sqlParams = (ctx) => ` +_cdb_res AS ( + SELECT + ${gridResolution(ctx)} AS res, + !bbox! AS bbox +), +_cdb_params AS ( + SELECT + res, + bbox, + CEIL(ST_XMIN(bbox)/res)*res AS xmin, + FLOOR(ST_XMAX(bbox)/res)*res AS xmax, + CEIL(ST_YMIN(bbox)/res)*res AS ymin, + FLOOR(ST_YMAX(bbox)/res)*res AS ymax + FROM _cdb_res +) +`; // 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 - ), + WITH ${sqlParams(ctx)}, _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 + WHERE ${spatialFilter} GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) @@ -284,12 +314,7 @@ const defaultAggregationQueryTemplate = ctx => ` const aggregationQueryTemplates = { 'centroid': ctx => ` - WITH - _cdb_params AS ( - SELECT - ${gridResolution(ctx)} AS res, - !bbox! AS bbox - ) + WITH ${sqlParams(ctx)} SELECT MIN(_cdb_query.cartodb_id) AS cartodb_id, ST_SetSRID( @@ -301,7 +326,7 @@ const aggregationQueryTemplates = { ${dimensionDefs(ctx)} ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params - WHERE _cdb_query.the_geom_webmercator && _cdb_params.bbox + WHERE ${spatialFilter} GROUP BY Floor(ST_X(_cdb_query.the_geom_webmercator)/_cdb_params.res), Floor(ST_Y(_cdb_query.the_geom_webmercator)/_cdb_params.res) @@ -310,12 +335,7 @@ const aggregationQueryTemplates = { `, 'point-grid': ctx => ` - WITH - _cdb_params AS ( - SELECT - ${gridResolution(ctx)} AS res, - !bbox! AS bbox - ), + WITH ${sqlParams(ctx)}, _cdb_clusters AS ( SELECT MIN(_cdb_query.cartodb_id) AS cartodb_id, @@ -324,7 +344,7 @@ const aggregationQueryTemplates = { ${dimensionDefs(ctx)} ${aggregateColumnDefs(ctx)} FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params - WHERE the_geom_webmercator && _cdb_params.bbox + WHERE ${spatialFilter} GROUP BY _cdb_gx, _cdb_gy ${dimensionNames(ctx)} ${havingClause(ctx)} ) @@ -337,19 +357,14 @@ const aggregationQueryTemplates = { `, 'point-sample': ctx => ` - WITH - _cdb_params AS ( - SELECT - ${gridResolution(ctx)} AS res, - !bbox! AS bbox - ), + WITH ${sqlParams(ctx)}, _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 + WHERE ${spatialFilter} 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 7b597e022346c73f1e0eca6472aee3f63c2bfb38 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 15 Jul 2018 22:27:49 +0200 Subject: [PATCH 05/11] Update NEWS --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index a070cabc..6fc9b403 100644 --- a/NEWS.md +++ b/NEWS.md @@ -49,6 +49,8 @@ Bug Fixes: - Fix `meta.stats.estimatedFeatureCount` for aggregations and queries with tokens - Fix numeric histogram bounds when `start` and `end` are specified (#991) - Static maps filters correctly if `layer` option is passed in the url. +- Aggregation doesn't return out-of-tile, partially aggregated clusters +- Aggregation was not accurate for high zoom, far away from the origin tiles Announcements: * Improve error message when the DB query is over the user's limits From 2063ac15ee9ca3b02a1f5b9fc5ec8eed97e7e247 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 16 Jul 2018 11:58:56 +0200 Subject: [PATCH 06/11] Fix long lines --- lib/cartodb/models/aggregation/aggregation-query.js | 9 ++++++--- test/acceptance/aggregation.js | 12 ++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 4c2c34bf..e7eb2ecb 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -245,7 +245,8 @@ const havingClause = ctx => { // inaccurate results due to rounding present in those values. const gridResolution = ctx => { const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38); - return `GREATEST(${256/ctx.res}*CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!)), ${minimumResolution})::double precision`; + const pixelSize = 'CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))'; + return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`; }; // Each aggregation cell is defined by the cell coordinates Floor(x/res), Floor(y/res), @@ -260,8 +261,10 @@ const gridResolution = ctx => { // conditions for spatial filtering. const spatialFilter = ` (_cdb_query.the_geom_webmercator && _cdb_params.bbox) AND - ST_X(_cdb_query.the_geom_webmercator) >= _cdb_params.xmin AND ST_X(_cdb_query.the_geom_webmercator) < _cdb_params.xmax AND - ST_Y(_cdb_query.the_geom_webmercator) >= _cdb_params.ymin AND ST_Y(_cdb_query.the_geom_webmercator) < _cdb_params.ymax + ST_X(_cdb_query.the_geom_webmercator) >= _cdb_params.xmin AND + ST_X(_cdb_query.the_geom_webmercator) < _cdb_params.xmax AND + ST_Y(_cdb_query.the_geom_webmercator) >= _cdb_params.ymin AND + ST_Y(_cdb_query.the_geom_webmercator) < _cdb_params.ymax `; // Notes: diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 7e8cb54d..b37466f6 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -2375,7 +2375,7 @@ describe('aggregation', function () { }); }); - it(`for ${placement} each aggr. cell is in a single tile`, function (done) { + it.only(`for ${placement} each aggr. cell is in a single tile`, function (done) { this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 0 }, @@ -2487,16 +2487,16 @@ describe('aggregation', function () { const tile = JSON.parse(mvt.toGeoJSONSync(0)); assert.equal(tile.features.length, 1); assert.equal(tile.features[0].properties._cdb_feature_count, 4); - console.log(tile.features[0].properties); if (placement === 'point-grid') { // check geometry x = 18181005.874444414, y = -18181043.94366749 - assert.deepEqual(tile.features[0].geometry.coordinates, [ 163.322754576802, -83.3823797469878 ]); + const expectedPoint = [ 163.322754576802, -83.3823797469878 ]; + assert.deepEqual(tile.features[0].geometry.coordinates, expectedPoint); } done(); }); }); - it(`for ${placement} points aggregated into correct cluster`, function (done) { + it.only(`for ${placement} points aggregated into correct cluster`, function (done) { this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 0 }, @@ -2527,10 +2527,10 @@ describe('aggregation', function () { const tile = JSON.parse(mvt.toGeoJSONSync(0)); assert.equal(tile.features.length, 1); assert.equal(tile.features[0].properties._cdb_feature_count, 4); - console.log(tile.features[0].properties); if (placement === 'point-grid') { // check geometry x = 18181006.023735486, y = -18181043.794376418 - assert.deepEqual(tile.features[0].geometry.coordinates, [ 163.322755917907, -83.3823795924354 ]); + const expectedPoint = [ 163.322755917907, -83.3823795924354 ]; + assert.deepEqual(tile.features[0].geometry.coordinates, expectedPoint); } done(); }); From e59d0f520a40d50531ef77b799d198839b4803b8 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 16 Jul 2018 12:02:37 +0200 Subject: [PATCH 07/11] Restore tests --- test/acceptance/aggregation.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index b37466f6..73a6ec11 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -2375,7 +2375,7 @@ describe('aggregation', function () { }); }); - it.only(`for ${placement} each aggr. cell is in a single tile`, function (done) { + it(`for ${placement} each aggr. cell is in a single tile`, function (done) { this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 0 }, @@ -2496,7 +2496,7 @@ describe('aggregation', function () { }); }); - it.only(`for ${placement} points aggregated into correct cluster`, function (done) { + it(`for ${placement} points aggregated into correct cluster`, function (done) { this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 0 }, From 2d102c48108cd0c5b9c50b2757af5e50270889b3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 16 Jul 2018 12:44:30 +0200 Subject: [PATCH 08/11] Compensate for bbox lack of accuracy --- lib/cartodb/models/aggregation/aggregation-query.js | 13 ++++++++----- test/acceptance/aggregation.js | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index e7eb2ecb..4f7060c5 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -274,20 +274,23 @@ const spatialFilter = ` // * This queries are used for rendering and the_geom is omitted in the results for better performance // * If the MVT extent or tile buffer was 0 or a multiple of the resolution we could use directly // the bbox for them, but in general we need to find the nearest cell limits inside the bbox. +// * bbox coordinates can have an error in the last digites; we apply a small correction before +// applying CEIL or FLOOR to compensate for this. const sqlParams = (ctx) => ` _cdb_res AS ( SELECT ${gridResolution(ctx)} AS res, - !bbox! AS bbox + !bbox! AS bbox, + (2*2.220446049250313e-16::double precision) AS eps ), _cdb_params AS ( SELECT res, bbox, - CEIL(ST_XMIN(bbox)/res)*res AS xmin, - FLOOR(ST_XMAX(bbox)/res)*res AS xmax, - CEIL(ST_YMIN(bbox)/res)*res AS ymin, - FLOOR(ST_YMAX(bbox)/res)*res AS ymax + CEIL((ST_XMIN(bbox) - eps*ABS(ST_XMIN(bbox)))/res)*res AS xmin, + FLOOR((ST_XMAX(bbox) + eps*ABS(ST_XMAX(bbox)))/res)*res AS xmax, + CEIL((ST_YMIN(bbox) - eps*ABS(ST_YMIN(bbox)))/res)*res AS ymin, + FLOOR((ST_YMAX(bbox) + eps*ABS(ST_YMAX(bbox)))/res)*res AS ymax FROM _cdb_res ) `; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 73a6ec11..8fd9029f 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -2455,7 +2455,7 @@ describe('aggregation', function () { }); }); - it.skip(`for ${placement} points aggregated into corner cluster`, function (done) { + it(`for ${placement} points aggregated into corner cluster`, function (done) { // this test will fail due to !bbox! lack of accuracy if strict cell filtering is in place this.mapConfig = { version: '1.6.0', From e1ec38446defee09033aeff57c488184e88f1335 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 16 Jul 2018 14:27:39 +0200 Subject: [PATCH 09/11] Add aggregation test with buffer overlapping cells partially Also fix resolution parameter incrrectly placed in some tests --- test/acceptance/aggregation.js | 113 +++++++++++++++++++++++++++------ 1 file changed, 95 insertions(+), 18 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 8fd9029f..5ca05e90 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -1245,9 +1245,9 @@ describe('aggregation', function () { type: 'cartodb', options: { sql: POINTS_SQL_1, - resolution: 256, aggregation: { - threshold: 1 + threshold: 1, + resolution: 256 } } } @@ -2256,9 +2256,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_0, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 } } } @@ -2385,9 +2385,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_GRID, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 } } } @@ -2455,6 +2455,83 @@ describe('aggregation', function () { }); }); + it(`for ${placement} no partially aggregated cells`, function (done) { + // Use level 1 with resolution 2 tiles and buffersize 1 (half the cell size) + // Only the cells completely inside the buffer are aggregated + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 1 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_GRID, + aggregation: { + threshold: 1, + resolution: 2 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + this.testClient.getTile(1, 0, 0, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile00 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 0, 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile01 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 1, 0, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile10 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(1, 1, 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile11 = JSON.parse(mvt.toGeoJSONSync(0)); + + const tile00Expected = [ + { cartodb_id: 4, _cdb_feature_count: 3 } + ]; + const tile10Expected = [ + { cartodb_id: 5, _cdb_feature_count: 5 } + ]; + const tile01Expected = [ + { cartodb_id: 1, _cdb_feature_count: 2 } + ]; + const tile11Expected = [ + { cartodb_id: 2, _cdb_feature_count: 3 } + ]; + const tile00Actual = tile00.features.map(f => f.properties); + const tile10Actual = tile10.features.map(f => f.properties); + const tile01Actual = tile01.features.map(f => f.properties); + const tile11Actual = tile11.features.map(f => f.properties); + const orderById = (a, b) => a.cartodb_id - b.cartodb_id; + assert.deepEqual(tile00Actual.sort(orderById), tile00Expected); + assert.deepEqual(tile10Actual.sort(orderById), tile10Expected); + assert.deepEqual(tile01Actual.sort(orderById), tile01Expected); + assert.deepEqual(tile11Actual.sort(orderById), tile11Expected); + + done(); + }); + }); + }); + + }); + }); + it(`for ${placement} points aggregated into corner cluster`, function (done) { // this test will fail due to !bbox! lack of accuracy if strict cell filtering is in place this.mapConfig = { @@ -2466,9 +2543,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_CELL, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 } } } @@ -2506,9 +2583,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_CELL_INNER, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 } } } @@ -2549,9 +2626,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_PAIRS, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 } } } @@ -2587,9 +2664,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_PAIRS, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 }, metadata: { aggrFeatureCount: 10 @@ -2628,9 +2705,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_PAIRS, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 }, metadata: { aggrFeatureCount: 0 @@ -2669,9 +2746,9 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_PAIRS, - resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + resolution: 1 }, metadata: { featureCount: true From e5d43692037e7e5b590196b602a09f046904d918 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 16 Jul 2018 14:59:26 +0200 Subject: [PATCH 10/11] Fix test --- test/acceptance/aggregation.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 5ca05e90..d0d272e5 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -1246,8 +1246,7 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_1, aggregation: { - threshold: 1, - resolution: 256 + threshold: 1 } } } From 51c3215137ff5f33d3090a1e88e6831c7703a1fa Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 17 Jul 2018 12:42:33 +0200 Subject: [PATCH 11/11] Change UNION by UNION ALL in tests It seems UNION (without ALL) can be much slower in recent PG versions --- test/acceptance/aggregation.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index d0d272e5..17944029 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -178,7 +178,7 @@ describe('aggregation', function () { ST_SetSRID(ST_MakePoint(x*l, y*l), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(x*l, y*l), 3857), 4326) AS the_geom FROM params, generate_series(-1,1) x, generate_series(-1,1) y - UNION + UNION ALL SELECT row_number() OVER () + 9 AS cartodb_id, ST_SetSRID(ST_MakePoint(x*l-l/2, y*l-l/2), 3857) AS the_geom_webmercator, @@ -193,15 +193,15 @@ describe('aggregation', function () { 1 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 2 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181005.9, -18181044.0), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181005.9, -18181044.0), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 3 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181005.87, -18181043.94), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181005.87, -18181043.94), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 4 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181005.8, -18181043.9), 3857), 4326) AS the_geom @@ -214,15 +214,15 @@ describe('aggregation', function () { 1 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181005.95, -18181043.8), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181005.95, -18181043.8), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 2 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181006.09, -18181043.72), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181006.09, -18181043.72), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 3 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181006.02, -18181043.79), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181006.02, -18181043.79), 3857), 4326) AS the_geom - UNION SELECT + UNION ALL SELECT 4 AS cartodb_id, ST_SetSRID(ST_MakePoint(18181006.01, -18181043.75), 3857) AS the_geom_webmercator, ST_Transform(ST_SetSRID(ST_MakePoint(18181006.01, -18181043.75), 3857), 4326) AS the_geom