From 7b731a24d17244dcf447d094a2538fe6d1a2349e Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Wed, 26 Jun 2019 16:52:16 +0200
Subject: [PATCH 01/19] Overviews Adapter: Do not check overviews if using MVT
 pure style or aggregations

---
 .../models/mapconfig/adapter/mapconfig-overviews-adapter.js  | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/cartodb/models/mapconfig/adapter/mapconfig-overviews-adapter.js b/lib/cartodb/models/mapconfig/adapter/mapconfig-overviews-adapter.js
index 3d0f6580..c2d9b784 100644
--- a/lib/cartodb/models/mapconfig/adapter/mapconfig-overviews-adapter.js
+++ b/lib/cartodb/models/mapconfig/adapter/mapconfig-overviews-adapter.js
@@ -2,6 +2,7 @@
 
 var queue = require('queue-async');
 var _ = require('underscore');
+const AggregationMapConfig = require('../../aggregation/aggregation-mapconfig');
 
 function MapConfigOverviewsAdapter(overviewsMetadataBackend, filterStatsBackend) {
     this.overviewsMetadataBackend = overviewsMetadataBackend;
@@ -14,7 +15,9 @@ MapConfigOverviewsAdapter.prototype.getMapConfig = function (user, requestMapCon
     var layers = requestMapConfig.layers;
     var analysesResults = context.analysesResults;
 
-    if (!layers || layers.length === 0) {
+    const aggMapConfig = new AggregationMapConfig(null, requestMapConfig);
+    if (aggMapConfig.isVectorOnlyMapConfig() || aggMapConfig.isAggregationMapConfig() ||
+        !layers || layers.length === 0) {
         return callback(null, requestMapConfig);
     }
 

From cd8624ae2d7e090fccd7af14eedbfc8e60cc418e Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Wed, 26 Jun 2019 18:03:02 +0200
Subject: [PATCH 02/19] Improve subquery naming

---
 lib/cartodb/backends/cluster.js                         | 2 +-
 lib/cartodb/models/aggregation/aggregation-mapconfig.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js
index ab663997..87c67539 100644
--- a/lib/cartodb/backends/cluster.js
+++ b/lib/cartodb/backends/cluster.js
@@ -127,7 +127,7 @@ function getClusterFeatures (pg, zoom, clusterId, columns, query, resolution, ag
     } , true); // use read-only transaction
 }
 
-const schemaQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`;
+const schemaQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_cluster_schema LIMIT 0`;
 const clusterFeaturesQuery = ctx => `
     WITH
     _cdb_params AS (
diff --git a/lib/cartodb/models/aggregation/aggregation-mapconfig.js b/lib/cartodb/models/aggregation/aggregation-mapconfig.js
index 34b01859..74099f1e 100644
--- a/lib/cartodb/models/aggregation/aggregation-mapconfig.js
+++ b/lib/cartodb/models/aggregation/aggregation-mapconfig.js
@@ -201,7 +201,7 @@ module.exports = class AggregationMapConfig extends MapConfig {
 
     getLayerColumns (index, skipGeoms, callback) {
         const geomColumns = ['the_geom', 'the_geom_webmercator'];
-        const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_schema LIMIT 0`;
+        const limitedQuery = ctx => `SELECT * FROM (${ctx.query}) __cdb_aggregation_schema LIMIT 0`;
         const layer = this.getLayer(index);
 
         this.pgConnection.getConnection(this.user, (err, connection) => {

From a3e8f45552b2e9df4e0208c872546156d82e8306 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Fri, 28 Jun 2019 13:50:26 +0200
Subject: [PATCH 03/19] WIP to change how aggregations are calculated

---
 lib/cartodb/backends/cluster.js               |   3 +-
 .../models/aggregation/aggregation-query.js   | 190 ++++++------------
 2 files changed, 65 insertions(+), 128 deletions(-)

diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js
index 87c67539..da468177 100644
--- a/lib/cartodb/backends/cluster.js
+++ b/lib/cartodb/backends/cluster.js
@@ -157,8 +157,7 @@ const clusterFeaturesQuery = ctx => `
 
 const gridResolution = ctx => {
     const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38);
-    const pixelSize = `CDB_XYZ_Resolution(${ctx.zoom})`;
-    return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`;
+    return `${256/ctx.res} * GREATEST(!pixel_height!, ${minimumResolution})::numeric`;
 };
 
 const aggregationQuery = ctx => `
diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index f74df93a..03e9caf2 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -35,7 +35,8 @@ function optionsToParams (options) {
         res: 256/options.resolution,
         columns: options.columns,
         dimensions: options.dimensions,
-        filters: options.filters
+        filters: options.filters,
+        placement: options.placement || DEFAULT_PLACEMENT
     };
 }
 
@@ -297,31 +298,24 @@ const havingClause = ctx => {
 // (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);
-    const pixelSize = 'CDB_XYZ_Resolution(CDB_ZoomFromScale(!scale_denominator!))';
-    return `GREATEST(${256/ctx.res}*${pixelSize}, ${minimumResolution})::double precision`;
+    return `${256/ctx.res} * GREATEST(!pixel_height!, ${minimumResolution})::numeric`;
 };
 
-// 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
-`;
+const aggregatedPoint = ctx => {
+    const placement = ctx.placement || DEFAULT_PLACEMENT;
+    switch (placement) {
+        case `centroid`:
+            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+        case `point-grid`:
+            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+        case `point-sample`:
+            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+        default:
+            throw new Error(`Invalid aggregation placement "${placement}`);
+    }
+};
 
 // Notes:
 // * We need to filter spatially using !bbox! to make the queries efficient because
@@ -333,116 +327,60 @@ const spatialFilter = `
 // * 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,
-    (1E-6::double precision) AS eps
-),
-_cdb_params AS (
-    SELECT
-      res,
-      bbox,
-      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
-)
-`;
 
-// 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 ${sqlParams(ctx)},
-    _cdb_clusters AS (
-        SELECT
-            MIN(cartodb_id) AS cartodb_id
-            ${dimensionDefs(ctx)}
-            ${aggregateColumnDefs(ctx)}
-        FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params
-        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)
-            ${dimensionNames(ctx)}
-    ) SELECT
-        _cdb_query.*
-        ${aggregateColumnNames(ctx)}
+SELECT
+    min(cartodb_id) as cartodb_id,
+    ${aggregatedPoint(ctx)} AS the_geom_webmercator
+    ${dimensionDefs(ctx)}
+    ${aggregateColumnDefs(ctx)}
+FROM
+(
+    SELECT
+        __cdb_src_query.*,
+        cdb_limit_x,
+        cdb_limit_y,
+        ((ST_X(the_geom_webmercator) - __cdb_src_params.cdb_xmin) / __cdb_src_params.res)::int as cdb_pos_grid_x,
+        ((ST_Y(the_geom_webmercator) - __cdb_src_params.cdb_ymin) / __cdb_src_params.res)::int as cdb_pos_grid_y
     FROM
-        _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query
-        ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id)
+    (
+        ${ctx.sourceQuery}
+    ) __cdb_src_query,
+    (
+        SELECT
+            _cdb_grid_bbox_margins.*,
+            ST_MakeEnvelope(cdb_xmin, cdb_ymin, cdb_xmax, cdb_ymax, 3857) AS cdb_point_bbox,
+            ROUND((cdb_xmax - cdb_xmin) /  res)::int as cdb_limit_x,
+            ROUND((cdb_ymax - cdb_ymin) /  res)::int as cdb_limit_y
+        FROM
+        (
+            SELECT
+                res,
+                CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
+                FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
+                CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
+                FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
+            FROM
+            (
+                SELECT
+                    ${gridResolution(ctx)} AS res,
+                    !bbox! cdb_full_bbox
+                OFFSET 0
+            ) _cdb_input_resources
+        ) _cdb_grid_bbox_margins OFFSET 0
+    ) __cdb_src_params
+    WHERE the_geom_webmercator && cdb_point_bbox OFFSET 0
+) __cdb_srd_grid
+WHERE cdb_pos_grid_x < cdb_limit_x AND cdb_pos_grid_y < cdb_limit_y
+GROUP BY cdb_pos_grid_x, cdb_pos_grid_y ${dimensionNames(ctx)}
+${havingClause(ctx)}
 `;
 
 const aggregationQueryTemplates = {
-    'centroid': ctx => `
-        WITH ${sqlParams(ctx)}
-        SELECT
-            MIN(_cdb_query.cartodb_id)  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
-            ${dimensionDefs(ctx)}
-            ${aggregateColumnDefs(ctx)}
-        FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params
-        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)
-            ${dimensionNames(ctx)}
-        ${havingClause(ctx)}
-    `,
-
-    'point-grid': ctx => `
-        WITH ${sqlParams(ctx)},
-        _cdb_clusters AS (
-            SELECT
-            MIN(_cdb_query.cartodb_id) AS cartodb_id,
-            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
-            ${dimensionDefs(ctx)}
-            ${aggregateColumnDefs(ctx)}
-            FROM (${ctx.sourceQuery}) _cdb_query, _cdb_params
-            WHERE ${spatialFilter}
-            GROUP BY _cdb_gx, _cdb_gy ${dimensionNames(ctx)}
-            ${havingClause(ctx)}
-        )
-        SELECT
-            _cdb_clusters.cartodb_id AS cartodb_id,
-            ST_SetSRID(ST_MakePoint((_cdb_gx+0.5)*res, (_cdb_gy+0.5)*res), 3857) AS the_geom_webmercator
-            ${dimensionNames(ctx)}
-            ${aggregateColumnNames(ctx)}
-            FROM _cdb_clusters, _cdb_params
-    `,
-
-    'point-sample': ctx => `
-        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 ${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)
-                ${dimensionNames(ctx)}
-            ${havingClause(ctx)}
-        )
-        SELECT
-            _cdb_clusters.cartodb_id,
-            the_geom_webmercator
-            ${dimensionNames(ctx, '_cdb_clusters')}
-            ${aggregateColumnNames(ctx, '_cdb_clusters')}
-        FROM
-            _cdb_clusters INNER JOIN (${ctx.sourceQuery}) _cdb_query
-            ON (_cdb_clusters.cartodb_id = _cdb_query.cartodb_id)
-    `
+    'centroid': defaultAggregationQueryTemplate,
+    'point-grid': defaultAggregationQueryTemplate,
+    'point-sample': defaultAggregationQueryTemplate
 
 };
 

From de38f1f6fd00c54e27164b762345f6f7f6d43a9c Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Wed, 3 Jul 2019 15:14:01 +0200
Subject: [PATCH 04/19] Tests: Avoid using st_buffer(geography)

It's really slow with PROJ6 since it involves creating different projections for almost each point,
and that's particularly slow in the new release
---
 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 39508837..3dcac93c 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -96,9 +96,9 @@ 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_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326), 10) as the_geom,
         st_transform(
-            st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326)::geography, 100000)::geometry,
+            st_buffer(st_setsrid(st_makepoint(x*10, x*10), 4326), 10),
             3857
         ) as the_geom_webmercator,
         x as value

From 5e24f650af5f0027bedad360e5f8c728a8d25b40 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Wed, 3 Jul 2019 16:13:54 +0200
Subject: [PATCH 05/19] Rework how aggregations are calculated

Pending fixing Mapnik tiles (pg-mvt work ok)
---
 .../models/aggregation/aggregation-query.js   | 114 ++++++++++++------
 test/acceptance/aggregation.js                |  91 +++++++-------
 2 files changed, 115 insertions(+), 90 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 03e9caf2..9e9ff805 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -36,7 +36,8 @@ function optionsToParams (options) {
         columns: options.columns,
         dimensions: options.dimensions,
         filters: options.filters,
-        placement: options.placement || DEFAULT_PLACEMENT
+        placement: options.placement || DEFAULT_PLACEMENT,
+        isDefaultAggregation: options.isDefaultAggregation
     };
 }
 
@@ -298,20 +299,50 @@ const havingClause = ctx => {
 // (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.
+//
+// NOTE: We'd rather use !pixel_height!, but in Mapnik this value is extent / 256 for raster
+// and extent / tile_extent {4096 default} for MVT, so since aggregations are always based
+// on 256 we can't have the same query in both cases
+// As this scale change doesn't happen in !scale_denominator! we use that instead
 const gridResolution = ctx => {
     const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38);
-    return `${256/ctx.res} * GREATEST(!pixel_height!, ${minimumResolution})::numeric`;
+    return `${256/ctx.res} * GREATEST(!scale_denominator! * 0.00028, ${minimumResolution})::double precision`;
 };
 
-const aggregatedPoint = ctx => {
+// Function to generate the resulting point for a cell from the aggregated
+const aggregatedPoint = (ctx, aggregated) => {
     const placement = ctx.placement || DEFAULT_PLACEMENT;
     switch (placement) {
+
+        // For centroid, we return the average of the cell
         case `centroid`:
-            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+            return aggregated ?
+                `, ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857) AS the_geom_webmercator` :
+                ``;
+
+        // Middle point of the cell
         case `point-grid`:
-            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+            return aggregated ?
+                `, ST_SetSRID(ST_MakePoint(cdb_xmin + (cdb_pos_grid_x + 0.5) * res,
+                                           cdb_ymin + (cdb_pos_grid_y + 0.5) * res),
+                                           3857) AS the_geom_webmercator`:
+                ``;
+
+        // For point-sample we'll get a single point directly from the source
+        // If it's default aggregation we'll add the extra columns to keep backwards compatibility
         case `point-sample`:
-            return `ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857)`;
+            return aggregated ?
+                `` :
+`NATURAL JOIN
+(
+    SELECT ${ctx.isDefaultAggregation ? `*` : `cartodb_id, the_geom_webmercator`}
+    FROM
+    (
+        ${ctx.sourceQuery}
+    ) __cdb_src_query
+) __cdb_query_columns
+`;
+
         default:
             throw new Error(`Invalid aggregation placement "${placement}`);
     }
