From c5f5f43ccbc77fd4ae23d9a634ea5eb9752e9dd0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 5 Mar 2019 22:17:04 +0100 Subject: [PATCH 1/5] Fix test for disabling aggregations See #1073 --- test/acceptance/aggregation.js | 6 ++++- test/support/sql/windshaft.test.sql | 41 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index d9924eee..731a9c88 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -92,6 +92,8 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; + const POINTS_OVER_THRESHOLD = 'SELECT * FROM test_table_200k'; + const POLYGONS_SQL_1 = ` select x + 4 as cartodb_id, @@ -666,7 +668,9 @@ describe('aggregation', function () { { type: 'cartodb', options: { - sql: POINTS_SQL_2, + // Note that we need the table to have more than AggregationMapConfig.THRESHOLD rows + // otherwise it won't get aggregated in any case + sql: POINTS_OVER_THRESHOLD, aggregation: false } } diff --git a/test/support/sql/windshaft.test.sql b/test/support/sql/windshaft.test.sql index 3c957db6..3a23171b 100644 --- a/test/support/sql/windshaft.test.sql +++ b/test/support/sql/windshaft.test.sql @@ -819,5 +819,46 @@ CREATE INDEX test_table_100_the_geom_webmercator_idx ON test_table_100 USING gis GRANT ALL ON TABLE test_table_100 TO :TESTUSER; GRANT SELECT ON TABLE test_table_100 TO :PUBLICUSER; +-- Table with 200K rows +CREATE TABLE test_table_200k ( + cartodb_id integer NOT NULL, + value int, + the_geom geometry, + the_geom_webmercator geometry, + CONSTRAINT enforce_dims_the_geom CHECK ((st_ndims(the_geom) = 2)), + CONSTRAINT enforce_dims_the_geom_webmercator CHECK ((st_ndims(the_geom_webmercator) = 2)), + CONSTRAINT enforce_geotype_the_geom CHECK (((geometrytype(the_geom) = 'POINT'::text) OR (the_geom IS NULL))), + CONSTRAINT enforce_geotype_the_geom_webmercator CHECK (((geometrytype(the_geom_webmercator) = 'POINT'::text) OR (the_geom_webmercator IS NULL))), + CONSTRAINT enforce_srid_the_geom CHECK ((st_srid(the_geom) = 4326)), + CONSTRAINT enforce_srid_the_geom_webmercator CHECK ((st_srid(the_geom_webmercator) = 3857)) +); + +CREATE SEQUENCE test_table_200k_cartodb_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE test_table_200k_cartodb_id_seq OWNED BY test_table_200k.cartodb_id; + +SELECT pg_catalog.setval('test_table_200k_cartodb_id_seq', 60, true); + +ALTER TABLE test_table_200k ALTER COLUMN cartodb_id SET DEFAULT nextval('test_table_200k_cartodb_id_seq'::regclass); + +INSERT INTO test_table_200k(the_geom, the_geom_webmercator, value) + SELECT + ST_SetSRID(ST_MakePoint(n*1E-4 + 9E-3, n*1E-4 + 9E-3), 4326) AS the_geom, + ST_Transform(ST_SetSRID(ST_MakePoint(n*1E-4 + 9E-3, n*1E-4 + 9E-3), 4326), 3857) AS the_geom_webmercator, + n AS value + FROM generate_series(1, 200000) n; + +ALTER TABLE ONLY test_table_200k ADD CONSTRAINT test_table_200k_pkey PRIMARY KEY (cartodb_id); + +CREATE INDEX test_table_200k_the_geom_idx ON test_table_200k USING gist (the_geom); +CREATE INDEX test_table_200k_the_geom_webmercator_idx ON test_table_200k USING gist (the_geom_webmercator); + +GRANT ALL ON TABLE test_table_200k TO :TESTUSER; +GRANT SELECT ON TABLE test_table_200k TO :PUBLICUSER; ANALYZE; From cf193a71b2f7a382aecb540404b303f8041bc93e Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 6 Mar 2019 15:28:38 +0100 Subject: [PATCH 2/5] Add tests for enabling default aggregation --- test/acceptance/aggregation.js | 60 ++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 731a9c88..a675aa39 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -694,6 +694,66 @@ describe('aggregation', function () { }); }); + it('use default aggregation by setting `aggregation: true`', function (done) { + const mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_OVER_THRESHOLD, + cartocss: '#layer { marker-width: 7; }', + cartocss_version: '2.3.0', + aggregation: true + } + } + ]); + + this.testClient = new TestClient(mapConfig); + + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + assert.equal(body.metadata.layers[0].meta.aggregation.mvt, true); + assert.equal(body.metadata.layers[0].meta.aggregation.png, true); + + done(); + }); + }); + + it('but do not aggregate below threshold by setting `aggregation: true`', function (done) { + const mapConfig = createVectorMapConfig([ + { + type: 'cartodb', + options: { + sql: POINTS_SQL_2, + cartocss: '#layer { marker-width: 7; }', + cartocss_version: '2.3.0', + aggregation: true + } + } + ]); + + this.testClient = new TestClient(mapConfig); + + this.testClient.getLayergroup((err, body) => { + if (err) { + return done(err); + } + + assert.equal(typeof body.metadata, 'object'); + assert.ok(Array.isArray(body.metadata.layers)); + + assert.equal(body.metadata.layers[0].meta.aggregation.mvt, false); + assert.equal(body.metadata.layers[0].meta.aggregation.png, false); + + done(); + }); + }); + it('when the aggregation param is not valid should respond with error', function (done) { const mapConfig = createVectorMapConfig([ { From 113b3728b15366bdbf1ed754f6e6563d8ccadb23 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 6 Mar 2019 18:14:15 +0100 Subject: [PATCH 3/5] Fix support of boolean aggregation layer option --- .../aggregation/aggregation-mapconfig.js | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index abd53fe3..0b533acc 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -49,6 +49,10 @@ module.exports = class AggregationMapConfig extends MapConfig { ]; } + static get HAS_AGGREGATION_DISABLED () { + return null; + } + static supportsGeometryType(geometryType) { return AggregationMapConfig.SUPPORTED_GEOMETRY_TYPES.includes(geometryType); } @@ -103,19 +107,17 @@ module.exports = class AggregationMapConfig extends MapConfig { }); } - isAggregationMapConfig () { - return this.isVectorOnlyMapConfig() || this.hasAnyLayerAggregation(); - } - isAggregationLayer (index) { - return this.isVectorOnlyMapConfig() || this.hasLayerAggregation(index); + let hasAggregation = this.hasLayerAggregation(index); + // for vector-only MapConfig are aggregated unless explicitly disabled + return hasAggregation || (this.isVectorOnlyMapConfig() && hasAggregation !== AggregationMapConfig.HAS_AGGREGATION_DISABLED); } - hasAnyLayerAggregation () { + isAggregationMapConfig () { const layers = this.getLayers(); for (let index = 0; index < layers.length; index++) { - if (this.hasLayerAggregation(index)) { + if (this.isAggregationLayer(index)) { return true; } } @@ -123,22 +125,32 @@ module.exports = class AggregationMapConfig extends MapConfig { return false; } + /* Three possible return values: + * * true: explicit aggregation ({}, true) + * * false: no aggregation (undefined) + * * AggregationMapConfig.HAS_AGGREGATION_DISABLED: explicitly disabled aggregation (false) + */ hasLayerAggregation (index) { const layer = this.getLayer(index); const { aggregation } = layer.options; + if (aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean')) { + if (aggregation === false) { + return AggregationMapConfig.HAS_AGGREGATION_DISABLED; + } + return true; + } + return false; return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } getAggregation (index) { - if (this.isVectorOnlyMapConfig() && !this.hasLayerAggregation(index)) { - return {}; - } - const { aggregation } = this.getLayer(index).options; - if (typeof aggregation === 'boolean') { - return {}; + if (this.isAggregationLayer(index)) { + if (typeof aggregation === 'boolean' || !aggregation) { + return {}; + } } return aggregation; @@ -221,10 +233,7 @@ module.exports = class AggregationMapConfig extends MapConfig { _isDefaultLayerAggregation (index) { const aggregation = this.getAggregation(index); - - return (this.isVectorOnlyMapConfig() && !this.hasLayerAggregation(index)) || - aggregation === true || - this._isDefaultAggregation(aggregation); + return aggregation && this._isDefaultAggregation(aggregation); } _isDefaultAggregation (aggregation) { From c91b28ee92310168291c3970440ab1bc1036c6d8 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 6 Mar 2019 18:32:17 +0100 Subject: [PATCH 4/5] Lint fixes --- lib/cartodb/models/aggregation/aggregation-mapconfig.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js index 0b533acc..34b01859 100644 --- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js +++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js @@ -110,7 +110,9 @@ module.exports = class AggregationMapConfig extends MapConfig { isAggregationLayer (index) { let hasAggregation = this.hasLayerAggregation(index); // for vector-only MapConfig are aggregated unless explicitly disabled - return hasAggregation || (this.isVectorOnlyMapConfig() && hasAggregation !== AggregationMapConfig.HAS_AGGREGATION_DISABLED); + return hasAggregation || ( + this.isVectorOnlyMapConfig() && hasAggregation !== AggregationMapConfig.HAS_AGGREGATION_DISABLED + ); } isAggregationMapConfig () { @@ -140,8 +142,6 @@ module.exports = class AggregationMapConfig extends MapConfig { return true; } return false; - - return aggregation !== undefined && (typeof aggregation === 'object' || typeof aggregation === 'boolean'); } getAggregation (index) { From ab9cfee7b1eb7e7c722732f9adaa35ccae051836 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 14 Mar 2019 17:02:23 +0100 Subject: [PATCH 5/5] Update NEWS --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index b3a26e19..ea605fbe 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,7 @@ Released 2019-mm-dd Announcements: - Experimental support for listing features in a grid when the map uses the dynamic agregation. - Numeric histogram performance improvement (#1080) +- Fix boolean aggregation layer option not working when numbers of rows are above the threshold (#1082) - Update deps: - windshaft@4.13.3: Upgrade grainstore to version 1.11.0, do not hang when child process is not able to generate a Mapnik XML