From 5d813b6e43d37f5fa9d1b90ce15e99b6cb10d00c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 12 Sep 2018 19:26:09 +0200 Subject: [PATCH 1/3] Add more tests for aggregation accuracy Some tests are skipped for Mapnik MVTs, because its bbox filtering isn't accurate --- test/acceptance/aggregation.js | 151 ++++++++++++++++++++++++++++++--- 1 file changed, 138 insertions(+), 13 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 4053054e..c86b1e5f 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -171,7 +171,7 @@ describe('aggregation', function () { // const POINTS_SQL_GRID = ` WITH params AS ( - SELECT CDB_XYZ_Resolution(1) AS l -- cell size for Z=1 res=1 + SELECT CDB_XYZ_Resolution($Z)*$resolution AS l -- cell size for Z, resolution ) SELECT row_number() OVER () AS cartodb_id, @@ -2380,6 +2380,9 @@ describe('aggregation', function () { }); it(`for ${placement} each aggr. cell is in a single tile`, function (done) { + const z = 1; + const resolution = 1; + const query = POINTS_SQL_GRID.replace('$Z', z).replace('$resolution', resolution); this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 0 }, @@ -2388,10 +2391,10 @@ describe('aggregation', function () { type: 'cartodb', options: { - sql: POINTS_SQL_GRID, + sql: query, aggregation: { threshold: 1, - resolution: 1 + resolution: resolution } } } @@ -2403,22 +2406,24 @@ describe('aggregation', function () { this.testClient = new TestClient(this.mapConfig); - this.testClient.getTile(1, 0, 0, { format: 'mvt' }, (err, res, mvt) => { + const c = Math.pow(2, z - 1) - 1; // center tile coordinates + + this.testClient.getTile(z, c + 0, c + 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) => { + this.testClient.getTile(z, c + 0, c + 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) => { + this.testClient.getTile(z, c + 1, c + 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) => { + this.testClient.getTile(z, c + 1, c + 1, { format: 'mvt' }, (err, res, mvt) => { if (err) { return done(err); } @@ -2462,6 +2467,11 @@ 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 + const z = 1; + const resolution = 2; + // space the test points by half the resolution: + const query = POINTS_SQL_GRID.replace('$Z', z).replace('$resolution', resolution/2); + this.mapConfig = { version: '1.6.0', buffersize: { 'mvt': 1 }, @@ -2470,10 +2480,10 @@ describe('aggregation', function () { type: 'cartodb', options: { - sql: POINTS_SQL_GRID, + sql: query, aggregation: { threshold: 1, - resolution: 2 + resolution: resolution } } } @@ -2485,22 +2495,24 @@ describe('aggregation', function () { this.testClient = new TestClient(this.mapConfig); - this.testClient.getTile(1, 0, 0, { format: 'mvt' }, (err, res, mvt) => { + const c = Math.pow(2, z - 1) - 1; // center tile coordinates + + this.testClient.getTile(z, c, c, { 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) => { + this.testClient.getTile(z, c, c + 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) => { + this.testClient.getTile(z, c + 1, c, { 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) => { + this.testClient.getTile(z, c + 1, c + 1, { format: 'mvt' }, (err, res, mvt) => { if (err) { return done(err); } @@ -2536,6 +2548,119 @@ describe('aggregation', function () { }); }); + it(`for ${placement} includes complete cells in buffer`, function (done) { + if (!usePostGIS && placement !== 'point-grid') { + // Mapnik seem to filter query results by its (inaccurate) bbox, + // which makes some aggregated clusters get lost here. + // The point-grid placement is resilient to this problem because the result + // coordinates are moved to cluster cell centers, so they're well within + // bbox limits. + this.testClient = new TestClient({}); + return done(); + } + + // use buffersize coincident with resolution, the buffer should include neighbour cells + const z = 2; + const resolution = 1; + const query = POINTS_SQL_GRID.replace('$Z', z).replace('$resolution', resolution); + + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 1 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: query, + aggregation: { + threshold: 1, + resolution: resolution + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.mapConfig); + + const c = Math.pow(2, z - 1) - 1; // center tile coordinates + + this.testClient.getTile(z, c, c, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile00 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(z, c, c + 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile01 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(z, c + 1, c, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile10 = JSON.parse(mvt.toGeoJSONSync(0)); + this.testClient.getTile(z, c + 1, c + 1, { format: 'mvt' }, (err, res, mvt) => { + if (err) { + return done(err); + } + const tile11 = JSON.parse(mvt.toGeoJSONSync(0)); + + const tile00Expected = [ + { _cdb_feature_count: 2, cartodb_id: 1 }, + { _cdb_feature_count: 2, cartodb_id: 2 }, + { _cdb_feature_count: 2, cartodb_id: 4 }, + { _cdb_feature_count: 2, cartodb_id: 5 }, + { _cdb_feature_count: 1, cartodb_id: 7 }, + { _cdb_feature_count: 1, cartodb_id: 8 } + ]; + const tile10Expected = [ + { _cdb_feature_count: 2, cartodb_id: 1 }, + { _cdb_feature_count: 2, cartodb_id: 2 }, + { _cdb_feature_count: 1, cartodb_id: 3 }, + { _cdb_feature_count: 2, cartodb_id: 4 }, + { _cdb_feature_count: 2, cartodb_id: 5 }, + { _cdb_feature_count: 1, cartodb_id: 6 }, + { _cdb_feature_count: 1, cartodb_id: 7 }, + { _cdb_feature_count: 1, cartodb_id: 8 }, + { _cdb_feature_count: 1, cartodb_id: 9 } + ]; + const tile01Expected = [ + { _cdb_feature_count: 2, cartodb_id: 1 }, + { _cdb_feature_count: 2, cartodb_id: 2 }, + { _cdb_feature_count: 2, cartodb_id: 4 }, + { _cdb_feature_count: 2, cartodb_id: 5 } + ]; + const tile11Expected = [ + { _cdb_feature_count: 2, cartodb_id: 1 }, + { _cdb_feature_count: 2, cartodb_id: 2 }, + { _cdb_feature_count: 1, cartodb_id: 3 }, + { _cdb_feature_count: 2, cartodb_id: 4 }, + { _cdb_feature_count: 2, cartodb_id: 5 }, + { _cdb_feature_count: 1, cartodb_id: 6 } + ]; + 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 = { From 0f20cdaae18a7303a483edfac1fb2471e9ac3b34 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 12 Sep 2018 19:27:21 +0200 Subject: [PATCH 2/3] More robust adjustment of spatial limits to the aggregation grid See #1034 --- .../models/aggregation/aggregation-query.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js index 4d964171..a455e02a 100644 --- a/lib/cartodb/models/aggregation/aggregation-query.js +++ b/lib/cartodb/models/aggregation/aggregation-query.js @@ -276,23 +276,24 @@ 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. +// * bbox coordinates can have an error in the last digits; we apply a small correction before +// applying CEIL or FLOOR to compensate for this, so that coordinates closer than a small (`eps`) +// fraction of the cell size to a cell limit are moved to the exact limit. const sqlParams = (ctx) => ` _cdb_res AS ( SELECT ${gridResolution(ctx)} AS res, !bbox! AS bbox, - (2*2.220446049250313e-16::double precision) AS eps + (1E-6::double precision) AS eps ), _cdb_params AS ( SELECT res, bbox, - 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 + CEIL((ST_XMIN(bbox) - eps*res)/res)*res AS xmin, + FLOOR((ST_XMAX(bbox) + eps*res)/res)*res AS xmax, + CEIL((ST_YMIN(bbox) - eps*res)/res)*res AS ymin, + FLOOR((ST_YMAX(bbox) + eps*res)/res)*res AS ymax FROM _cdb_res ) `; From 6ba9e50da741e8c012d8eb30ca3fb2071b2845b0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 13 Sep 2018 08:54:59 +0200 Subject: [PATCH 3/3] Minor tests refactor --- test/acceptance/aggregation.js | 78 ++++++++++++---------------------- 1 file changed, 26 insertions(+), 52 deletions(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index c86b1e5f..39fb7845 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -1648,7 +1648,8 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_1, aggregation: { - threshold: 1 + threshold: 1, + placement: placement !== 'default' ? placement : undefined }, cartocss: '#layer { marker-width: 1; }', cartocss_version: '2.3.0', @@ -1656,9 +1657,6 @@ describe('aggregation', function () { } } ]); - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -1683,14 +1681,12 @@ describe('aggregation', function () { options: { sql: POINTS_SQL_ONLY_WEBMERCATOR, aggregation: { - threshold: 1 + threshold: 1, + placement: placement !== 'default' ? placement : undefined } } } ]); - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -2262,15 +2258,13 @@ describe('aggregation', function () { sql: POINTS_SQL_0, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2317,15 +2311,13 @@ describe('aggregation', function () { sql: POINTS_SQL_1, resolution: 1, aggregation: { - threshold: 1 + threshold: 1, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2394,15 +2386,13 @@ describe('aggregation', function () { sql: query, aggregation: { threshold: 1, - resolution: resolution + resolution: resolution, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2483,15 +2473,13 @@ describe('aggregation', function () { sql: query, aggregation: { threshold: 1, - resolution: resolution + resolution: resolution, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2575,15 +2563,13 @@ describe('aggregation', function () { sql: query, aggregation: { threshold: 1, - resolution: resolution + resolution: resolution, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2674,15 +2660,13 @@ describe('aggregation', function () { sql: POINTS_SQL_CELL, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2714,15 +2698,13 @@ describe('aggregation', function () { sql: POINTS_SQL_CELL_INNER, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); @@ -2757,15 +2739,13 @@ describe('aggregation', function () { sql: POINTS_SQL_PAIRS, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined } } } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -2795,7 +2775,8 @@ describe('aggregation', function () { sql: POINTS_SQL_PAIRS, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined }, metadata: { aggrFeatureCount: 10 @@ -2804,9 +2785,6 @@ describe('aggregation', function () { } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -2836,7 +2814,8 @@ describe('aggregation', function () { sql: POINTS_SQL_PAIRS, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined }, metadata: { aggrFeatureCount: 0 @@ -2845,9 +2824,6 @@ describe('aggregation', function () { } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => { @@ -2877,7 +2853,8 @@ describe('aggregation', function () { sql: POINTS_SQL_PAIRS, aggregation: { threshold: 1, - resolution: 1 + resolution: 1, + placement: placement !== 'default' ? placement : undefined }, metadata: { featureCount: true @@ -2886,9 +2863,6 @@ describe('aggregation', function () { } ] }; - if (placement !== 'default') { - this.mapConfig.layers[0].options.aggregation.placement = placement; - } this.testClient = new TestClient(this.mapConfig); this.testClient.getLayergroup((err, body) => {