@@ -330,51 +361,54 @@ const aggregatedPoint = ctx => {
 
 
 const defaultAggregationQueryTemplate = ctx => `
-SELECT
-    min(cartodb_id) as cartodb_id,
-    ${aggregatedPoint(ctx)} AS the_geom_webmercator
-    ${dimensionDefs(ctx)}
-    ${aggregateColumnDefs(ctx)}
-FROM
+SELECT * FROM
 (
     SELECT
-        __cdb_src_query.*,
-        cdb_limit_x,
-        cdb_limit_y,
-        ((ST_X(the_geom_webmercator) - __cdb_src_params.cdb_xmin) / __cdb_src_params.res)::int as cdb_pos_grid_x,
-        ((ST_Y(the_geom_webmercator) - __cdb_src_params.cdb_ymin) / __cdb_src_params.res)::int as cdb_pos_grid_y
+        min(cartodb_id) as cartodb_id
+        ${aggregatedPoint(ctx, true)}
+        ${dimensionDefs(ctx)}
+        ${aggregateColumnDefs(ctx)}
     FROM
-    (
-        ${ctx.sourceQuery}
-    ) __cdb_src_query,
     (
         SELECT
-            _cdb_grid_bbox_margins.*,
-            ST_MakeEnvelope(cdb_xmin, cdb_ymin, cdb_xmax, cdb_ymax, 3857) AS cdb_point_bbox,
-            ROUND((cdb_xmax - cdb_xmin) /  res)::int as cdb_limit_x,
-            ROUND((cdb_ymax - cdb_ymin) /  res)::int as cdb_limit_y
+            __cdb_src_query.*,
+            __cdb_src_params.*,
+            FLOOR((ST_X(the_geom_webmercator) - __cdb_src_params.cdb_xmin) / __cdb_src_params.res) as cdb_pos_grid_x,
+            FLOOR((ST_Y(the_geom_webmercator) - __cdb_src_params.cdb_ymin) / __cdb_src_params.res) as cdb_pos_grid_y
         FROM
+        (
+            ${ctx.sourceQuery}
+        ) __cdb_src_query,
         (
             SELECT
-                res,
-                CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
-                FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
-                CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
-                FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
+                _cdb_grid_bbox_margins.*,
+                ST_MakeEnvelope(cdb_xmin, cdb_ymin, cdb_xmax, cdb_ymax, 3857) AS cdb_point_bbox,
+                ((cdb_xmax - cdb_xmin) /  res)::int as cdb_limit_x,
+                ((cdb_ymax - cdb_ymin) /  res)::int as cdb_limit_y
             FROM
             (
                 SELECT
-                    ${gridResolution(ctx)} AS res,
-                    !bbox! cdb_full_bbox
-                OFFSET 0
-            ) _cdb_input_resources
-        ) _cdb_grid_bbox_margins OFFSET 0
-    ) __cdb_src_params
-    WHERE the_geom_webmercator && cdb_point_bbox OFFSET 0
-) __cdb_srd_grid
-WHERE cdb_pos_grid_x < cdb_limit_x AND cdb_pos_grid_y < cdb_limit_y
-GROUP BY cdb_pos_grid_x, cdb_pos_grid_y ${dimensionNames(ctx)}
-${havingClause(ctx)}
+                    res,
+                    CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
+                    FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
+                    CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
+                    FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
+                FROM
+                (
+                    SELECT
+                        ${gridResolution(ctx)} AS res,
+                        !bbox! cdb_full_bbox
+                    OFFSET 0
+                ) _cdb_input_resources
+            ) _cdb_grid_bbox_margins OFFSET 0
+        ) __cdb_src_params
+        WHERE the_geom_webmercator && cdb_point_bbox OFFSET 0
+    ) __cdb_srd_grid
+    WHERE cdb_pos_grid_x < cdb_limit_x AND cdb_pos_grid_y < cdb_limit_y
+    GROUP BY cdb_pos_grid_x, cdb_pos_grid_y, __cdb_srd_grid.cdb_xmin, __cdb_srd_grid.cdb_ymin, __cdb_srd_grid.res ${dimensionNames(ctx)}
+    ${havingClause(ctx)}
+) __cdb_aggregation_src
+${aggregatedPoint(ctx, false)}
 `;
 
 const aggregationQueryTemplates = {
diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index 3dcac93c..904f747c 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -6,11 +6,14 @@ const assert = require('../support/assert');
 const TestClient = require('../support/test-client');
 const serverOptions = require('../../lib/cartodb/server_options');
 
+const windshaft = require('windshaft');
+const psql_utils = new windshaft.cartodb_utils();
+
 const suites = [
-    {
-        desc: 'mvt (mapnik)',
-        usePostGIS: false
-    },
+//    {
+//        desc: 'mvt (mapnik)',
+//        usePostGIS: false
+//    },
     {
         desc: 'mvt (postgis)',
         usePostGIS: true
@@ -109,15 +112,15 @@ describe('aggregation', function () {
     WITH hgrid AS (
         SELECT
             CDB_RectangleGrid (
-                ST_Expand(!bbox!, CDB_XYZ_Resolution(1) * 12),
-                CDB_XYZ_Resolution(1) * 12,
-                CDB_XYZ_Resolution(1) * 12
+                ST_Expand(!bbox!, ${psql_utils.cdbXYZResolution(1)} * 12),
+                ${psql_utils.cdbXYZResolution(1)} * 12,
+                ${psql_utils.cdbXYZResolution(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,
+        count(1) /power( 12 * ${psql_utils.cdbXYZResolution(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
@@ -202,9 +205,9 @@ describe('aggregation', function () {
     //       |   |   |   |   |   |   |
     //    Tile 0, 1 -+---+---+---+- Tile 1,1
     //
-    const POINTS_SQL_GRID = `
+    const POINTS_SQL_GRID = (z, resolution) => `
         WITH params AS (
-            SELECT CDB_XYZ_Resolution($Z)*$resolution AS l -- cell size for Z, resolution
+            SELECT ${psql_utils.cdbXYZResolution(z)}*${resolution} AS l -- cell size for Z, resolution
         )
         SELECT
             row_number() OVER () AS cartodb_id,
@@ -521,7 +524,7 @@ describe('aggregation', function () {
             });
 
             ['centroid', 'point-sample', 'point-grid'].forEach(placement => {
-                it('should provide all the requested columns in non-default aggregation ',
+                it('should provide all the requested columns in non-default aggregation: ' + placement,
                 function (done) {
                     const response = {
                         status: 200,
@@ -570,7 +573,7 @@ describe('aggregation', function () {
                     });
                 });
 
-                it('should provide only the requested columns in non-default aggregation ',
+                it('should provide only the requested columns in non-default aggregation: ' + placement,
                 function (done) {
                     this.mapConfig = createVectorMapConfig([
                         {
@@ -2970,7 +2973,7 @@ 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);
+                    const query = POINTS_SQL_GRID(z, resolution);
                     this.mapConfig = {
                         version: '1.6.0',
                         buffersize: { 'mvt': 0 },
@@ -3015,32 +3018,17 @@ describe('aggregation', function () {
                                     }
                                     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);
+                                    // There needs to be 13 points
+                                    const count_features = ((tile) =>
+                                        tile.features.map(f => f.properties)
+                                                     .map(f => f._cdb_feature_count)
+                                                     .reduce((a,b) => a + b, 0));
+
+                                    const tile00Count = count_features(tile00);
+                                    const tile10Count = count_features(tile10);
+                                    const tile01Count = count_features(tile01);
+                                    const tile11Count = count_features(tile11);
+                                    assert.equal(13, tile00Count + tile10Count + tile01Count + tile11Count);
 
                                     done();
                                 });
@@ -3056,7 +3044,7 @@ describe('aggregation', function () {
                     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);
+                    const query = POINTS_SQL_GRID(z, resolution / 2);
 
                     this.mapConfig = {
                         version: '1.6.0',
@@ -3133,20 +3121,11 @@ 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);
+                    const query = POINTS_SQL_GRID(z, resolution);
 
                     this.mapConfig = {
                         version: '1.6.0',
@@ -3192,6 +3171,18 @@ describe('aggregation', function () {
                                     }
                                     const tile11 = JSON.parse(mvt.toGeoJSONSync(0));
 
+                                    // There needs to be 41 points
+                                    const count_features = ((tile) =>
+                                        tile.features.map(f => f.properties)
+                                                     .map(f => f._cdb_feature_count)
+                                                     .reduce((a,b) => a + b, 0));
+
+                                    const tile00Count = count_features(tile00);
+                                    const tile10Count = count_features(tile10);
+                                    const tile01Count = count_features(tile01);
+                                    const tile11Count = count_features(tile11);
+                                    assert.equal(41, tile00Count + tile10Count + tile01Count + tile11Count);
+
                                     const tile00Expected = [
                                         { _cdb_feature_count: 2, cartodb_id: 1 },
                                         { _cdb_feature_count: 2, cartodb_id: 2 },

From 892479d9b9dedc4d8bce0ae8498980ed1abea4d5 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Thu, 4 Jul 2019 13:43:27 +0200
Subject: [PATCH 06/19] API changes

---
 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 904f747c..48232614 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -6,8 +6,8 @@ const assert = require('../support/assert');
 const TestClient = require('../support/test-client');
 const serverOptions = require('../../lib/cartodb/server_options');
 
-const windshaft = require('windshaft');
-const psql_utils = new windshaft.cartodb_utils();
+const windshaftUtils = require('windshaft').utils;
+const wmh = new windshaftUtils.WebMercatorHelper();
 
 const suites = [
 //    {
@@ -112,15 +112,15 @@ describe('aggregation', function () {
     WITH hgrid AS (
         SELECT
             CDB_RectangleGrid (
-                ST_Expand(!bbox!, ${psql_utils.cdbXYZResolution(1)} * 12),
-                ${psql_utils.cdbXYZResolution(1)} * 12,
-                ${psql_utils.cdbXYZResolution(1)} * 12
+                ST_Expand(!bbox!, ${wmh.getResolution({ z : 1 })} * 12),
+                ${wmh.getResolution({ z : 1 })} * 12,
+                ${wmh.getResolution({ z : 1 })} * 12
             ) as cell
     )
     SELECT
         hgrid.cell as the_geom_webmercator,
         count(1) as agg_value,
-        count(1) /power( 12 * ${psql_utils.cdbXYZResolution(1)}, 2 ) as agg_value_density,
+        count(1) /power( 12 * ${wmh.getResolution({ z : 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
@@ -207,7 +207,7 @@ describe('aggregation', function () {
     //
     const POINTS_SQL_GRID = (z, resolution) => `
         WITH params AS (
-            SELECT ${psql_utils.cdbXYZResolution(z)}*${resolution} AS l -- cell size for Z, resolution
+            SELECT ${wmh.getResolution({ z : z })}*${resolution} AS l -- cell size for Z, resolution
         )
         SELECT
             row_number() OVER () AS cartodb_id,

From 8454eef6e9aeddfc22cb716b391baaf148bcf525 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Thu, 4 Jul 2019 17:15:53 +0200
Subject: [PATCH 07/19] Aggregation: Extract the query to get the grid data

---
 .../models/aggregation/aggregation-query.js   | 36 +++++++++++--------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 9e9ff805..2c3026a3 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -300,7 +300,7 @@ const havingClause = ctx => {
 // 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.
 //
-// NOTE: We'd rather use !pixel_height!, but in Mapnik this value is extent / 256 for raster
+// NOTE: We'd rather use !scale_denominator!, but in Mapnik this value is extent / 256 for raster
 // and extent / tile_extent {4096 default} for MVT, so since aggregations are always based
 // on 256 we can't have the same query in both cases
 // As this scale change doesn't happen in !scale_denominator! we use that instead
@@ -309,6 +309,26 @@ const gridResolution = ctx => {
     return `${256/ctx.res} * GREATEST(!scale_denominator! * 0.00028, ${minimumResolution})::double precision`;
 };
 
+// SQL query to extra the boundaries of the area to be aggregated and the grid resolution
+const gridInfoQuery = ctx => {
+    return `
+    SELECT
+        res,
+        CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
+        FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
+        CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
+        FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
+    FROM
+    (
+        SELECT
+            ${gridResolution(ctx)} AS res,
+            !bbox! cdb_full_bbox
+        OFFSET 0
+    ) _cdb_input_resources
+`;
+};
+
+
 // Function to generate the resulting point for a cell from the aggregated
 const aggregatedPoint = (ctx, aggregated) => {
     const placement = ctx.placement || DEFAULT_PLACEMENT;
@@ -387,19 +407,7 @@ SELECT * FROM
                 ((cdb_ymax - cdb_ymin) /  res)::int as cdb_limit_y
             FROM
             (
-                SELECT
-                    res,
-                    CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
-                    FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
-                    CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
-                    FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
-                FROM
-                (
-                    SELECT
-                        ${gridResolution(ctx)} AS res,
-                        !bbox! cdb_full_bbox
-                    OFFSET 0
-                ) _cdb_input_resources
+                ${gridInfoQuery(ctx)}
             ) _cdb_grid_bbox_margins OFFSET 0
         ) __cdb_src_params
         WHERE the_geom_webmercator && cdb_point_bbox OFFSET 0

From 262f957218fdc83f5e748b40d6645b318391ea7c Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 8 Jul 2019 15:51:55 +0200
Subject: [PATCH 08/19] Aggregation: Improve speeeeeeed

---
 .../models/aggregation/aggregation-query.js   | 101 ++++++++++--------
 1 file changed, 59 insertions(+), 42 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 2c3026a3..f55c9e75 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -310,26 +310,39 @@ const gridResolution = ctx => {
 };
 
 // SQL query to extra the boundaries of the area to be aggregated and the grid resolution
+// cdb_{x-y}{min_max} return the limits of the tile. Aggregations do [min, max) in both axis
+// cdb_res: Aggregation resolution (as specified by gridResolution)
+// cdb_point_bbox: Tile bounding box [min, max]
 const gridInfoQuery = ctx => {
     return `
     SELECT
-        res,
-        CEIL (ST_XMIN(cdb_full_bbox) / res) * res AS cdb_xmin,
-        FLOOR(ST_XMAX(cdb_full_bbox) / res) * res AS cdb_xmax,
-        CEIL (ST_YMIN(cdb_full_bbox) / res) * res AS cdb_ymin,
-        FLOOR(ST_YMAX(cdb_full_bbox) / res) * res AS cdb_ymax
+        cdb_xmin,
+        cdb_ymin,
+        cdb_xmax,
+        cdb_ymax,
+        cdb_res,
+        ST_MakeEnvelope(cdb_xmin, cdb_ymin, cdb_xmax, cdb_ymax, 3857) AS cdb_point_bbox
     FROM
     (
         SELECT
-            ${gridResolution(ctx)} AS res,
-            !bbox! cdb_full_bbox
-        OFFSET 0
-    ) _cdb_input_resources
+            cdb_res,
+            CEIL (ST_XMIN(cdb_full_bbox) / cdb_res) * cdb_res AS cdb_xmin,
+            FLOOR(ST_XMAX(cdb_full_bbox) / cdb_res) * cdb_res AS cdb_xmax,
+            CEIL (ST_YMIN(cdb_full_bbox) / cdb_res) * cdb_res AS cdb_ymin,
+            FLOOR(ST_YMAX(cdb_full_bbox) / cdb_res) * cdb_res AS cdb_ymax
+        FROM
+        (
+            SELECT
+                ${gridResolution(ctx)} AS cdb_res,
+                !bbox! cdb_full_bbox
+        ) _cdb_input_resources
+    ) _cdb_grid_bbox_margins
 `;
 };
 
 
-// Function to generate the resulting point for a cell from the aggregated
+// Function to generate the resulting point for a cell from the aggregated data
+// Point sample joins the query with itself to get the data from the lowest id
 const aggregatedPoint = (ctx, aggregated) => {
     const placement = ctx.placement || DEFAULT_PLACEMENT;
     switch (placement) {
@@ -337,15 +350,13 @@ const aggregatedPoint = (ctx, aggregated) => {
         // For centroid, we return the average of the cell
         case `centroid`:
             return aggregated ?
-                `, ST_SetSRID(ST_MakePoint(AVG(ST_X(the_geom_webmercator)), AVG(ST_Y(the_geom_webmercator))), 3857) AS the_geom_webmercator` :
+                `, ST_SetSRID(ST_MakePoint(AVG(cdb_x), AVG(cdb_y)), 3857) AS the_geom_webmercator` :
                 ``;
 
         // Middle point of the cell
         case `point-grid`:
             return aggregated ?
-                `, ST_SetSRID(ST_MakePoint(cdb_xmin + (cdb_pos_grid_x + 0.5) * res,
-                                           cdb_ymin + (cdb_pos_grid_y + 0.5) * res),
-                                           3857) AS the_geom_webmercator`:
+                `, ST_SetSRID(ST_MakePoint(cdb_pos_grid_x, cdb_pos_grid_y), 3857) AS the_geom_webmercator`:
                 ``;
 
         // For point-sample we'll get a single point directly from the source
@@ -368,19 +379,29 @@ const aggregatedPoint = (ctx, aggregated) => {
     }
 };
 
-// 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.
-// * 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.
+// Function to generate the values common to all points in a cell
+// By default we use the cell number (which is fast), but for point-grid we
+// get the coordinates of the mid point so we don't need to calculate them later
+// which requires extra data in the group by clause
+const aggregatedPosCoordinate = (ctx, coordinate) => {
+    const placement = ctx.placement || DEFAULT_PLACEMENT;
+    switch (placement) {
+        // For point-grid we return the coordinate of the middle point of the grid
+        case `point-grid`:
+            return `(FLOOR(cdb_${coordinate} / __cdb_grid_params.cdb_res) + 0.5) * __cdb_grid_params.cdb_res`;
+
+        // For other, we return the cell position (relative to the world)
+        default:
+            return `FLOOR(cdb_${coordinate} / __cdb_grid_params.cdb_res)`;
+    }
+};
 
 
 const defaultAggregationQueryTemplate = ctx => `
+WITH __cdb_grid_params AS
+(
+    ${gridInfoQuery(ctx)}
+)
 SELECT * FROM
 (
     SELECT
@@ -391,29 +412,25 @@ SELECT * FROM
     FROM
     (
         SELECT
-            __cdb_src_query.*,
-            __cdb_src_params.*,
-            FLOOR((ST_X(the_geom_webmercator) - __cdb_src_params.cdb_xmin) / __cdb_src_params.res) as cdb_pos_grid_x,
-            FLOOR((ST_Y(the_geom_webmercator) - __cdb_src_params.cdb_ymin) / __cdb_src_params.res) as cdb_pos_grid_y
+            *,
+            ${aggregatedPosCoordinate(ctx, 'x')} as cdb_pos_grid_x,
+            ${aggregatedPosCoordinate(ctx, 'y')} as cdb_pos_grid_y
         FROM
-        (
-            ${ctx.sourceQuery}
-        ) __cdb_src_query,
         (
             SELECT
-                _cdb_grid_bbox_margins.*,
-                ST_MakeEnvelope(cdb_xmin, cdb_ymin, cdb_xmax, cdb_ymax, 3857) AS cdb_point_bbox,
-                ((cdb_xmax - cdb_xmin) /  res)::int as cdb_limit_x,
-                ((cdb_ymax - cdb_ymin) /  res)::int as cdb_limit_y
+                __cdb_src_query.*,
+                ST_X(the_geom_webmercator) cdb_x,
+                ST_Y(the_geom_webmercator) cdb_y
             FROM
             (
-                ${gridInfoQuery(ctx)}
-            ) _cdb_grid_bbox_margins OFFSET 0
-        ) __cdb_src_params
-        WHERE the_geom_webmercator && cdb_point_bbox OFFSET 0
-    ) __cdb_srd_grid
-    WHERE cdb_pos_grid_x < cdb_limit_x AND cdb_pos_grid_y < cdb_limit_y
-    GROUP BY cdb_pos_grid_x, cdb_pos_grid_y, __cdb_srd_grid.cdb_xmin, __cdb_srd_grid.cdb_ymin, __cdb_srd_grid.res ${dimensionNames(ctx)}
+                ${ctx.sourceQuery}
+            ) __cdb_src_query, __cdb_grid_params
+            WHERE the_geom_webmercator && cdb_point_bbox
+            OFFSET 0
+        ) __cdb_src_get_x_y, __cdb_grid_params
+        WHERE cdb_x < __cdb_grid_params.cdb_xmax AND cdb_y < __cdb_grid_params.cdb_ymax
+    ) __cdb_src_gridded, __cdb_grid_params
+    GROUP BY cdb_pos_grid_x, cdb_pos_grid_y ${dimensionNames(ctx)}
     ${havingClause(ctx)}
 ) __cdb_aggregation_src
 ${aggregatedPoint(ctx, false)}

From d3e807583ad47c7117406791134edc96a73dd201 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 8 Jul 2019 16:27:31 +0200
Subject: [PATCH 09/19] Use proper mode

---
 lib/cartodb/models/aggregation/aggregation-query.js | 2 +-
 test/acceptance/aggregation.js                      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index f55c9e75..55bdf7f0 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -89,7 +89,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) => `mode() WITHIN GROUP (ORDER BY ${params.aggregated_column || column_name})`
     }
 };
 
diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index 48232614..82a3da5a 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -2354,7 +2354,7 @@ describe('aggregation', function () {
                                     threshold: 1,
                                     columns: {
                                         value: {
-                                            aggregate_function: 'sum',
+                                            aggregate_function: 'mode',
                                             aggregated_column: 'value'
                                         }
                                     },
@@ -2400,7 +2400,7 @@ describe('aggregation', function () {
                                     threshold: 1,
                                     columns: {
                                         value: {
-                                            aggregate_function: 'sum',
+                                            aggregate_function: 'mode',
                                             aggregated_column: 'value'
                                         }
                                     },

From aed456bf3243b4d60a1c60f4d01f95eaf73ccfb5 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 8 Jul 2019 18:32:04 +0200
Subject: [PATCH 10/19] Cluster: Use new webmercator utilities

---
 lib/cartodb/backends/cluster.js                     | 7 +++++--
 lib/cartodb/models/aggregation/aggregation-query.js | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js
index da468177..62ef86a9 100644
--- a/lib/cartodb/backends/cluster.js
+++ b/lib/cartodb/backends/cluster.js
@@ -5,6 +5,9 @@ const dbParamsFromReqParams = require('../utils/database-params');
 const debug = require('debug')('backend:cluster');
 const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig');
 
+const windshaftUtils = require('windshaft').utils;
+const wmh = new windshaftUtils.WebMercatorHelper();
+
 module.exports = class ClusterBackend {
     getClusterFeatures (mapConfigProvider, params, callback) {
         mapConfigProvider.getMapConfig((err, _mapConfig) => {
@@ -156,8 +159,8 @@ const clusterFeaturesQuery = ctx => `
 `;
 
 const gridResolution = ctx => {
-    const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38);
-    return `${256/ctx.res} * GREATEST(!pixel_height!, ${minimumResolution})::numeric`;
+    const zoom_resolution = wmh.getResolution({ z : Math.min(38, ctx.zoom) });
+    return `${256/ctx.res} * (${zoom_resolution})::double precision`;
 };
 
 const aggregationQuery = ctx => `
diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 55bdf7f0..52778443 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -429,7 +429,7 @@ SELECT * FROM
             OFFSET 0
         ) __cdb_src_get_x_y, __cdb_grid_params
         WHERE cdb_x < __cdb_grid_params.cdb_xmax AND cdb_y < __cdb_grid_params.cdb_ymax
-    ) __cdb_src_gridded, __cdb_grid_params
+    ) __cdb_src_gridded
     GROUP BY cdb_pos_grid_x, cdb_pos_grid_y ${dimensionNames(ctx)}
     ${havingClause(ctx)}
 ) __cdb_aggregation_src

From 46600bf4fccab036318eb2c3875950f83a081877 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 8 Jul 2019 18:52:35 +0200
Subject: [PATCH 11/19] Please jshint

---
 .../models/aggregation/aggregation-query.js   | 57 ++++++++++---------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 52778443..866f1eb0 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -108,16 +108,6 @@ const aggregateColumns = ctx => {
     }, ctx.columns || {});
 };
 
-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));
-};
-
 const aggregateExpression = (column_name, column_parameters) => {
     const aggregate_function = column_parameters.aggregate_function || 'count';
     const aggregate_definition = SUPPORTED_AGGREGATE_FUNCTIONS[aggregate_function];
@@ -342,29 +332,41 @@ const gridInfoQuery = ctx => {
 
 
 // Function to generate the resulting point for a cell from the aggregated data
-// Point sample joins the query with itself to get the data from the lowest id
-const aggregatedPoint = (ctx, aggregated) => {
-    const placement = ctx.placement || DEFAULT_PLACEMENT;
-    switch (placement) {
+const aggregatedPointWebMercator = (ctx) => {
+    switch (ctx.placement) {
 
         // For centroid, we return the average of the cell
         case `centroid`:
-            return aggregated ?
-                `, ST_SetSRID(ST_MakePoint(AVG(cdb_x), AVG(cdb_y)), 3857) AS the_geom_webmercator` :
-                ``;
+            return `, ST_SetSRID(ST_MakePoint(AVG(cdb_x), AVG(cdb_y)), 3857) AS the_geom_webmercator`;
 
         // Middle point of the cell
         case `point-grid`:
-            return aggregated ?
-                `, ST_SetSRID(ST_MakePoint(cdb_pos_grid_x, cdb_pos_grid_y), 3857) AS the_geom_webmercator`:
-                ``;
+            return `, ST_SetSRID(ST_MakePoint(cdb_pos_grid_x, cdb_pos_grid_y), 3857) AS the_geom_webmercator`;
 
         // For point-sample we'll get a single point directly from the source
         // If it's default aggregation we'll add the extra columns to keep backwards compatibility
         case `point-sample`:
-            return aggregated ?
-                `` :
-`NATURAL JOIN
+            return ``;
+
+        default:
+            throw new Error(`Invalid aggregation placement "${ctx.placement}`);
+    }
+};
+
+// Function to generate the resulting point for a cell from the a join with the source
+const aggregatedPointJoin = (ctx) => {
+    switch (ctx.placement) {
+
+        case `centroid`:
+            return ``;
+
+        case `point-grid`:
+            return ``;
+
+        // For point-sample we'll get a single point directly from the source
+        // If it's default aggregation we'll add the extra columns to keep backwards compatibility
+        case `point-sample`:
+            return `NATURAL JOIN
 (
     SELECT ${ctx.isDefaultAggregation ? `*` : `cartodb_id, the_geom_webmercator`}
     FROM
@@ -375,7 +377,7 @@ const aggregatedPoint = (ctx, aggregated) => {
 `;
 
         default:
-            throw new Error(`Invalid aggregation placement "${placement}`);
+            throw new Error(`Invalid aggregation placement "${ctx.placement}`);
     }
 };
 
@@ -384,8 +386,7 @@ const aggregatedPoint = (ctx, aggregated) => {
 // get the coordinates of the mid point so we don't need to calculate them later
 // which requires extra data in the group by clause
 const aggregatedPosCoordinate = (ctx, coordinate) => {
-    const placement = ctx.placement || DEFAULT_PLACEMENT;
-    switch (placement) {
+    switch (ctx.placement) {
         // For point-grid we return the coordinate of the middle point of the grid
         case `point-grid`:
             return `(FLOOR(cdb_${coordinate} / __cdb_grid_params.cdb_res) + 0.5) * __cdb_grid_params.cdb_res`;
@@ -406,7 +407,7 @@ SELECT * FROM
 (
     SELECT
         min(cartodb_id) as cartodb_id
-        ${aggregatedPoint(ctx, true)}
+        ${aggregatedPointWebMercator(ctx)}
         ${dimensionDefs(ctx)}
         ${aggregateColumnDefs(ctx)}
     FROM
@@ -433,7 +434,7 @@ SELECT * FROM
     GROUP BY cdb_pos_grid_x, cdb_pos_grid_y ${dimensionNames(ctx)}
     ${havingClause(ctx)}
 ) __cdb_aggregation_src
-${aggregatedPoint(ctx, false)}
+${aggregatedPointJoin(ctx)}
 `;
 
 const aggregationQueryTemplates = {

From 492bcbfdaa3d801a67bd5dbe149f7c0072a1a640 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Tue, 9 Jul 2019 14:02:34 +0200
Subject: [PATCH 12/19] Aggregation tests: Enable mapnik and rework the
 complete cells test

Before it used to test exact tiles, now it test that if a cell/value is included
it will contain exactly the features that we expect
---
 test/acceptance/aggregation.js | 87 +++++++++++++---------------------
 1 file changed, 34 insertions(+), 53 deletions(-)

diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index 82a3da5a..22ec6a4c 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -10,10 +10,10 @@ const windshaftUtils = require('windshaft').utils;
 const wmh = new windshaftUtils.WebMercatorHelper();
 
 const suites = [
-//    {
-//        desc: 'mvt (mapnik)',
-//        usePostGIS: false
-//    },
+    {
+        desc: 'mvt (mapnik)',
+        usePostGIS: false
+    },
     {
         desc: 'mvt (postgis)',
         usePostGIS: true
@@ -3171,60 +3171,41 @@ describe('aggregation', function () {
                                     }
                                     const tile11 = JSON.parse(mvt.toGeoJSONSync(0));
 
-                                    // There needs to be 41 points
-                                    const count_features = ((tile) =>
-                                        tile.features.map(f => f.properties)
-                                                     .map(f => f._cdb_feature_count)
-                                                     .reduce((a,b) => a + b, 0));
+                                    const allValues = [
+                                        { cartodb_id: 1, _cdb_feature_count: 2 },
+                                        { cartodb_id: 2, _cdb_feature_count: 2 },
+                                        { cartodb_id: 3, _cdb_feature_count: 1 },
+                                        { cartodb_id: 4, _cdb_feature_count: 2 },
+                                        { cartodb_id: 5, _cdb_feature_count: 2 },
+                                        { cartodb_id: 6, _cdb_feature_count: 1 },
+                                        { cartodb_id: 7, _cdb_feature_count: 1 },
+                                        { cartodb_id: 8, _cdb_feature_count: 1 },
+                                        { cartodb_id: 9, _cdb_feature_count: 1 }
+                                    ];
 
-                                    const tile00Count = count_features(tile00);
-                                    const tile10Count = count_features(tile10);
-                                    const tile01Count = count_features(tile01);
-                                    const tile11Count = count_features(tile11);
-                                    assert.equal(41, tile00Count + tile10Count + tile01Count + tile11Count);
-
-                                    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 }
-                                    ];
+                                    // We check that the cells either added completely in the tile
+                                    // or not at all
+                                    const f_check_properties = (tile => {
+                                        tile.forEach(property => {
+                                            allValues.forEach(v => {
+                                                if (v.cartodb_id === property.cartodb_id) {
+                                                    assert.equal(v._cdb_feature_count,
+                                                                 property._cdb_feature_count,
+                                                                 "Tile: " + JSON.stringify(tile) +
+                                                                 ". ID: " + property.cartodb_id);
+                                                }
+                                            });
+                                        });
+                                    });
                                     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);
+
+                                    f_check_properties(tile00Actual);
+                                    f_check_properties(tile10Actual);
+                                    f_check_properties(tile01Actual);
+                                    f_check_properties(tile11Actual);
 
                                     done();
                                 });

From bdbe132311a606fccd743ad66179913c5ba964af Mon Sep 17 00:00:00 2001
From: Raul Marin <rmrodriguez@cartodb.com>
Date: Thu, 11 Jul 2019 10:03:41 +0200
Subject: [PATCH 13/19] Update lodash

---
 package-lock.json | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 0bc4990b..13ff3a03 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -2566,9 +2566,9 @@
       }
     },
     "lodash": {
-      "version": "4.17.11",
-      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.11.tgz",
-      "integrity": "sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg=="
+      "version": "4.17.14",
+      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.14.tgz",
+      "integrity": "sha512-mmKYbW3GLuJeX+iGP+Y7Gp1AiGHGbXHCOh/jZmrawMmsE7MS4znI3RL2FsjbqOyMayHInjOeykW7PEajUk1/xw=="
     },
     "lodash.assign": {
       "version": "4.2.0",
@@ -4098,9 +4098,9 @@
       "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8="
     },
     "xtend": {
-      "version": "4.0.1",
-      "resolved": "https://registry.npmjs.org/xtend/-/xtend-4.0.1.tgz",
-      "integrity": "sha1-pcbVMr5lbiPbgg77lDofBJmNY68="
+      "version": "4.0.2",
+      "resolved": "https://registry.npmjs.org/xtend/-/xtend-4.0.2.tgz",
+      "integrity": "sha512-LKYU1iAXJXUgAXn9URjiu+MWhyUXHsvfp7mcuYm9dSUKK0/CjtrUwFAxD82/mCWbtLsGjFIad0wIsod4zrTAEQ=="
     },
     "y18n": {
       "version": "3.2.1",

From 63b6af2ac73e0862a0ae706f4dcf4dfa8afb3c55 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 15 Jul 2019 14:16:21 +0200
Subject: [PATCH 14/19] Query utils: Use webmercator utils, reuse code and
 always substitute tokens

---
 lib/cartodb/utils/query-utils.js | 99 ++++++++++++++------------------
 1 file changed, 42 insertions(+), 57 deletions(-)

diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js
index 2fc00c5a..f5daf0e1 100644
--- a/lib/cartodb/utils/query-utils.js
+++ b/lib/cartodb/utils/query-utils.js
@@ -1,48 +1,41 @@
 'use strict';
 
-const SubstitutionTokens = require('./substitution-tokens');
+const windshaftUtils = require('windshaft').utils;
 
-function prepareQuery(sql) {
-    var affectedTableRegexCache = {
-        bbox: /!bbox!/g,
-        scale_denominator: /!scale_denominator!/g,
-        pixel_width: /!pixel_width!/g,
-        pixel_height: /!pixel_height!/g
-    };
-
-    return sql
-        .replace(affectedTableRegexCache.bbox, 'ST_MakeEnvelope(0,0,0,0)')
-        .replace(affectedTableRegexCache.scale_denominator, '0')
-        .replace(affectedTableRegexCache.pixel_width, '1')
-        .replace(affectedTableRegexCache.pixel_height, '1');
-}
-
-module.exports.extractTableNames = function extractTableNames(query) {
+module.exports.extractTableNames = function (query) {
     return [
         'SELECT * FROM CDB_QueryTablesText($windshaft$',
-        prepareQuery(query),
+        substituteDummyTokens(query),
         '$windshaft$) as tablenames'
     ].join('');
 };
 
 module.exports.getQueryActualRowCount = function (query) {
-    return `select COUNT(*) AS rows FROM (${query}) AS __cdb_query`;
+    return `select COUNT(*) AS rows FROM (${substituteDummyTokens(query)}) AS __cdb_query`;
 };
 
 function getQueryRowEstimation(query) {
-    return 'select CDB_EstimateRowCount($windshaft$' + query + '$windshaft$) as rows';
+    return 'select CDB_EstimateRowCount($windshaft$' + substituteDummyTokens(query) + '$windshaft$) as rows';
 }
-
 module.exports.getQueryRowEstimation = getQueryRowEstimation;
 
+function getQueryGeometryType(query, geometryColumn) {
+    return `
+        SELECT ST_GeometryType(${geometryColumn}) AS geom_type
+            FROM (${substituteDummyTokens(query)}) AS __cdb_query
+            WHERE ${geometryColumn} IS NOT NULL
+            LIMIT 1
+    `;
+}
+module.exports.getQueryGeometryType = getQueryGeometryType;
+
 module.exports.getAggregationMetadata = ctx => `
     WITH
     rowEstimation AS (
         ${getQueryRowEstimation(ctx.query)}
     ),
     geometryType AS (
-        SELECT ST_GeometryType(${ctx.geometryColumn}) as geom_type
-        FROM (${ctx.query}) AS __cdb_query WHERE ${ctx.geometryColumn} IS NOT NULL LIMIT 1
+        ${getQueryGeometryType(ctx.query, ctx.geometryColumn)}
     )
     SELECT
         rows AS count,
@@ -85,7 +78,7 @@ module.exports.getQueryTopCategories = function (query, column, topN, includeNul
     const where = includeNulls ? '' : `WHERE ${column} IS NOT NULL`;
     return `
         SELECT ${column} AS category, COUNT(*) AS frequency
-        FROM (${query}) AS __cdb_query
+        FROM (${substituteDummyTokens(query)}) AS __cdb_query
         ${where}
         GROUP BY ${column} ORDER BY 2 DESC
         LIMIT ${topN}
@@ -116,7 +109,7 @@ module.exports.getQuerySample = function (query, sampleProb, limit = null, rando
             SELECT setseed(${randomSeed})
         )
         SELECT ${columnSelector(columns)}
-            FROM (${query}) AS __cdb_query
+            FROM (${substituteDummyTokens(query)}) AS __cdb_query
             WHERE random() < ${sampleProb}
             ${limitClause}
     `;
@@ -157,19 +150,11 @@ function simpleQueryTable(sql) {
     return false;
 }
 
-module.exports.getQueryGeometryType = function (query, geometryColumn) {
-    return `
-        SELECT ST_GeometryType(${geometryColumn}) AS geom_type
-            FROM (${query}) AS __cdb_query
-            WHERE ${geometryColumn} IS NOT NULL
-            LIMIT 1
-    `;
-};
 
 function getQueryLimited(query, limit = 0) {
     return `
         SELECT *
-            FROM (${query}) AS __cdb_query
+            FROM (${substituteDummyTokens(query)}) AS __cdb_query
             LIMIT ${limit}
     `;
 }
@@ -181,32 +166,32 @@ function queryPromise(dbConnection, query) {
 }
 
 function substituteDummyTokens(sql) {
-    return sql && SubstitutionTokens.replace(sql, {
-        bbox: 'ST_MakeEnvelope(0,0,0,0)',
-        scale_denominator: '0',
-        pixel_width: '1',
-        pixel_height: '1'
-    });
+    return subsituteTokensForZoom(sql, 0);
 }
 
-function subsituteTokensForZoom(sql, zoom, singleTile=false) {
-    const tileRes = 256;
-    const wmSize = 6378137.0*2*Math.PI;
-    const nTiles = Math.pow(2, zoom);
-    const tileSize = wmSize / nTiles;
-    const resolution = tileSize / tileRes;
-    const scaleDenominator = resolution / 0.00028;
-    const x0 = -wmSize/2, y0 = -wmSize/2;
-    let bbox = `ST_MakeEnvelope(${x0}, ${y0}, ${x0+wmSize}, ${y0+wmSize})`;
-    if (singleTile) {
-        bbox = `ST_MakeEnvelope(${x0}, ${y0}, ${x0 + tileSize}, ${y0 + tileSize})`;
+function subsituteTokensForZoom(sql, zoom) {
+    if (!sql) {
+        return undefined;
     }
-    return SubstitutionTokens.replace(sql, {
-        bbox: bbox,
-        scale_denominator: scaleDenominator,
-        pixel_width: resolution,
-        pixel_height: resolution
-    });
+    const affectedTableRegexCache = {
+        bbox: /!bbox!/g,
+        scale_denominator: /!scale_denominator!/g,
+        pixel_width: /!pixel_width!/g,
+        pixel_height: /!pixel_height!/g
+    };
+
+    const webmercator = new windshaftUtils.WebMercatorHelper();
+    const resolution = webmercator.getResolution({ z : zoom });
+    const scaleDenominator = resolution.dividedBy(0.00028);
+    // We always use the whole world as the bbox
+    const extent = webmercator.getExtent({ x : 0, y : 0, z : 0 });
+
+    return sql
+        .replace(affectedTableRegexCache.bbox,
+                 `ST_MakeEnvelope(${extent.xmin}, ${extent.ymin}, ${extent.xmax}, ${extent.ymax}, 3857)`)
+        .replace(affectedTableRegexCache.scale_denominator, scaleDenominator)
+        .replace(affectedTableRegexCache.pixel_width, resolution)
+        .replace(affectedTableRegexCache.pixel_height, resolution);
 }
 
 module.exports.queryPromise = queryPromise;

From 65beb6e4602f2577d07065d5f5a67e40544d2d5d Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 15 Jul 2019 15:30:56 +0200
Subject: [PATCH 15/19] Tweak the tests to accept Mapnik precision as valid

---
 test/acceptance/aggregation.js | 56 ++++++++++++----------------------
 1 file changed, 19 insertions(+), 37 deletions(-)

diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index 22ec6a4c..ea3f0261 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -227,8 +227,8 @@ describe('aggregation', function () {
     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
+        ST_SetSRID(ST_MakePoint(18181005.82, -18181043.9), 3857) AS the_geom_webmercator,
+        ST_Transform(ST_SetSRID(ST_MakePoint(18181005.82, -18181043.9), 3857), 4326) AS the_geom
       UNION ALL SELECT
         2 AS cartodb_id,
         ST_SetSRID(ST_MakePoint(18181005.9, -18181044.0), 3857) AS the_geom_webmercator,
@@ -239,8 +239,8 @@ describe('aggregation', function () {
         ST_Transform(ST_SetSRID(ST_MakePoint(18181005.87, -18181043.94), 3857), 4326) AS the_geom
       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
+        ST_SetSRID(ST_MakePoint(18181005.82, -18181043.9), 3857) AS the_geom_webmercator,
+        ST_Transform(ST_SetSRID(ST_MakePoint(18181005.82, -18181043.9), 3857), 4326) AS the_geom
     `;
 
     // Points positioned inside one cell of Z=20, X=1000000, X=1000000 (inner cell not on border)
@@ -252,8 +252,8 @@ describe('aggregation', function () {
         ST_Transform(ST_SetSRID(ST_MakePoint(18181005.95, -18181043.8), 3857), 4326) AS the_geom
       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
+        ST_SetSRID(ST_MakePoint(18181006.09, -18181043.74), 3857) AS the_geom_webmercator,
+        ST_Transform(ST_SetSRID(ST_MakePoint(18181006.09, -18181043.74), 3857), 4326) AS the_geom
       UNION ALL SELECT
         3 AS cartodb_id,
         ST_SetSRID(ST_MakePoint(18181006.02, -18181043.79), 3857) AS the_geom_webmercator,
@@ -3171,42 +3171,24 @@ describe('aggregation', function () {
                                     }
                                     const tile11 = JSON.parse(mvt.toGeoJSONSync(0));
 
-                                    const allValues = [
-                                        { cartodb_id: 1, _cdb_feature_count: 2 },
-                                        { cartodb_id: 2, _cdb_feature_count: 2 },
-                                        { cartodb_id: 3, _cdb_feature_count: 1 },
-                                        { cartodb_id: 4, _cdb_feature_count: 2 },
-                                        { cartodb_id: 5, _cdb_feature_count: 2 },
-                                        { cartodb_id: 6, _cdb_feature_count: 1 },
-                                        { cartodb_id: 7, _cdb_feature_count: 1 },
-                                        { cartodb_id: 8, _cdb_feature_count: 1 },
-                                        { cartodb_id: 9, _cdb_feature_count: 1 }
-                                    ];
-
-                                    // We check that the cells either added completely in the tile
-                                    // or not at all
-                                    const f_check_properties = (tile => {
-                                        tile.forEach(property => {
-                                            allValues.forEach(v => {
-                                                if (v.cartodb_id === property.cartodb_id) {
-                                                    assert.equal(v._cdb_feature_count,
-                                                                 property._cdb_feature_count,
-                                                                 "Tile: " + JSON.stringify(tile) +
-                                                                 ". ID: " + property.cartodb_id);
-                                                }
-                                            });
-                                        });
-                                    });
+                                    // We check that if an id/cell is present in multiple tiles,
+                                    // it always contains the same amount of features
                                     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);
 
-                                    f_check_properties(tile00Actual);
-                                    f_check_properties(tile10Actual);
-                                    f_check_properties(tile01Actual);
-                                    f_check_properties(tile11Actual);
-
+                                    const allFeatures = [... tile00Actual, ...tile10Actual,
+                                                         ...tile01Actual, ...tile11Actual];
+                                    for (let i = 0; i < allFeatures.length; i++) {
+                                        for (let j = i + 1; j < allFeatures.length; j++) {
+                                            const c1 = allFeatures[i];
+                                            const c2 = allFeatures[j];
+                                            if (c1.cartodb_id === c2.cartodb_id) {
+                                                assert.equal(c1._cdb_feature_count, c2._cdb_feature_count);
+                                            }
+                                        }
+                                    }
                                     done();
                                 });
                             });

From 286daa9bec9b45e5edd69ad5235e00b3fef79ceb Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 15 Jul 2019 15:35:50 +0200
Subject: [PATCH 16/19] Simplify aggregation templates

---
 .../models/aggregation/aggregation-query.js   | 38 ++-----------------
 1 file changed, 3 insertions(+), 35 deletions(-)

diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 866f1eb0..6fd416c5 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -4,31 +4,6 @@ const timeDimension = require('./time-dimension');
 
 const DEFAULT_PLACEMENT = 'point-sample';
 
-
-/**
- * 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
- * - columns
- * - dimensions
- */
-const templateForOptions = (options) => {
-    let templateFn = defaultAggregationQueryTemplate;
-    if (!options.isDefaultAggregation) {
-        templateFn = aggregationQueryTemplates[options.placement || DEFAULT_PLACEMENT];
-        if (!templateFn) {
-            throw new Error("Invalid Aggregation placement: '" + options.placement + "'");
-        }
-    }
-    return templateFn;
-};
-
 function optionsToParams (options) {
     return {
         sourceQuery: options.query,
@@ -55,7 +30,7 @@ function optionsToParams (options) {
  * 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)(optionsToParams(options));
+const queryForOptions = (options) => aggregationQueryTemplate(optionsToParams(options));
 
 module.exports = queryForOptions;
 
@@ -398,7 +373,7 @@ const aggregatedPosCoordinate = (ctx, coordinate) => {
 };
 
 
-const defaultAggregationQueryTemplate = ctx => `
+const aggregationQueryTemplate = ctx => `
 WITH __cdb_grid_params AS
 (
     ${gridInfoQuery(ctx)}
@@ -437,14 +412,7 @@ SELECT * FROM
 ${aggregatedPointJoin(ctx)}
 `;
 
-const aggregationQueryTemplates = {
-    'centroid': defaultAggregationQueryTemplate,
-    'point-grid': defaultAggregationQueryTemplate,
-    'point-sample': defaultAggregationQueryTemplate
-
-};
-
-module.exports.SUPPORTED_PLACEMENTS = Object.keys(aggregationQueryTemplates);
+module.exports.SUPPORTED_PLACEMENTS = ['centroid', 'point-grid', 'point-sample'];
 module.exports.GEOMETRY_COLUMN = 'the_geom_webmercator';
 
 const clusterFeaturesQuery = ctx => `

From de49aa0bd45ac75aa493fa5154a9385d87bcd80f Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Mon, 15 Jul 2019 15:54:31 +0200
Subject: [PATCH 17/19] Aggregation: Style improvements

---
 lib/cartodb/backends/cluster.js               |  6 ++--
 .../models/aggregation/aggregation-query.js   | 35 +++++++++++--------
 test/acceptance/aggregation.js                | 12 +++----
 3 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/lib/cartodb/backends/cluster.js b/lib/cartodb/backends/cluster.js
index 62ef86a9..24145b8d 100644
--- a/lib/cartodb/backends/cluster.js
+++ b/lib/cartodb/backends/cluster.js
@@ -6,7 +6,7 @@ const debug = require('debug')('backend:cluster');
 const AggregationMapConfig = require('../models/aggregation/aggregation-mapconfig');
 
 const windshaftUtils = require('windshaft').utils;
-const wmh = new windshaftUtils.WebMercatorHelper();
+const webmercator = new windshaftUtils.WebMercatorHelper();
 
 module.exports = class ClusterBackend {
     getClusterFeatures (mapConfigProvider, params, callback) {
@@ -159,8 +159,8 @@ const clusterFeaturesQuery = ctx => `
 `;
 
 const gridResolution = ctx => {
-    const zoom_resolution = wmh.getResolution({ z : Math.min(38, ctx.zoom) });
-    return `${256/ctx.res} * (${zoom_resolution})::double precision`;
+    const zoomResolution = webmercator.getResolution({ z : Math.min(38, ctx.zoom) });
+    return `${256/ctx.res} * (${zoomResolution})::double precision`;
 };
 
 const aggregationQuery = ctx => `
diff --git a/lib/cartodb/models/aggregation/aggregation-query.js b/lib/cartodb/models/aggregation/aggregation-query.js
index 6fd416c5..c70ec9dc 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -3,6 +3,8 @@
 const timeDimension = require('./time-dimension');
 
 const DEFAULT_PLACEMENT = 'point-sample';
+const windshaftUtils = require('windshaft').utils;
+const webmercator = new windshaftUtils.WebMercatorHelper();
 
 function optionsToParams (options) {
     return {
@@ -265,16 +267,18 @@ const havingClause = ctx => {
 // 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.
 //
-// NOTE: We'd rather use !scale_denominator!, but in Mapnik this value is extent / 256 for raster
+// NOTE: We'd rather use !pixel_width!, but in Mapnik this value is extent / 256 for raster
 // and extent / tile_extent {4096 default} for MVT, so since aggregations are always based
 // on 256 we can't have the same query in both cases
 // As this scale change doesn't happen in !scale_denominator! we use that instead
+// NOTE 2: The 0.00028 is used in Mapnik (and replicated in pg-mvt) and comes from
+// OGC's Styled Layer Descriptor Implementation Specification
 const gridResolution = ctx => {
-    const minimumResolution = 2*Math.PI*6378137/Math.pow(2,38);
+    const minimumResolution = webmercator.getResolution({ z : 38 });
     return `${256/ctx.res} * GREATEST(!scale_denominator! * 0.00028, ${minimumResolution})::double precision`;
 };
 
-// SQL query to extra the boundaries of the area to be aggregated and the grid resolution
+// SQL query to extract the boundaries of the area to be aggregated and the grid resolution
 // cdb_{x-y}{min_max} return the limits of the tile. Aggregations do [min, max) in both axis
 // cdb_res: Aggregation resolution (as specified by gridResolution)
 // cdb_point_bbox: Tile bounding box [min, max]
@@ -311,17 +315,17 @@ const aggregatedPointWebMercator = (ctx) => {
     switch (ctx.placement) {
 
         // For centroid, we return the average of the cell
-        case `centroid`:
-            return `, ST_SetSRID(ST_MakePoint(AVG(cdb_x), AVG(cdb_y)), 3857) AS the_geom_webmercator`;
+        case 'centroid':
+            return ', ST_SetSRID(ST_MakePoint(AVG(cdb_x), AVG(cdb_y)), 3857) AS the_geom_webmercator';
 
         // Middle point of the cell
-        case `point-grid`:
+        case 'point-grid':
             return `, ST_SetSRID(ST_MakePoint(cdb_pos_grid_x, cdb_pos_grid_y), 3857) AS the_geom_webmercator`;
 
         // For point-sample we'll get a single point directly from the source
         // If it's default aggregation we'll add the extra columns to keep backwards compatibility
-        case `point-sample`:
-            return ``;
+        case 'point-sample':
+            return '';
 
         default:
             throw new Error(`Invalid aggregation placement "${ctx.placement}`);
@@ -332,16 +336,17 @@ const aggregatedPointWebMercator = (ctx) => {
 const aggregatedPointJoin = (ctx) => {
     switch (ctx.placement) {
 
-        case `centroid`:
-            return ``;
+        case 'centroid':
+            return '';
 
-        case `point-grid`:
-            return ``;
+        case 'point-grid':
+            return '';
 
         // For point-sample we'll get a single point directly from the source
         // If it's default aggregation we'll add the extra columns to keep backwards compatibility
-        case `point-sample`:
-            return `NATURAL JOIN
+        case 'point-sample':
+            return `
+NATURAL JOIN
 (
     SELECT ${ctx.isDefaultAggregation ? `*` : `cartodb_id, the_geom_webmercator`}
     FROM
@@ -352,7 +357,7 @@ const aggregatedPointJoin = (ctx) => {
 `;
 
         default:
-            throw new Error(`Invalid aggregation placement "${ctx.placement}`);
+            throw new Error('Invalid aggregation placement "${ctx.placement}"');
     }
 };
 
diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index ea3f0261..c722a23c 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -7,7 +7,7 @@ const TestClient = require('../support/test-client');
 const serverOptions = require('../../lib/cartodb/server_options');
 
 const windshaftUtils = require('windshaft').utils;
-const wmh = new windshaftUtils.WebMercatorHelper();
+const webmercator = new windshaftUtils.WebMercatorHelper();
 
 const suites = [
     {
@@ -112,15 +112,15 @@ describe('aggregation', function () {
     WITH hgrid AS (
         SELECT
             CDB_RectangleGrid (
-                ST_Expand(!bbox!, ${wmh.getResolution({ z : 1 })} * 12),
-                ${wmh.getResolution({ z : 1 })} * 12,
-                ${wmh.getResolution({ z : 1 })} * 12
+                ST_Expand(!bbox!, ${webmercator.getResolution({ z : 1 })} * 12),
+                ${webmercator.getResolution({ z : 1 })} * 12,
+                ${webmercator.getResolution({ z : 1 })} * 12
             ) as cell
     )
     SELECT
         hgrid.cell as the_geom_webmercator,
         count(1) as agg_value,
-        count(1) /power( 12 * ${wmh.getResolution({ z : 1 })}, 2 ) as agg_value_density,
+        count(1) /power( 12 * ${webmercator.getResolution({ z : 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
@@ -207,7 +207,7 @@ describe('aggregation', function () {
     //
     const POINTS_SQL_GRID = (z, resolution) => `
         WITH params AS (
-            SELECT ${wmh.getResolution({ z : z })}*${resolution} AS l -- cell size for Z, resolution
+            SELECT ${webmercator.getResolution({ z : z })}*${resolution} AS l -- cell size for Z, resolution
         )
         SELECT
             row_number() OVER () AS cartodb_id,

From b572b979a1c372a64a0fbb811fd4ee33920024bb Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Tue, 16 Jul 2019 13:43:46 +0200
Subject: [PATCH 18/19] Style

---
 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 c70ec9dc..dbcbc616 100644
--- a/lib/cartodb/models/aggregation/aggregation-query.js
+++ b/lib/cartodb/models/aggregation/aggregation-query.js
@@ -328,7 +328,7 @@ const aggregatedPointWebMercator = (ctx) => {
             return '';
 
         default:
-            throw new Error(`Invalid aggregation placement "${ctx.placement}`);
+            throw new Error(`Invalid aggregation placement "${ctx.placement}"`);
     }
 };
 

From ffe347cfdcb1d0f9e5d8ce941a0c0c8eaafde4e0 Mon Sep 17 00:00:00 2001
From: Raul Marin <git@rmr.ninja>
Date: Tue, 16 Jul 2019 13:44:13 +0200
Subject: [PATCH 19/19] package.json: Revert changes to match master

---
 package-lock.json | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 13ff3a03..0bc4990b 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -2566,9 +2566,9 @@
       }
     },
     "lodash": {
-      "version": "4.17.14",
-      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.14.tgz",
-      "integrity": "sha512-mmKYbW3GLuJeX+iGP+Y7Gp1AiGHGbXHCOh/jZmrawMmsE7MS4znI3RL2FsjbqOyMayHInjOeykW7PEajUk1/xw=="
+      "version": "4.17.11",
+      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.11.tgz",
+      "integrity": "sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg=="
     },
     "lodash.assign": {
       "version": "4.2.0",
@@ -4098,9 +4098,9 @@
       "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8="
     },
     "xtend": {
-      "version": "4.0.2",
-      "resolved": "https://registry.npmjs.org/xtend/-/xtend-4.0.2.tgz",
-      "integrity": "sha512-LKYU1iAXJXUgAXn9URjiu+MWhyUXHsvfp7mcuYm9dSUKK0/CjtrUwFAxD82/mCWbtLsGjFIad0wIsod4zrTAEQ=="
+      "version": "4.0.1",
+      "resolved": "https://registry.npmjs.org/xtend/-/xtend-4.0.1.tgz",
+      "integrity": "sha1-pcbVMr5lbiPbgg77lDofBJmNY68="
     },
     "y18n": {
       "version": "3.2.1",