From 7561635b24e6dcc61c0dbcc31b897b187edf0dfc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 7 May 2018 19:03:19 +0200 Subject: [PATCH 01/38] WIP:add layer metadata --- .../layer-stats/mapnik-layer-stats.js | 170 +++++++++++++++++- lib/cartodb/utils/query-utils.js | 81 +++++++++ 2 files changed, 242 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index c060f964..d5806960 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -1,4 +1,5 @@ var queryUtils = require('../../utils/query-utils'); +const AggregationMapConfig = require('../../models/aggregation/aggregation-mapconfig'); function MapnikLayerStats () { this._types = { @@ -11,18 +12,169 @@ MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; +function queryPromise(dbConnection, query, callback) { + return new Promise(function(resolve, reject) { + dbConnection.query(query, function (err, res) { + err = callback(err, res); + if (err) { + reject(err); + } + else { + resolve(); + } + }); + + }); +} + MapnikLayerStats.prototype.getStats = function (layer, dbConnection, callback) { - var queryRowCountSql = queryUtils.getQueryRowCount(layer.options.sql); - // This query would gather stats for postgresql table if not exists - dbConnection.query(queryRowCountSql, function (err, res) { - if (err) { - return callback(null, {estimatedFeatureCount: -1}); - } else { - // We decided that the relation is 1 row == 1 feature - return callback(null, {estimatedFeatureCount: res.rows[0].rows}); + let query = layer.options.sql; + let rawQuery = layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql; + let metaOptions = layer.options.metadata || {}; + + let stats = {}; + + // TODO: could save some queries if queryUtils.getAggregationMetadata() has been used and kept somewhere + // we would set stats.estimatedFeatureCount and stats.geometryType (if metaOptions.geometryType) from it. + + // We'll add promises for queries to be executed to the next two lists; + // the queries in statQueries2 will be executed after all of statQueries are completed, + // so any results from them can be used. + // Query promises will store results in the shared stats object. + let statQueries = [], statQueries2 = []; + + if (stats.estimatedFeatureCount === undefined) { + statQueries.push( + queryPromise(dbConnection, queryUtils.getQueryRowEstimation(query), function(err, res) { + if (err) { + // at least for debugging we should err + stats.estimatedFeatureCount = -1; + return null; + } else { + // We decided that the relation is 1 row == 1 feature + stats.estimatedFeatureCount = res.rows[0].rows; + return null; + } + }) + ); + } + + if (metaOptions.featureCount) { + // TODO: if metaOptions.columnStats we can combine this with column stats query + statQueries.push( + queryPromise( + queryUtils.getQueryActualRowCount(rawQuery), + function(err, res) { + if (err) { + stats.featureCount = -1; + } else { + stats.featureCount = res.rows[0].rows; + } + return err; + } + ) + ); + } + + if (metaOptions.sample) { + const numRows = stats.featureCount === undefined ? stats.estimatedFeatureCount : stats.featureCount; + const sampleProb = Math.min(metaOptions.sample / numRows, 1); + statQueries2.push( + queryPromise( + queryUtils.getQuerySample(rawQuery, sampleProb), + function(err, res) { + if (err) { + stats.sample = []; + } else { + stats.sample = res.rows; + } + return err; + } + ) + ); + } + + if (metaOptions.geometryType && stats.geometryType === undefined) { + const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); + statQueries.push( + queryPromise(queryUtils.getQueryGeometryType(rawQuery, geometryColumn), function(err, res) { + if (!err) { + stats.geometryType = res.geom_type; + } + return err; + }) + ); + } + + function columnAggregations(field) { + if (field.type === 'number') { + return ['min', 'max', 'avg', 'sum']; } - }); + if (field.type === 'date') { // TODO other types too? + return ['min', 'max']; + } + } + + if (metaOptions.columns || metaOptions.columnStats) { + statQueries.push( + // TODO: note we have getLayerColumns in aggregation mapconfig. + // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters + queryPromise(queryUtils.getQueryLimited(rawQuery, 0), function(err, res) { + if (!err) { + stats.columns = res.fields; + if (metaOptions.columnStats) { + let aggr = []; + Object.keys(stats.columns).forEach(name => { + aggr = aggr.concat(columnAggregations(stats.columns[name]) + .map(fn => `${fn}(${name}) AS ${name}_${fn}`)); + if (stats.columns[name].type === 'string') { + statQueries2.push( + queryPromise(topQuery(rawQuery, name, N), function(err, res){ + if (!err) { + const topN = metaOptions.columnStats.topCategories || 1024; + // TODO: metaOptions.columnStats.maxCategories => use PG stats to dismiss columns with more distinct values + statQueries2.push( + queryPromise( + queryUtils.getQueryTopCategories(rawQuery, topN), + function(err, res) { + if (!err) { + stats.columns[name].categories = res.rows; + } + return err; + } + ) + ); + + } + return err; + }) + ); + } + }) + statQueries2.push( + queryPromise(`SELECT ${aggr.join(',')} FROM (${rawQuery})`, function(err, res){ + if (!err) { + Object.keys(stats.columns).forEach(name => { + columnAggregations(stats.columns[name]).forEach(fn => { + stats.columns[name][fn] = res.rows[0][`${name}_${fn}`] + }); + }); + } + return err; + }) + ); + } + } + return err; + }) + ); + + } + + Promise.all(statQueries).then( () => { + Promise.all(statQueries2).then( () => callback(null, stats) ).catch( err => callback(err) ); + }).catch( err => callback(err) ); }; module.exports = MapnikLayerStats; diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index c1b56276..e4bd67d3 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -71,3 +71,84 @@ module.exports.countNaNs = function countNaNs(ctx) { `sum(CASE WHEN (${ctx.column} = 'NaN'::float) THEN 1 ELSE 0 END)` }`; }; + +module.exports.getQueryTopCategories = function(query, column, topN, includeNulls=false) { + const where = includeNulls ? '' : `WHERE ${column} IS NOT NULL`; + return ` + SELECT ${column} AS category, COUNT(*) AS frequency + FROM (${query}) AS __cdb_query + ${where} + GROUP BY ${column} ORDER BY 2 DESC + LIMIT ${topN} + `; +} + +module.exports.getQueryActualRowCount = function (query) { + return 'select COUNT(*) AS rows FROM (${query}) AS __cdb_query'; +}; + + +module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { + const table = simpleQueryTable(query); + if (table) { + return getTableSample(table, sampleProb, randomSeed); + } + return ` + WITH __cdb_rndseed AS ( + SELECT setseed(${randomSeed}) + ) + SELECT * + FROM (${query}) AS __cdb_query + WHERE random() < $ + `; + q = `WITH _rndseed as (SELECT setseed(0.5)) + SELECT * FROM (${this._source._query}) as _cdb_query_wrapper WHERE random() < ${sampleProb};`; +}; + +module.exports.getTableSample = function(table, sampleProb, randomSeed) { + sampleProb *= 100; + randomSeed *= Math.pow(2, 31) -1; + return ` + SELECT * FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) + `; +} + +function simpleQueryTable(sql) { + const basicQuery = + /\s*SELECT\s+[\*a-z0-9_,\s]+?\s+FROM\s+((\"[^"]+\"|[a-z0-9_]+)\.)?(\"[^"]+\"|[a-z0-9_]+)\s*;?\s*/i; + const unwrappedQuery = new RegExp("^"+basicQuery.source+"$", 'i'); + // queries for named maps are wrapped like this: + var wrappedQuery = new RegExp( + "^\\s*SELECT\\s+\\*\\s+FROM\\s+\\(" + + basicQuery.source + + "\\)\\s+AS\\s+wrapped_query\\s+WHERE\\s+\\d+=1\\s*$", + 'i' + ); + let match = sql.match(unwrappedQuery); + if (!match) { + match = sql.match(wrappedQuery); + } + if (match) { + schema = match[2]; + table = match[3]; + return schema ? `${schema}.${table}` : table; + } + 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 + `; +}; + +module.exports.getQueryLimited = function(query, limit=0) { + return ` + SELECT * + FROM (${query}) AS __cdb_query + LIMIT ${limit} + `; +}; From ebab879acaf622ab3c3ddf5df83761d4e5ec3cbf Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 11:07:47 +0200 Subject: [PATCH 02/38] Fix bugs & typos --- lib/cartodb/utils/query-utils.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index e4bd67d3..238a905d 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -81,10 +81,10 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull GROUP BY ${column} ORDER BY 2 DESC LIMIT ${topN} `; -} +}; module.exports.getQueryActualRowCount = function (query) { - return 'select COUNT(*) AS rows FROM (${query}) AS __cdb_query'; + return `select COUNT(*) AS rows FROM (${query}) AS __cdb_query`; }; @@ -101,11 +101,9 @@ module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { FROM (${query}) AS __cdb_query WHERE random() < $ `; - q = `WITH _rndseed as (SELECT setseed(0.5)) - SELECT * FROM (${this._source._query}) as _cdb_query_wrapper WHERE random() < ${sampleProb};`; }; -module.exports.getTableSample = function(table, sampleProb, randomSeed) { +function getTableSample(table, sampleProb, randomSeed) { sampleProb *= 100; randomSeed *= Math.pow(2, 31) -1; return ` @@ -129,8 +127,8 @@ function simpleQueryTable(sql) { match = sql.match(wrappedQuery); } if (match) { - schema = match[2]; - table = match[3]; + const schema = match[2]; + const table = match[3]; return schema ? `${schema}.${table}` : table; } return false; From c647f852d64a3cdb3c2e8f14b80d3bc1c62b9b70 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 11:09:09 +0200 Subject: [PATCH 03/38] Refactor metadata queries execution Also fixed bug where sampling query generation needed results of count queries --- .../layer-stats/mapnik-layer-stats.js | 277 +++++++++--------- lib/cartodb/utils/phased-execution.js | 91 ++++++ 2 files changed, 231 insertions(+), 137 deletions(-) create mode 100644 lib/cartodb/utils/phased-execution.js diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index d5806960..047229af 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -1,4 +1,5 @@ var queryUtils = require('../../utils/query-utils'); +const PhasedExecution = require('../../utils/phased-execution'); const AggregationMapConfig = require('../../models/aggregation/aggregation-mapconfig'); function MapnikLayerStats () { @@ -27,154 +28,156 @@ function queryPromise(dbConnection, query, callback) { }); } +function columnAggregations(field) { + if (field.type === 'number') { + return ['min', 'max', 'avg', 'sum']; + } + if (field.type === 'date') { // TODO other types too? + return ['min', 'max']; + } +} + +function firstPhaseQueries(queries, ctx) { + if (queries.results.estimatedFeatureCount === undefined) { + queries.task( + queryPromise(ctx.dbConnection, queryUtils.getQueryRowEstimation(ctx.query), function(err, res) { + if (err) { + // at least for debugging we should err + queries.results.estimatedFeatureCount = -1; + return null; + } else { + // We decided that the relation is 1 row == 1 feature + queries.results.estimatedFeatureCount = res.rows[0].rows; + return null; + } + }) + ); + } + + if (ctx.metaOptions.featureCount) { + // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query + queries.task( + queryPromise( + queryUtils.getQueryActualRowCount(ctx.rawQuery), + function(err, res) { + if (err) { + queries.results.featureCount = -1; + } else { + queries.results.featureCount = res.rows[0].rows; + } + return err; + } + ) + ); + } + + if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { + const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); + queries.task( + queryPromise(queryUtils.getQueryGeometryType(ctx.rawQuery, geometryColumn), function(err, res) { + if (!err) { + queries.results.geometryType = res.geom_type; + } + return err; + }) + ); + } + + if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { + queries.task( + // TODO: note we have getLayerColumns in aggregation mapconfig. + // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters + queryPromise(queryUtils.getQueryLimited(ctx.rawQuery, 0), function(err, res) { + if (!err) { + queries.results.columns = res.fields; + } + return err; + }) + ); + } +} + +function secondPhaseQueries(queries, ctx) { + if (ctx.metaOptions.sample) { + const numRows = queries.results.featureCount === undefined ? + queries.results.estimatedFeatureCount : + queries.results.featureCount; + const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); + queries.task( + queryPromise( + queryUtils.getQuerySample(ctx.rawQuery, sampleProb), + function(err, res) { + if (err) { + queries.results.sample = []; + } else { + queries.results.sample = res.rows; + } + return err; + } + ) + ); + } + + if (ctx.metaOptions.columnStats) { + let aggr = []; + Object.keys(queries.results.columns).forEach(name => { + aggr = aggr.concat(columnAggregations(queries.results.columns[name]) + .map(fn => `${fn}(${name}) AS ${name}_${fn}`)); + if (queries.results.columns[name].type === 'string') { + const topN = ctx.metaOptions.columnStats.topCategories || 1024; + // TODO: ctx.metaOptions.columnStats.maxCategories + // => use PG stats to dismiss columns with more distinct values + queries.task( + queryPromise(queryUtils.getQueryTopCategories(ctx.rawQuery, name, topN), function(err, res){ + if (!err) { + queries.results.columns[name].categories = res.rows; + } + return err; + }) + ); + } + }); + queries.task( + queryPromise(`SELECT ${aggr.join(',')} FROM (${ctx.rawQuery})`, function(err, res){ + if (!err) { + Object.keys(queries.results.columns).forEach(name => { + columnAggregations(queries.results.columns[name]).forEach(fn => { + queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; + }); + }); + } + return err; + }) + ); + } + +} + MapnikLayerStats.prototype.getStats = function (layer, dbConnection, callback) { - let query = layer.options.sql; - let rawQuery = layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql; - let metaOptions = layer.options.metadata || {}; + let context = { + dbConnection, + query: layer.options.sql, + rawQuery: layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql, + metaOptions: layer.options.metadata || {} + }; - let stats = {}; + let queries = new PhasedExecution(); // TODO: could save some queries if queryUtils.getAggregationMetadata() has been used and kept somewhere - // we would set stats.estimatedFeatureCount and stats.geometryType (if metaOptions.geometryType) from it. + // we would set queries.results.estimatedFeatureCount and queries.results.geometryType + // (if metaOptions.geometryType) from it. // We'll add promises for queries to be executed to the next two lists; // the queries in statQueries2 will be executed after all of statQueries are completed, // so any results from them can be used. // Query promises will store results in the shared stats object. - let statQueries = [], statQueries2 = []; - if (stats.estimatedFeatureCount === undefined) { - statQueries.push( - queryPromise(dbConnection, queryUtils.getQueryRowEstimation(query), function(err, res) { - if (err) { - // at least for debugging we should err - stats.estimatedFeatureCount = -1; - return null; - } else { - // We decided that the relation is 1 row == 1 feature - stats.estimatedFeatureCount = res.rows[0].rows; - return null; - } - }) - ); - } - - if (metaOptions.featureCount) { - // TODO: if metaOptions.columnStats we can combine this with column stats query - statQueries.push( - queryPromise( - queryUtils.getQueryActualRowCount(rawQuery), - function(err, res) { - if (err) { - stats.featureCount = -1; - } else { - stats.featureCount = res.rows[0].rows; - } - return err; - } - ) - ); - } - - if (metaOptions.sample) { - const numRows = stats.featureCount === undefined ? stats.estimatedFeatureCount : stats.featureCount; - const sampleProb = Math.min(metaOptions.sample / numRows, 1); - statQueries2.push( - queryPromise( - queryUtils.getQuerySample(rawQuery, sampleProb), - function(err, res) { - if (err) { - stats.sample = []; - } else { - stats.sample = res.rows; - } - return err; - } - ) - ); - } - - if (metaOptions.geometryType && stats.geometryType === undefined) { - const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); - statQueries.push( - queryPromise(queryUtils.getQueryGeometryType(rawQuery, geometryColumn), function(err, res) { - if (!err) { - stats.geometryType = res.geom_type; - } - return err; - }) - ); - } - - function columnAggregations(field) { - if (field.type === 'number') { - return ['min', 'max', 'avg', 'sum']; - } - if (field.type === 'date') { // TODO other types too? - return ['min', 'max']; - } - } - - if (metaOptions.columns || metaOptions.columnStats) { - statQueries.push( - // TODO: note we have getLayerColumns in aggregation mapconfig. - // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters - queryPromise(queryUtils.getQueryLimited(rawQuery, 0), function(err, res) { - if (!err) { - stats.columns = res.fields; - if (metaOptions.columnStats) { - let aggr = []; - Object.keys(stats.columns).forEach(name => { - aggr = aggr.concat(columnAggregations(stats.columns[name]) - .map(fn => `${fn}(${name}) AS ${name}_${fn}`)); - if (stats.columns[name].type === 'string') { - statQueries2.push( - queryPromise(topQuery(rawQuery, name, N), function(err, res){ - if (!err) { - const topN = metaOptions.columnStats.topCategories || 1024; - // TODO: metaOptions.columnStats.maxCategories => use PG stats to dismiss columns with more distinct values - statQueries2.push( - queryPromise( - queryUtils.getQueryTopCategories(rawQuery, topN), - function(err, res) { - if (!err) { - stats.columns[name].categories = res.rows; - } - return err; - } - ) - ); - - } - return err; - }) - ); - } - }) - statQueries2.push( - queryPromise(`SELECT ${aggr.join(',')} FROM (${rawQuery})`, function(err, res){ - if (!err) { - Object.keys(stats.columns).forEach(name => { - columnAggregations(stats.columns[name]).forEach(fn => { - stats.columns[name][fn] = res.rows[0][`${name}_${fn}`] - }); - }); - } - return err; - }) - ); - } - } - return err; - }) - ); - - } - - Promise.all(statQueries).then( () => { - Promise.all(statQueries2).then( () => callback(null, stats) ).catch( err => callback(err) ); - }).catch( err => callback(err) ); + // Queries will be executed in two phases, with results from the first phase needed + // to define the queries of the second phase + queries.phase(() => firstPhaseQueries(queries, context)); + queries.phase(() => secondPhaseQueries(queries, context)); + queries.run(callback); }; module.exports = MapnikLayerStats; diff --git a/lib/cartodb/utils/phased-execution.js b/lib/cartodb/utils/phased-execution.js new file mode 100644 index 00000000..ec1de9b1 --- /dev/null +++ b/lib/cartodb/utils/phased-execution.js @@ -0,0 +1,91 @@ +/** + * PhasedExecution handles the execution of async tasks (via Promises) + * which have dependencies between them in a simplified manner. + * Instead of using the complete task dependency graph, tasks + * are organized into execution phases. So that tasks from a latter + * phase will be initialized after tasks from previous phases have + * finished. + * + * All tasks place their results in a shared object to make them + * available to tasks of latter phases. + * + * Each phase is defined by a function that defines its tasks. + * + * Example: + * + * let p = new PhasedExecution(); + * // Define first phase with tasks 1 & 2 + * p.phase(() => { + * p.results.phase1 = 1 + * p.task(new Promise((resolve) => { + * setTimeout( () => { + * console.log('At 1:', p.results); + * p.results.task1 = 100; + * resolve(); + * }, 400); + * })); + * p.task(new Promise((resolve) => { + * setTimeout( () => { + * console.log('At 2:', p.results); + * p.results.task2 = 200; + * resolve(); + * }, 100); + * })); + * }); + * // Define second phase with tasks 3 & 4 + * p.phase(() => { + * p.results.phase2 = 2 + * p.task(new Promise((resolve) => { + * setTimeout( () => { + * console.log('At 3:', p.results); + * p.results.task3 = 300; + * resolve(); + * }, 50); + * })); + * p.task(new Promise((resolve) => { + * setTimeout( () => { + * console.log('At 4:', p.results); + * p.results.task4 = 400; + * resolve(); + * }, 100); + * })); + * }); + * // Define third phase with task 5 + * p.phase(() => { + * p.results.phase3 = 3 + * p.task(new Promise((resolve) => { + * setTimeout( () => { + * console.log('At 5:', p.results); + * p.results.task5 = 500; + * resolve(); + * }, 50); + * })); + * }); + * // Execute all tasks + * p.run(() => { + * console.log("RESULTS:", p.results); + * }); + */ +module.exports = class PhasedExecution { + constructor() { + this.results = {}; + this.phases = []; + } + phase(phasegenerator) { + this.phases.push(phasegenerator); + } + task(promise) { + this.tasks.push(promise); + } + run(callback) { + this.tasks = []; + let phase = this.phases.shift(); + if (phase) { + phase(this); + return Promise.all(this.tasks) + .then(this.run()) + .then(() => { if (callback) { callback(this.results); } }); + } + } +}; +// TODO: error handling From 636cd8cd509124e38a97f350cb51f7ecd49770bc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 12:56:06 +0200 Subject: [PATCH 04/38] Fix:phase execution phase (not only its tasks) must be executed after the tasks of previous phases --- lib/cartodb/utils/phased-execution.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/utils/phased-execution.js b/lib/cartodb/utils/phased-execution.js index ec1de9b1..68fc7933 100644 --- a/lib/cartodb/utils/phased-execution.js +++ b/lib/cartodb/utils/phased-execution.js @@ -83,7 +83,7 @@ module.exports = class PhasedExecution { if (phase) { phase(this); return Promise.all(this.tasks) - .then(this.run()) + .then(() => this.run()) .then(() => { if (callback) { callback(this.results); } }); } } From b96be69a5c68df5af0753b4a6a6f4e5b8c28bcc0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 13:08:01 +0200 Subject: [PATCH 05/38] Clarify example --- lib/cartodb/utils/phased-execution.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/utils/phased-execution.js b/lib/cartodb/utils/phased-execution.js index 68fc7933..c9ac445d 100644 --- a/lib/cartodb/utils/phased-execution.js +++ b/lib/cartodb/utils/phased-execution.js @@ -16,17 +16,18 @@ * let p = new PhasedExecution(); * // Define first phase with tasks 1 & 2 * p.phase(() => { + * console.log('At phase I', p.results);* * p.results.phase1 = 1 * p.task(new Promise((resolve) => { * setTimeout( () => { - * console.log('At 1:', p.results); + * console.log('At task 1:', p.results); * p.results.task1 = 100; * resolve(); * }, 400); * })); * p.task(new Promise((resolve) => { * setTimeout( () => { - * console.log('At 2:', p.results); + * console.log('At task 2:', p.results); * p.results.task2 = 200; * resolve(); * }, 100); @@ -34,17 +35,18 @@ * }); * // Define second phase with tasks 3 & 4 * p.phase(() => { + * console.log('At phase II', p.results); * p.results.phase2 = 2 * p.task(new Promise((resolve) => { * setTimeout( () => { - * console.log('At 3:', p.results); + * console.log('At task 3:', p.results); * p.results.task3 = 300; * resolve(); * }, 50); * })); * p.task(new Promise((resolve) => { * setTimeout( () => { - * console.log('At 4:', p.results); + * console.log('At task 4:', p.results); * p.results.task4 = 400; * resolve(); * }, 100); @@ -52,18 +54,21 @@ * }); * // Define third phase with task 5 * p.phase(() => { + * console.log('At phase III', p.results); * p.results.phase3 = 3 * p.task(new Promise((resolve) => { * setTimeout( () => { - * console.log('At 5:', p.results); + * console.log('At task 5:', p.results); * p.results.task5 = 500; * resolve(); * }, 50); * })); * }); * // Execute all tasks - * p.run(() => { - * console.log("RESULTS:", p.results); + * p.run((results) => { + * console.log("RESULTS:", results); + * }).catch((err) => { + * console.log("ERROR:", error); * }); */ module.exports = class PhasedExecution { @@ -88,4 +93,3 @@ module.exports = class PhasedExecution { } } }; -// TODO: error handling From 7d68a2967fd38e4c5ecfbf9c0a7407763937f268 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 13:08:26 +0200 Subject: [PATCH 06/38] Fix: callback expected errors in first argument --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 047229af..a86f49be 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -177,7 +177,7 @@ function (layer, dbConnection, callback) { // to define the queries of the second phase queries.phase(() => firstPhaseQueries(queries, context)); queries.phase(() => secondPhaseQueries(queries, context)); - queries.run(callback); + queries.run(results => callback(null, results)).catch(error => callback(error)); }; module.exports = MapnikLayerStats; From 9c9cfd015d5b906e6ff160cead7cb3b79b2523b2 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 20:06:14 +0200 Subject: [PATCH 07/38] Add test for optional layer metadata --- .../stats/mapnik_stats_layergroup.js | 170 +++++++++++++++++- 1 file changed, 169 insertions(+), 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index cdac7bb1..549f10c9 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -49,7 +49,7 @@ describe('Create mapnik layergroup', function() { sql: [ 'select t1.cartodb_id, t1.the_geom, t1.the_geom_webmercator, t2.address', ' from test_table t1, test_table_2 t2', - ' where t1.cartodb_id = t2.cartodb_id;' + ' where t1.cartodb_id = t2.cartodb_id' ].join(''), cartocss_version: cartocssVersion, cartocss: cartocss @@ -276,4 +276,172 @@ describe('Create mapnik layergroup', function() { testClient.drain(done); }); }); + + function layerWithMetadata(layer, metadata) { + return Object.assign(layer, { + options: Object.assign(layer.options, { metadata }) + }); + } + + it('should provide columns as optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + columns: true + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + const expectedColumns = { + cartodb_id: { type: 'number' }, + the_geom: { type: 'geometry' }, + the_geom_webmercator: { type: 'geometry' }, + address: { type: 'string' } + }; + assert.deepEqual(layergroup.metadata.layers[0].meta.stats.columns, expectedColumns); + testClient.drain(done); + }); + }); + + it('should provide column stats as optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + columnStats: true + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + const expectedColumns = { + cartodb_id: { + type: 'number', + avg: 3, + max: 5, + min: 1, + sum: 15 + }, + the_geom: { type: 'geometry' }, + the_geom_webmercator: { type: 'geometry' }, + address: { + type: 'string', + categories: [ + { + category: "Calle de la Palma 72, Madrid, Spain", + frequency: 1 + }, + { + category: "Calle de Pérez Galdós 9, Madrid, Spain", + frequency: 1 + }, + { + category: "Calle Divino Pastor 12, Madrid, Spain", + frequency: 1 + }, + { + category: "Manuel Fernández y González 8, Madrid, Spain", + frequency: 1 + }, + { + category: "Plaza Conde de Toreno 2, Madrid, Spain", + frequency: 1 + } + ] + } + }; + assert.deepEqual(layergroup.metadata.layers[0].meta.stats.columns, expectedColumns); + testClient.drain(done); + }); + }); + + it('should provide row count as optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + featureCount: true + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + assert.equal(layergroup.metadata.layers[0].meta.stats.featureCount, 5); + testClient.drain(done); + }); + }); + + it('should provide geometry type as optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + geometryType: true + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, 'ST_Point'); + testClient.drain(done); + }); + }); + + it('should provide a sample as optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + sample: 3 + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + assert(layergroup.metadata.layers[0].meta.stats.sample.length > 0); + const expectedCols = [ 'cartodb_id', 'address', 'the_geom', 'the_geom_webmercator' ].sort(); + assert.deepEqual(Object.keys(layergroup.metadata.layers[0].meta.stats.sample[0]).sort(), expectedCols); + testClient.drain(done); + }); + }); + + it('should only provide requested optional metadata', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + geometryType: true, + featureCount: true + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + assert.ok(!err); + assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); + assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, 'ST_Point'); + assert.equal(layergroup.metadata.layers[0].meta.stats.featureCount, 5); + assert.equal(layergroup.metadata.layers[0].meta.stats.sample, undefined); + assert.equal(layergroup.metadata.layers[0].meta.stats.columns, undefined); + testClient.drain(done); + }); + }); }); From 741bcd1a8092cb40179f4a63c63a25e00da39ddb Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 20:07:20 +0200 Subject: [PATCH 08/38] Metadata fixes --- .../layer-stats/mapnik-layer-stats.js | 122 +++++++++++++----- lib/cartodb/utils/query-utils.js | 14 +- 2 files changed, 97 insertions(+), 39 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index a86f49be..1f7fce76 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -35,6 +35,7 @@ function columnAggregations(field) { if (field.type === 'date') { // TODO other types too? return ['min', 'max']; } + return []; } function firstPhaseQueries(queries, ctx) { @@ -58,8 +59,9 @@ function firstPhaseQueries(queries, ctx) { // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query queries.task( queryPromise( + ctx.dbConnection, queryUtils.getQueryActualRowCount(ctx.rawQuery), - function(err, res) { + (err, res) => { if (err) { queries.results.featureCount = -1; } else { @@ -74,12 +76,16 @@ function firstPhaseQueries(queries, ctx) { if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); queries.task( - queryPromise(queryUtils.getQueryGeometryType(ctx.rawQuery, geometryColumn), function(err, res) { - if (!err) { - queries.results.geometryType = res.geom_type; + queryPromise( + ctx.dbConnection, + queryUtils.getQueryGeometryType(ctx.rawQuery, geometryColumn), + (err, res) => { + if (!err) { + queries.results.geometryType = res.rows[0].geom_type; + } + return err; } - return err; - }) + ) ); } @@ -87,12 +93,16 @@ function firstPhaseQueries(queries, ctx) { queries.task( // TODO: note we have getLayerColumns in aggregation mapconfig. // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters - queryPromise(queryUtils.getQueryLimited(ctx.rawQuery, 0), function(err, res) { - if (!err) { - queries.results.columns = res.fields; + queryPromise( + ctx.dbConnection, + queryUtils.getQueryLimited(ctx.rawQuery, 0), + (err, res) => { + if (!err) { + queries.results.columns = formatResultFields(ctx.dbConnection, res.fields); + } + return err; } - return err; - }) + ) ); } } @@ -105,8 +115,9 @@ function secondPhaseQueries(queries, ctx) { const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); queries.task( queryPromise( + ctx.dbConnection, queryUtils.getQuerySample(ctx.rawQuery, sampleProb), - function(err, res) { + (err, res) => { if (err) { queries.results.sample = []; } else { @@ -121,38 +132,90 @@ function secondPhaseQueries(queries, ctx) { if (ctx.metaOptions.columnStats) { let aggr = []; Object.keys(queries.results.columns).forEach(name => { - aggr = aggr.concat(columnAggregations(queries.results.columns[name]) - .map(fn => `${fn}(${name}) AS ${name}_${fn}`)); + aggr = aggr.concat( + columnAggregations(queries.results.columns[name]) + .map(fn => `${fn}(${name}) AS ${name}_${fn}`) + ); if (queries.results.columns[name].type === 'string') { const topN = ctx.metaOptions.columnStats.topCategories || 1024; // TODO: ctx.metaOptions.columnStats.maxCategories // => use PG stats to dismiss columns with more distinct values queries.task( - queryPromise(queryUtils.getQueryTopCategories(ctx.rawQuery, name, topN), function(err, res){ - if (!err) { - queries.results.columns[name].categories = res.rows; + queryPromise( + ctx.dbConnection, + queryUtils.getQueryTopCategories(ctx.rawQuery, name, topN), + (err, res) => { + if (!err) { + queries.results.columns[name].categories = res.rows; + } + return err; } - return err; - }) + ) ); } }); queries.task( - queryPromise(`SELECT ${aggr.join(',')} FROM (${ctx.rawQuery})`, function(err, res){ - if (!err) { - Object.keys(queries.results.columns).forEach(name => { - columnAggregations(queries.results.columns[name]).forEach(fn => { - queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; + queryPromise( + ctx.dbConnection, + `SELECT ${aggr.join(',')} FROM (${ctx.rawQuery}) AS __cdb_query`, + (err, res) => { + if (!err) { + Object.keys(queries.results.columns).forEach(name => { + columnAggregations(queries.results.columns[name]).forEach(fn => { + queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; + }); }); - }); + } + return err; } - return err; - }) + ) ); } } +// This is adapted from SQL API: +function fieldType(cname) { + let tname; + switch (true) { + case /bool/.test(cname): + tname = 'boolean'; + break; + case /int|float|numeric/.test(cname): + tname = 'number'; + break; + case /text|char|unknown/.test(cname): + tname = 'string'; + break; + case /date|time/.test(cname): + tname = 'date'; + break; + default: + tname = cname; + } + if ( tname && cname.match(/^_/) ) { + tname += '[]'; + } + return tname; +} + +function formatResultFields(dbConnection, flds) { + flds = flds || []; + var nfields = {}; + for (var i=0; i firstPhaseQueries(queries, context)); diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 238a905d..c7cb131d 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -21,10 +21,15 @@ module.exports.extractTableNames = function extractTableNames(query) { ].join(''); }; +module.exports.getQueryActualRowCount = function (query) { + return `select COUNT(*) AS rows FROM (${query}) AS __cdb_query`; +}; + function getQueryRowEstimation(query) { return 'select CDB_EstimateRowCount($windshaft$' + query + '$windshaft$) as rows'; } -module.exports.getQueryRowCount = getQueryRowEstimation; + +module.exports.getQueryRowEstimation = getQueryRowEstimation; module.exports.getAggregationMetadata = ctx => ` WITH @@ -83,11 +88,6 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull `; }; -module.exports.getQueryActualRowCount = function (query) { - return `select COUNT(*) AS rows FROM (${query}) AS __cdb_query`; -}; - - module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { const table = simpleQueryTable(query); if (table) { @@ -99,7 +99,7 @@ module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { ) SELECT * FROM (${query}) AS __cdb_query - WHERE random() < $ + WHERE random() < ${sampleProb} `; }; From eea7bed2f37d06dad5393e6b123bf8110fd28ac3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 8 May 2018 20:41:42 +0200 Subject: [PATCH 09/38] Slightly more elegant results of queries --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 5 ++++- lib/cartodb/utils/phased-execution.js | 9 ++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 1f7fce76..b590f979 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -235,7 +235,10 @@ function (layer, dbConnection, callback) { // to define the queries of the second phase queries.phase(() => firstPhaseQueries(queries, context)); queries.phase(() => secondPhaseQueries(queries, context)); - queries.run(results => callback(null, results)).catch(error => callback(error)); + queries.run() + .then(results => callback(null, results)) + .catch(error => callback(error)); + }; module.exports = MapnikLayerStats; diff --git a/lib/cartodb/utils/phased-execution.js b/lib/cartodb/utils/phased-execution.js index c9ac445d..46dfbb39 100644 --- a/lib/cartodb/utils/phased-execution.js +++ b/lib/cartodb/utils/phased-execution.js @@ -65,7 +65,7 @@ * })); * }); * // Execute all tasks - * p.run((results) => { + * p.run().then((results) => { * console.log("RESULTS:", results); * }).catch((err) => { * console.log("ERROR:", error); @@ -82,14 +82,13 @@ module.exports = class PhasedExecution { task(promise) { this.tasks.push(promise); } - run(callback) { + run() { this.tasks = []; let phase = this.phases.shift(); if (phase) { phase(this); - return Promise.all(this.tasks) - .then(() => this.run()) - .then(() => { if (callback) { callback(this.results); } }); + return Promise.all(this.tasks).then(() => this.run()); } + return this.results; } }; From d8ef8cb12fd66673d973709c8d3ee23f1b5952d0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 11:08:02 +0200 Subject: [PATCH 10/38] Debug travis test failures --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index b590f979..ffef2423 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -236,7 +236,7 @@ function (layer, dbConnection, callback) { queries.phase(() => firstPhaseQueries(queries, context)); queries.phase(() => secondPhaseQueries(queries, context)); queries.run() - .then(results => callback(null, results)) + .then(() => callback(null, queries.results)) .catch(error => callback(error)); }; From 944ce80c1efeb92de759665657fd1d62b64def7a Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 11:42:53 +0200 Subject: [PATCH 11/38] Revert debugging change --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index ffef2423..b590f979 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -236,7 +236,7 @@ function (layer, dbConnection, callback) { queries.phase(() => firstPhaseQueries(queries, context)); queries.phase(() => secondPhaseQueries(queries, context)); queries.run() - .then(() => callback(null, queries.results)) + .then(results => callback(null, results)) .catch(error => callback(error)); }; From d706d0eb222ca2d6d6027a27714750199666fab0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 11:44:49 +0200 Subject: [PATCH 12/38] More travis debugging through commits --- test/acceptance/stats/mapnik_stats_layergroup.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 549f10c9..3068c969 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -296,7 +296,6 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); - assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { cartodb_id: { type: 'number' }, the_geom: { type: 'geometry' }, @@ -321,7 +320,6 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); - assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { cartodb_id: { type: 'number', @@ -376,7 +374,6 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); - assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[0].meta.stats.featureCount, 5); testClient.drain(done); }); From fff5b3d85a4847506fb6f8a458ac8925ddedcfa3 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 11:59:24 +0200 Subject: [PATCH 13/38] Revert debugging changes --- test/acceptance/stats/mapnik_stats_layergroup.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 3068c969..549f10c9 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -296,6 +296,7 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { cartodb_id: { type: 'number' }, the_geom: { type: 'geometry' }, @@ -320,6 +321,7 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { cartodb_id: { type: 'number', @@ -374,6 +376,7 @@ describe('Create mapnik layergroup', function() { testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); + assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[0].meta.stats.featureCount, 5); testClient.drain(done); }); From ee7bd5fb8a171b789c4d701e6d219e6d092a5763 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 12:42:42 +0200 Subject: [PATCH 14/38] Fix tests --- test/acceptance/stats/mapnik_stats_layergroup.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 549f10c9..f5030783 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -273,6 +273,7 @@ describe('Create mapnik layergroup', function() { assert.ok(!layergroup.metadata.layers[1].meta.hasOwnProperty('stats')); assert.equal(layergroup.metadata.layers[2].id, typeLayerId('http', 1)); assert.equal(layergroup.metadata.layers[2].type, 'http'); + global.environment.enabledFeatures.layerStats = true; testClient.drain(done); }); }); From f7745928abaa01d2ac5500541e1f696ece40556d Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 9 May 2018 15:42:41 +0200 Subject: [PATCH 15/38] Fix tests eliminate dependency on the order of PostgreSQL results --- .../stats/mapnik_stats_layergroup.js | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index f5030783..508878cf 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -319,6 +319,32 @@ describe('Create mapnik layergroup', function() { ] }); + // metadata categories are ordered only partially by descending frequency; + // this orders them completely to avoid ambiguities when comparing + function withSortedCategories(columns) { + function catOrder(a, b) { + if (a.frequency !== b.frequency) { + return b.frequency - a.frequency; + } + if (a.category < b.category) { + return -1; + } + if (a.category > b.category) { + return +1; + } + return 0; + } + let sorted = {}; + Object.keys(columns).forEach(name => { + let data = columns[name]; + if (data.hasOwnProperty('categories')) { + data = Object.assign(data, { categories: data.categories.sort(catOrder)}); + } + sorted[name] = data; + }); + return sorted; + } + testClient.getLayergroup(function(err, layergroup) { assert.ok(!err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); @@ -359,7 +385,10 @@ describe('Create mapnik layergroup', function() { ] } }; - assert.deepEqual(layergroup.metadata.layers[0].meta.stats.columns, expectedColumns); + assert.deepEqual( + withSortedCategories(layergroup.metadata.layers[0].meta.stats.columns), + withSortedCategories(expectedColumns) + ); testClient.drain(done); }); }); From cae4dd81c9b370e70b42521e752196681bd84d02 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 10 May 2018 19:12:47 +0200 Subject: [PATCH 16/38] WIP: fix problems for aggregations & metadata --- .../layer-stats/mapnik-layer-stats.js | 32 +++++++++++++------ .../adapter/aggregation-mapconfig-adapter.js | 14 ++++++-- .../stats/mapnik_stats_layergroup.js | 2 ++ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index b590f979..ac62ac2b 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -9,6 +9,8 @@ function MapnikLayerStats () { }; } +// TODO: estimatedFeatureCount is post-aggregation; the rest is pre; distinguish and complement + MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; @@ -41,7 +43,7 @@ function columnAggregations(field) { function firstPhaseQueries(queries, ctx) { if (queries.results.estimatedFeatureCount === undefined) { queries.task( - queryPromise(ctx.dbConnection, queryUtils.getQueryRowEstimation(ctx.query), function(err, res) { + queryPromise(ctx.dbConnection, queryUtils.getQueryRowEstimation(ctx.aggrQuery), function(err, res) { if (err) { // at least for debugging we should err queries.results.estimatedFeatureCount = -1; @@ -56,11 +58,13 @@ function firstPhaseQueries(queries, ctx) { } if (ctx.metaOptions.featureCount) { + // TODO: pre/aggr + // TODO: for pre, use ctx.aggrMeta.pre_aggregation_count // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query queries.task( queryPromise( ctx.dbConnection, - queryUtils.getQueryActualRowCount(ctx.rawQuery), + queryUtils.getQueryActualRowCount(ctx.preQuery), (err, res) => { if (err) { queries.results.featureCount = -1; @@ -74,11 +78,13 @@ function firstPhaseQueries(queries, ctx) { } if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { + // TODO: pre/aggr + // TODO: for pre, use ctx.aggrMeta.geometry_type const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); queries.task( queryPromise( ctx.dbConnection, - queryUtils.getQueryGeometryType(ctx.rawQuery, geometryColumn), + queryUtils.getQueryGeometryType(ctx.preQuery, geometryColumn), (err, res) => { if (!err) { queries.results.geometryType = res.rows[0].geom_type; @@ -90,12 +96,15 @@ function firstPhaseQueries(queries, ctx) { } if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { + // TODO: pre/aggr + // TODO: for aggr, use layer.options.columns (will need to pass in ctx) + // note: post-aggregation columns are in layer.options.columns when aggregation is present queries.task( // TODO: note we have getLayerColumns in aggregation mapconfig. // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters queryPromise( ctx.dbConnection, - queryUtils.getQueryLimited(ctx.rawQuery, 0), + queryUtils.getQueryLimited(ctx.preQuery, 0), (err, res) => { if (!err) { queries.results.columns = formatResultFields(ctx.dbConnection, res.fields); @@ -116,7 +125,7 @@ function secondPhaseQueries(queries, ctx) { queries.task( queryPromise( ctx.dbConnection, - queryUtils.getQuerySample(ctx.rawQuery, sampleProb), + queryUtils.getQuerySample(ctx.preQuery, sampleProb), (err, res) => { if (err) { queries.results.sample = []; @@ -130,6 +139,7 @@ function secondPhaseQueries(queries, ctx) { } if (ctx.metaOptions.columnStats) { + // TODO: pre/aggr let aggr = []; Object.keys(queries.results.columns).forEach(name => { aggr = aggr.concat( @@ -143,7 +153,7 @@ function secondPhaseQueries(queries, ctx) { queries.task( queryPromise( ctx.dbConnection, - queryUtils.getQueryTopCategories(ctx.rawQuery, name, topN), + queryUtils.getQueryTopCategories(ctx.preQuery, name, topN), (err, res) => { if (!err) { queries.results.columns[name].categories = res.rows; @@ -157,7 +167,7 @@ function secondPhaseQueries(queries, ctx) { queries.task( queryPromise( ctx.dbConnection, - `SELECT ${aggr.join(',')} FROM (${ctx.rawQuery}) AS __cdb_query`, + `SELECT ${aggr.join(',')} FROM (${ctx.preQuery}) AS __cdb_query`, (err, res) => { if (!err) { Object.keys(queries.results.columns).forEach(name => { @@ -218,10 +228,14 @@ function formatResultFields(dbConnection, flds) { MapnikLayerStats.prototype.getStats = function (layer, dbConnection, callback) { + let aggrQuery = layer.options.sql_raw || layer.options.sql; + let preQuery = layer.options.aggregation_metadata ? layer.options.aggregation_metadata.pre_aggregation_sql : aggrQuery; + let context = { dbConnection, - query: layer.options.sql, - rawQuery: layer.options.sql_raw ? layer.options.sql_raw : layer.options.sql, + preQuery, + aggrQuery, + aggrMeta: layer.options.aggregation_metadata, metaOptions: layer.options.metadata || {} }; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 67c5eac4..8639b85b 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -73,7 +73,7 @@ module.exports = class AggregationMapConfigAdapter { if (adapted) { requestMapConfig.layers[index] = layer; } - const aggregatedFormats = this._getAggregationMetadata(mapConfig, layer, adapted); + const aggregatedFormats = this._getAggregationMetadata(mapConfig, layer, adapted); // <<- context.aggregation.layers.push(aggregatedFormats); }); @@ -84,7 +84,7 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, mapConfig, layer, index) { return new Promise((resolve, reject) => { - this._shouldAdaptLayer(connection, mapConfig, layer, index, (err, shouldAdapt) => { + this._shouldAdaptLayer(connection, mapConfig, layer, index, (err, shouldAdapt, aggrMeta) => { if (err) { return reject(err); } @@ -93,6 +93,7 @@ module.exports = class AggregationMapConfigAdapter { return resolve({ layer, index, adapted: shouldAdapt }); } + const sqlQuery = layer.options.sql; const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = mapConfig.getAggregatedQuery(index); @@ -110,6 +111,12 @@ module.exports = class AggregationMapConfigAdapter { layer.options.columns = columns; + layer.options.aggregation_metadata = { + pre_aggregation_sql: sqlQueryWrap || sqlQuery, + geometry_type: aggrMeta.type, + pre_aggregation_count: aggrMeta.count + }; + return resolve({ layer, index, adapted: shouldAdapt }); }); }); @@ -154,11 +161,12 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, false); } - callback(null, true); + callback(null, true, result); }); } _getAggregationMetadata (mapConfig, layer, adapted) { + // also: pre-aggr query, columns, ... if (!adapted) { return { png: false, mvt: false }; } diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 508878cf..d8a9a06b 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -474,4 +474,6 @@ describe('Create mapnik layergroup', function() { testClient.drain(done); }); }); + + // TODO: add tests for metadata with aggregation }); From 68b3cb8a34d0050edda23e845d83ee61facd2420 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 13:44:43 +0200 Subject: [PATCH 17/38] Fix estimated row count with aggregations All stats are computed now pre-aggregation Code to help compute post-aggregation stats remains for testing --- .../layer-stats/mapnik-layer-stats.js | 211 ++++++++++-------- 1 file changed, 113 insertions(+), 98 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index ac62ac2b..aada928a 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -1,6 +1,24 @@ var queryUtils = require('../../utils/query-utils'); const PhasedExecution = require('../../utils/phased-execution'); const AggregationMapConfig = require('../../models/aggregation/aggregation-mapconfig'); +var SubstitutionTokens = require('../../utils/substitution-tokens'); + +// Instantiate a query with tokens for a given zoom level +function queryForZoom(sql, zoom) { + 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; + return SubstitutionTokens.replace(sql, { + bbox: `ST_MakeEnvelope(${x0}, ${y0}, ${x0 + tileSize}, ${y0 + tileSize})`, + scale_denominator: scaleDenominator, + pixel_width: resolution, + pixel_height: resolution + }); +} function MapnikLayerStats () { this._types = { @@ -9,8 +27,6 @@ function MapnikLayerStats () { }; } -// TODO: estimatedFeatureCount is post-aggregation; the rest is pre; distinguish and complement - MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; @@ -40,106 +56,109 @@ function columnAggregations(field) { return []; } +/* Helper to add a task to the queries PhasedExecution + * type can be either 'pre' (for pre-aggregation metadata) or 'post' + * zoom is used only for post-aggregation metadata + * query is a function that generates a metadata query from a data query + * assign is a function to assign the results of the metadata query + * if a assignDefault function is present, it will be used in case of error + * during the query execution and any errors will be ignored + */ +function addStat(queries, ctx, type, zoom, query, assign, assignDefault=null) { + let sql; + if (type === 'pre') { + sql = ctx. preQuery; + } + else { + sql = queryForZoom(ctx.aggrQuery, zoom); + } + sql = query(sql); + queries.task( + queryPromise( + ctx.dbConnection, + sql, + (err, res) => { + if (!err) { + assign(res); + } + else if (assignDefault !== null) { + assignDefault(); + return null; + } + return err; + } + ) + ); +} function firstPhaseQueries(queries, ctx) { + // estimatedFeatureCount if (queries.results.estimatedFeatureCount === undefined) { - queries.task( - queryPromise(ctx.dbConnection, queryUtils.getQueryRowEstimation(ctx.aggrQuery), function(err, res) { - if (err) { - // at least for debugging we should err - queries.results.estimatedFeatureCount = -1; - return null; - } else { - // We decided that the relation is 1 row == 1 feature - queries.results.estimatedFeatureCount = res.rows[0].rows; - return null; - } - }) + // This is always computed; a default value of -1 is used in case of error + addStat( + queries, + ctx, + 'pre', 0, + queryUtils.getQueryRowEstimation, + res => queries.results.estimatedFeatureCount = res.rows[0].rows, + () => queries.results.estimatedFeatureCount = -1 ); } + // featureCount if (ctx.metaOptions.featureCount) { - // TODO: pre/aggr - // TODO: for pre, use ctx.aggrMeta.pre_aggregation_count // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query - queries.task( - queryPromise( - ctx.dbConnection, - queryUtils.getQueryActualRowCount(ctx.preQuery), - (err, res) => { - if (err) { - queries.results.featureCount = -1; - } else { - queries.results.featureCount = res.rows[0].rows; - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + queryUtils.getQueryActualRowCount, + res => queries.results.featureCount = res.rows[0].rows ); } + // geometryType if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { - // TODO: pre/aggr - // TODO: for pre, use ctx.aggrMeta.geometry_type const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); - queries.task( - queryPromise( - ctx.dbConnection, - queryUtils.getQueryGeometryType(ctx.preQuery, geometryColumn), - (err, res) => { - if (!err) { - queries.results.geometryType = res.rows[0].geom_type; - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + sql => queryUtils.getQueryGeometryType(sql, geometryColumn), + res => queries.results.geometryType = res.rows[0].geom_type ); } + // columns (names & types) if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { - // TODO: pre/aggr - // TODO: for aggr, use layer.options.columns (will need to pass in ctx) // note: post-aggregation columns are in layer.options.columns when aggregation is present - queries.task( - // TODO: note we have getLayerColumns in aggregation mapconfig. - // and also getLayerAggregationColumns which either uses getLayerColumns or derives columns from parameters - queryPromise( - ctx.dbConnection, - queryUtils.getQueryLimited(ctx.preQuery, 0), - (err, res) => { - if (!err) { - queries.results.columns = formatResultFields(ctx.dbConnection, res.fields); - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + sql => queryUtils.getQueryLimited(sql, 0), + res => queries.results.columns = formatResultFields(ctx.dbConnection, res.fields) ); } } function secondPhaseQueries(queries, ctx) { + // sample if (ctx.metaOptions.sample) { const numRows = queries.results.featureCount === undefined ? queries.results.estimatedFeatureCount : queries.results.featureCount; const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); - queries.task( - queryPromise( - ctx.dbConnection, - queryUtils.getQuerySample(ctx.preQuery, sampleProb), - (err, res) => { - if (err) { - queries.results.sample = []; - } else { - queries.results.sample = res.rows; - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + sql => queryUtils.getQuerySample(sql, sampleProb), + res => queries.results.sample = res.rows ); } + // columnStats if (ctx.metaOptions.columnStats) { - // TODO: pre/aggr let aggr = []; Object.keys(queries.results.columns).forEach(name => { aggr = aggr.concat( @@ -150,35 +169,27 @@ function secondPhaseQueries(queries, ctx) { const topN = ctx.metaOptions.columnStats.topCategories || 1024; // TODO: ctx.metaOptions.columnStats.maxCategories // => use PG stats to dismiss columns with more distinct values - queries.task( - queryPromise( - ctx.dbConnection, - queryUtils.getQueryTopCategories(ctx.preQuery, name, topN), - (err, res) => { - if (!err) { - queries.results.columns[name].categories = res.rows; - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + sql => queryUtils.getQueryTopCategories(sql, name, topN), + res => queries.results.columns[name].categories = res.rows ); } }); - queries.task( - queryPromise( - ctx.dbConnection, - `SELECT ${aggr.join(',')} FROM (${ctx.preQuery}) AS __cdb_query`, - (err, res) => { - if (!err) { - Object.keys(queries.results.columns).forEach(name => { - columnAggregations(queries.results.columns[name]).forEach(fn => { - queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; - }); - }); - } - return err; - } - ) + addStat( + queries, + ctx, + 'pre', 0, + sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`, + res => { + Object.keys(queries.results.columns).forEach(name => { + columnAggregations(queries.results.columns[name]).forEach(fn => { + queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; + }); + }); + } ); } @@ -209,6 +220,8 @@ function fieldType(cname) { return tname; } +// columns are returned as an object { columnName1: { type1: ...}, ..} +// for consistency with SQL API function formatResultFields(dbConnection, flds) { flds = flds || []; var nfields = {}; @@ -229,7 +242,9 @@ function formatResultFields(dbConnection, flds) { MapnikLayerStats.prototype.getStats = function (layer, dbConnection, callback) { let aggrQuery = layer.options.sql_raw || layer.options.sql; - let preQuery = layer.options.aggregation_metadata ? layer.options.aggregation_metadata.pre_aggregation_sql : aggrQuery; + let preQuery = layer.options.aggregation_metadata ? + layer.options.aggregation_metadata.pre_aggregation_sql : + aggrQuery; let context = { dbConnection, From 34ad3fcfe85d6b2762fa6f76031251cf473c9757 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 14:18:31 +0200 Subject: [PATCH 18/38] Add aggregated stat for testing Also change aggregated stats to not filter a single tile --- .../layer-stats/mapnik-layer-stats.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index aada928a..b5818bc7 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -4,7 +4,7 @@ const AggregationMapConfig = require('../../models/aggregation/aggregation-mapco var SubstitutionTokens = require('../../utils/substitution-tokens'); // Instantiate a query with tokens for a given zoom level -function queryForZoom(sql, zoom) { +function queryForZoom(sql, zoom, singleTile=false) { const tileRes = 256; const wmSize = 6378137.0*2*Math.PI; const nTiles = Math.pow(2, zoom); @@ -12,8 +12,12 @@ function queryForZoom(sql, zoom) { 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})`; + } return SubstitutionTokens.replace(sql, { - bbox: `ST_MakeEnvelope(${x0}, ${y0}, ${x0 + tileSize}, ${y0 + tileSize})`, + bbox: bbox, scale_denominator: scaleDenominator, pixel_width: resolution, pixel_height: resolution @@ -116,6 +120,17 @@ function firstPhaseQueries(queries, ctx) { ); } + // aggrFeatureCount + if (ctx.metaOptions.hasOwnProperty('aggrFeatureCount')) { + addStat( + queries, + ctx, + 'post', ctx.metaOptions.aggrFeatureCount || 0, + queryUtils.getQueryActualRowCount, + res => queries.results.aggrfeatureCount = res.rows[0].rows + ); + } + // geometryType if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); From 3b4668cc19651e8591b2e985304de215ebf95d59 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 14:45:12 +0200 Subject: [PATCH 19/38] Fix simple tabley sampling --- lib/cartodb/utils/query-utils.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index c7cb131d..0bbd38c0 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -89,9 +89,9 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull }; module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { - const table = simpleQueryTable(query); - if (table) { - return getTableSample(table, sampleProb, randomSeed); + const singleTable = simpleQueryTable(query); + if (singleTable) { + return getTableSample(singleTable.table, singleTable.columns, sampleProb, randomSeed); } return ` WITH __cdb_rndseed AS ( @@ -103,17 +103,17 @@ module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { `; }; -function getTableSample(table, sampleProb, randomSeed) { +function getTableSample(table, columns, sampleProb, randomSeed) { sampleProb *= 100; randomSeed *= Math.pow(2, 31) -1; return ` - SELECT * FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) + SELECT ${columns} FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) `; } function simpleQueryTable(sql) { const basicQuery = - /\s*SELECT\s+[\*a-z0-9_,\s]+?\s+FROM\s+((\"[^"]+\"|[a-z0-9_]+)\.)?(\"[^"]+\"|[a-z0-9_]+)\s*;?\s*/i; + /\s*SELECT\s+([\*a-z0-9_,\s]+?)\s+FROM\s+((\"[^"]+\"|[a-z0-9_]+)\.)?(\"[^"]+\"|[a-z0-9_]+)\s*;?\s*/i; const unwrappedQuery = new RegExp("^"+basicQuery.source+"$", 'i'); // queries for named maps are wrapped like this: var wrappedQuery = new RegExp( @@ -127,9 +127,10 @@ function simpleQueryTable(sql) { match = sql.match(wrappedQuery); } if (match) { - const schema = match[2]; - const table = match[3]; - return schema ? `${schema}.${table}` : table; + const columns = match[1]; + const schema = match[3]; + const table = match[4]; + return { table: schema ? `${schema}.${table}` : table, columns }; } return false; } From 53fae9fbbdb4da34f3456a48fcceef9b08063865 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 18:57:14 +0200 Subject: [PATCH 20/38] Comment --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index b5818bc7..49988aa8 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -122,6 +122,9 @@ function firstPhaseQueries(queries, ctx) { // aggrFeatureCount if (ctx.metaOptions.hasOwnProperty('aggrFeatureCount')) { + // We expect as zoom level as the value of aggrFeatureCount + // TODO: it'd be nice to admit an array of zoom levels to + // return metadata for multiple levels. addStat( queries, ctx, From b906f88a4407fe9211605f761b33eb505d720155 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 19:32:03 +0200 Subject: [PATCH 21/38] Slight refactor --- .../layer-stats/mapnik-layer-stats.js | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 49988aa8..6a71435e 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -94,8 +94,8 @@ function addStat(queries, ctx, type, zoom, query, assign, assignDefault=null) { ) ); } -function firstPhaseQueries(queries, ctx) { - // estimatedFeatureCount + +function _estimatedFeatureCount(queries, ctx) { if (queries.results.estimatedFeatureCount === undefined) { // This is always computed; a default value of -1 is used in case of error addStat( @@ -107,8 +107,9 @@ function firstPhaseQueries(queries, ctx) { () => queries.results.estimatedFeatureCount = -1 ); } +} - // featureCount +function _featureCount(queries, ctx) { if (ctx.metaOptions.featureCount) { // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query addStat( @@ -119,8 +120,9 @@ function firstPhaseQueries(queries, ctx) { res => queries.results.featureCount = res.rows[0].rows ); } +} - // aggrFeatureCount +function _aggrFeatureCount(queries, ctx) { if (ctx.metaOptions.hasOwnProperty('aggrFeatureCount')) { // We expect as zoom level as the value of aggrFeatureCount // TODO: it'd be nice to admit an array of zoom levels to @@ -133,8 +135,9 @@ function firstPhaseQueries(queries, ctx) { res => queries.results.aggrfeatureCount = res.rows[0].rows ); } +} - // geometryType +function _geometryType(queries, ctx) { if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); addStat( @@ -145,8 +148,9 @@ function firstPhaseQueries(queries, ctx) { res => queries.results.geometryType = res.rows[0].geom_type ); } +} - // columns (names & types) +function _columns(queries, ctx) { if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { // note: post-aggregation columns are in layer.options.columns when aggregation is present addStat( @@ -159,8 +163,15 @@ function firstPhaseQueries(queries, ctx) { } } -function secondPhaseQueries(queries, ctx) { - // sample +function firstPhaseQueries(queries, ctx) { + _estimatedFeatureCount(queries, ctx); + _featureCount(queries, ctx); + _aggrFeatureCount(queries, ctx); + _geometryType(queries, ctx); + _columns(queries, ctx); +} + +function _sample(queries, ctx) { if (ctx.metaOptions.sample) { const numRows = queries.results.featureCount === undefined ? queries.results.estimatedFeatureCount : @@ -174,8 +185,9 @@ function secondPhaseQueries(queries, ctx) { res => queries.results.sample = res.rows ); } +} - // columnStats +function _columnStats(queries, ctx) { if (ctx.metaOptions.columnStats) { let aggr = []; Object.keys(queries.results.columns).forEach(name => { @@ -210,7 +222,11 @@ function secondPhaseQueries(queries, ctx) { } ); } +} +function secondPhaseQueries(queries, ctx) { + _sample(queries, ctx); + _columnStats(queries, ctx); } // This is adapted from SQL API: From 5e09c80b717e6b7261e48e97850b54887028f81c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 11 May 2018 19:57:49 +0200 Subject: [PATCH 22/38] Remove comment --- .../models/mapconfig/adapter/aggregation-mapconfig-adapter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index 8639b85b..b3df3d96 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -73,7 +73,7 @@ module.exports = class AggregationMapConfigAdapter { if (adapted) { requestMapConfig.layers[index] = layer; } - const aggregatedFormats = this._getAggregationMetadata(mapConfig, layer, adapted); // <<- + const aggregatedFormats = this._getAggregationMetadata(mapConfig, layer, adapted); context.aggregation.layers.push(aggregatedFormats); }); From b8109401d1bf83d723e21249d4af9c71fa13bf4f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Sun, 13 May 2018 13:05:39 +0200 Subject: [PATCH 23/38] Tests for metadata with aggregation --- test/acceptance/aggregation.js | 176 ++++++++++++++++++ .../stats/mapnik_stats_layergroup.js | 1 - 2 files changed, 176 insertions(+), 1 deletion(-) diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 6658eb81..6aab8d95 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -100,6 +100,18 @@ describe('aggregation', function () { from generate_series(-3, 3) x `; + const POINTS_SQL_PAIRS = ` + -- Generate pairs of near points + select + x + 4 as cartodb_id, + st_setsrid(st_makepoint(Floor(x/2)*10 + x/1000.0, Floor(x/2)*10 + x/1000.0), 4326) as the_geom, + st_transform( + st_setsrid(st_makepoint(Floor(x/2)*10 + x/1000.0, Floor(x/2)*10 + x/1000.0),4326), + 3857) as the_geom_webmercator, + x as value + from generate_series(-6, 6) x + `; + function createVectorMapConfig (layers = [ { type: 'cartodb', @@ -130,6 +142,7 @@ describe('aggregation', function () { before(function () { serverOptions.renderer.mvt.usePostGIS = usePostGIS; + this.layerStatsConfig = global.environment.enabledFeatures.layerStats; }); after(function (){ @@ -138,6 +151,7 @@ describe('aggregation', function () { afterEach(function (done) { this.testClient.drain(done); + global.environment.enabledFeatures.layerStats = this.layerStatsConfig; }); it('should return a layergroup indicating the mapconfig was aggregated', function (done) { @@ -2173,6 +2187,168 @@ describe('aggregation', function () { }); }); + ['default', 'centroid', 'point-sample', 'point-grid'].forEach(placement => { + it(`default pre-aggregation stats are available with ${placement} aggregation`, function (done) { + global.environment.enabledFeatures.layerStats = true; + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_PAIRS, + resolution: 1, + aggregation: { + threshold: 1 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.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.ok(body.metadata.layers[0].meta.aggregation.mvt); + assert.ok(body.metadata.layers[0].meta.stats.estimatedFeatureCount > 0); + + done(); + }); + }); + + it(`on demand post-aggregation stats are available with ${placement} aggregation`, function (done) { + global.environment.enabledFeatures.layerStats = true; + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_PAIRS, + resolution: 1, + aggregation: { + threshold: 1 + }, + metadata: { + aggrFeatureCount: 10 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.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.ok(body.metadata.layers[0].meta.aggregation.mvt); + assert.equal(body.metadata.layers[0].meta.stats.aggrfeatureCount, 13); + + done(); + }); + }); + + it(`post-aggregation count adapts to zoom level with ${placement} aggregation`, function (done) { + global.environment.enabledFeatures.layerStats = true; + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_PAIRS, + resolution: 1, + aggregation: { + threshold: 1 + }, + metadata: { + aggrFeatureCount: 0 + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.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.ok(body.metadata.layers[0].meta.aggregation.mvt); + assert.equal(body.metadata.layers[0].meta.stats.aggrfeatureCount, 9); + + done(); + }); + }); + + it(`on-demand pre-aggregation stats are available with ${placement} aggregation`, function (done) { + global.environment.enabledFeatures.layerStats = true; + this.mapConfig = { + version: '1.6.0', + buffersize: { 'mvt': 0 }, + layers: [ + { + type: 'cartodb', + + options: { + sql: POINTS_SQL_PAIRS, + resolution: 1, + aggregation: { + threshold: 1 + }, + metadata: { + featureCount: true + } + } + } + ] + }; + if (placement !== 'default') { + this.mapConfig.layers[0].options.aggregation.placement = placement; + } + + this.testClient = new TestClient(this.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.ok(body.metadata.layers[0].meta.aggregation.mvt); + assert.equal(body.metadata.layers[0].meta.stats.featureCount, 13); + + done(); + }); + }); + }); }); }); diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index d8a9a06b..f74272d0 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -475,5 +475,4 @@ describe('Create mapnik layergroup', function() { }); }); - // TODO: add tests for metadata with aggregation }); From 3af118220644de57cefebf331987ae5c13f02ef0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 16 May 2018 14:45:19 +0200 Subject: [PATCH 24/38] Rename misleading function argument --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 6a71435e..50e3bc16 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -35,10 +35,10 @@ MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; -function queryPromise(dbConnection, query, callback) { +function queryPromise(dbConnection, query, setResults) { return new Promise(function(resolve, reject) { dbConnection.query(query, function (err, res) { - err = callback(err, res); + err = setResults(err, res); if (err) { reject(err); } From 012fa91e83407051aa19938dd8730392f1cc8967 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 16 May 2018 14:45:34 +0200 Subject: [PATCH 25/38] Typo --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 50e3bc16..f9e0000f 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -71,7 +71,7 @@ function columnAggregations(field) { function addStat(queries, ctx, type, zoom, query, assign, assignDefault=null) { let sql; if (type === 'pre') { - sql = ctx. preQuery; + sql = ctx.preQuery; } else { sql = queryForZoom(ctx.aggrQuery, zoom); From 4bc8fb207ab05395e780b0e6a4cae814b325922c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 18 May 2018 15:29:46 +0200 Subject: [PATCH 26/38] Use sql_raw for query without aggregation --- .../backends/layer-stats/mapnik-layer-stats.js | 7 ++----- .../adapter/aggregation-mapconfig-adapter.js | 17 ++++++++--------- .../mapconfig/adapter/turbo-carto-adapter.js | 4 +++- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index f9e0000f..365bb3a5 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -275,16 +275,13 @@ function formatResultFields(dbConnection, flds) { MapnikLayerStats.prototype.getStats = function (layer, dbConnection, callback) { - let aggrQuery = layer.options.sql_raw || layer.options.sql; - let preQuery = layer.options.aggregation_metadata ? - layer.options.aggregation_metadata.pre_aggregation_sql : - aggrQuery; + let aggrQuery = layer.options.sql; + let preQuery = layer.options.sql_raw || aggrQuery; let context = { dbConnection, preQuery, aggrQuery, - aggrMeta: layer.options.aggregation_metadata, metaOptions: layer.options.metadata || {} }; diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js index b3df3d96..9752ab57 100644 --- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js @@ -84,7 +84,7 @@ module.exports = class AggregationMapConfigAdapter { _adaptLayer (connection, mapConfig, layer, index) { return new Promise((resolve, reject) => { - this._shouldAdaptLayer(connection, mapConfig, layer, index, (err, shouldAdapt, aggrMeta) => { + this._shouldAdaptLayer(connection, mapConfig, layer, index, (err, shouldAdapt) => { if (err) { return reject(err); } @@ -93,7 +93,6 @@ module.exports = class AggregationMapConfigAdapter { return resolve({ layer, index, adapted: shouldAdapt }); } - const sqlQuery = layer.options.sql; const sqlQueryWrap = layer.options.sql_wrap; let aggregationSql = mapConfig.getAggregatedQuery(index); @@ -102,6 +101,12 @@ module.exports = class AggregationMapConfigAdapter { aggregationSql = sqlQueryWrap.replace(/<%=\s*sql\s*%>/g, aggregationSql); } + if (!layer.options.sql_raw) { + // if sql_wrap is present, the original query should already be + // in sql_raw (with sql being the wrapped query); + // otherwise we keep the now the original query in sql_raw + layer.options.sql_raw = layer.options.sql; + } layer.options.sql = aggregationSql; mapConfig.getLayerAggregationColumns(index, (err, columns) => { @@ -111,12 +116,6 @@ module.exports = class AggregationMapConfigAdapter { layer.options.columns = columns; - layer.options.aggregation_metadata = { - pre_aggregation_sql: sqlQueryWrap || sqlQuery, - geometry_type: aggrMeta.type, - pre_aggregation_count: aggrMeta.count - }; - return resolve({ layer, index, adapted: shouldAdapt }); }); }); @@ -161,7 +160,7 @@ module.exports = class AggregationMapConfigAdapter { return callback(null, false); } - callback(null, true, result); + callback(null, true); }); } diff --git a/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js b/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js index 2bd30152..31c4f7c4 100644 --- a/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js +++ b/lib/cartodb/models/mapconfig/adapter/turbo-carto-adapter.js @@ -117,7 +117,9 @@ TurboCartoAdapter.prototype._parseCartoCss = function (username, params, layer, var layerSql = layer.options.sql; var layerRawSql = layer.options.sql_raw; - if (SubstitutionTokens.hasTokens(layerSql) && layerRawSql) { + if (SubstitutionTokens.hasTokens(layerSql) && layerRawSql && layer.options.sql_wrap) { + // For wrapped queries we'll derive the tokens from the data extent + // instead of the whole Earth/root tile. var self = this; var tokensQuery = tokensQueryTpl({_sql: layerRawSql}); return pg.query(tokensQuery, function(err, resultSet) { From 391ac51f0f1d40d4fd58aafee1ab7ef2d452d57c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 18 May 2018 15:33:07 +0200 Subject: [PATCH 27/38] Implement metadata queries with plain Promises Remove usage of PhasedExecution This achives better query execution granularity and removes questionable usage of shared results object. It introduces a couple of behavior changes: * estimatedFeatureCount desn't ignore errors now * sample always uses estimatedFeatureCount,even if the actual count is also computed. --- .../layer-stats/mapnik-layer-stats.js | 251 +++++++++--------- lib/cartodb/utils/phased-execution.js | 94 ------- test/acceptance/aggregation.js | 4 +- 3 files changed, 129 insertions(+), 220 deletions(-) delete mode 100644 lib/cartodb/utils/phased-execution.js diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 365bb3a5..36f4824a 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -1,5 +1,4 @@ var queryUtils = require('../../utils/query-utils'); -const PhasedExecution = require('../../utils/phased-execution'); const AggregationMapConfig = require('../../models/aggregation/aggregation-mapconfig'); var SubstitutionTokens = require('../../utils/substitution-tokens'); @@ -35,15 +34,14 @@ MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; -function queryPromise(dbConnection, query, setResults) { +function queryPromise(dbConnection, query, adaptResults) { return new Promise(function(resolve, reject) { dbConnection.query(query, function (err, res) { - err = setResults(err, res); if (err) { reject(err); } else { - resolve(); + resolve(adaptResults(res)); } }); @@ -60,15 +58,7 @@ function columnAggregations(field) { return []; } -/* Helper to add a task to the queries PhasedExecution - * type can be either 'pre' (for pre-aggregation metadata) or 'post' - * zoom is used only for post-aggregation metadata - * query is a function that generates a metadata query from a data query - * assign is a function to assign the results of the metadata query - * if a assignDefault function is present, it will be used in case of error - * during the query execution and any errors will be ignored - */ -function addStat(queries, ctx, type, zoom, query, assign, assignDefault=null) { +function _getSQL(ctx, type, zoom, query) { let sql; if (type === 'pre') { sql = ctx.preQuery; @@ -76,157 +66,164 @@ function addStat(queries, ctx, type, zoom, query, assign, assignDefault=null) { else { sql = queryForZoom(ctx.aggrQuery, zoom); } - sql = query(sql); - queries.task( - queryPromise( - ctx.dbConnection, - sql, - (err, res) => { - if (!err) { - assign(res); - } - else if (assignDefault !== null) { - assignDefault(); - return null; - } - return err; - } - ) + return query(sql); +} + +function _estimatedFeatureCount(ctx) { + // TODO: restore -1 on errors behavior? + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, queryUtils.getQueryRowEstimation), + res => ({ estimatedFeatureCount: res.rows[0].rows }) ); } -function _estimatedFeatureCount(queries, ctx) { - if (queries.results.estimatedFeatureCount === undefined) { - // This is always computed; a default value of -1 is used in case of error - addStat( - queries, - ctx, - 'pre', 0, - queryUtils.getQueryRowEstimation, - res => queries.results.estimatedFeatureCount = res.rows[0].rows, - () => queries.results.estimatedFeatureCount = -1 - ); - } -} - -function _featureCount(queries, ctx) { +function _featureCount(ctx) { if (ctx.metaOptions.featureCount) { // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query - addStat( - queries, - ctx, - 'pre', 0, - queryUtils.getQueryActualRowCount, - res => queries.results.featureCount = res.rows[0].rows + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, queryUtils.getQueryActualRowCount), + res => ({ featureCount: res.rows[0].rows }) ); } + return Promise.resolve(); } -function _aggrFeatureCount(queries, ctx) { +function _aggrFeatureCount(ctx) { if (ctx.metaOptions.hasOwnProperty('aggrFeatureCount')) { // We expect as zoom level as the value of aggrFeatureCount // TODO: it'd be nice to admit an array of zoom levels to // return metadata for multiple levels. - addStat( - queries, - ctx, - 'post', ctx.metaOptions.aggrFeatureCount || 0, - queryUtils.getQueryActualRowCount, - res => queries.results.aggrfeatureCount = res.rows[0].rows + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'post', ctx.metaOptions.aggrFeatureCount || 0, queryUtils.getQueryActualRowCount), + res => ({ aggrFeatureCount: res.rows[0].rows }) ); } + return Promise.resolve(); } -function _geometryType(queries, ctx) { - if (ctx.metaOptions.geometryType && queries.results.geometryType === undefined) { +function _geometryType(ctx) { + if (ctx.metaOptions.geometryType) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); - addStat( - queries, - ctx, - 'pre', 0, - sql => queryUtils.getQueryGeometryType(sql, geometryColumn), - res => queries.results.geometryType = res.rows[0].geom_type + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryGeometryType(sql, geometryColumn)), + res => ({ geometryType: res.rows[0].geom_type }) ); } + return Promise.resolve(); } -function _columns(queries, ctx) { +function _columns(ctx) { if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { // note: post-aggregation columns are in layer.options.columns when aggregation is present - addStat( - queries, - ctx, - 'pre', 0, - sql => queryUtils.getQueryLimited(sql, 0), - res => queries.results.columns = formatResultFields(ctx.dbConnection, res.fields) + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryLimited(sql, 0)), + res => formatResultFields(ctx.dbConnection, res.fields) ); } + return Promise.resolve(); +} + +// combine a list of results merging the properties of all the objects +// undefined results are admitted and ignored +function mergeResults(results) { + if (results) { + if (results.length === 0) { + return {}; + } + return results.reduce((a, b) => { + if (a === undefined) { + return b; + } + if (b === undefined) { + return a; + } + return Object.assign({}, a, b); + }); + } } -function firstPhaseQueries(queries, ctx) { - _estimatedFeatureCount(queries, ctx); - _featureCount(queries, ctx); - _aggrFeatureCount(queries, ctx); - _geometryType(queries, ctx); - _columns(queries, ctx); +// deeper (1 level) combination of a list of objects: +// mergeColumns([{ col1: { a: 1 }, col2: { a: 2 } }, { col1: { b: 3 } }]) => { col1: { a: 1, b: 3 }, col2: { a: 2 } } +function mergeColumns(results) { + if (results) { + if (results.length === 0) { + return {}; + } + return results.reduce((a, b) => { + let c = Object.assign({}, b || {}, a || {}); + Object.keys(c).forEach(key => { + if (b.hasOwnProperty(key)) { + c[key] = Object.assign(c[key], b[key]); + } + }); + return c; + }); + } } -function _sample(queries, ctx) { + +function _sample(ctx, numRows) { if (ctx.metaOptions.sample) { - const numRows = queries.results.featureCount === undefined ? - queries.results.estimatedFeatureCount : - queries.results.featureCount; const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); - addStat( - queries, - ctx, - 'pre', 0, - sql => queryUtils.getQuerySample(sql, sampleProb), - res => queries.results.sample = res.rows + return queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, sql => queryUtils.getQuerySample(sql, sampleProb)), + res => ({ sample: res.rows }) ); } + return Promise.resolve(); } -function _columnStats(queries, ctx) { +function _columnStats(ctx, columns) { + if (!columns) { + return Promise.resolve(); + } if (ctx.metaOptions.columnStats) { + let queries = []; let aggr = []; - Object.keys(queries.results.columns).forEach(name => { + queries.push(new Promise(resolve => resolve(columns))); // add columns as first result + Object.keys(columns).forEach(name => { aggr = aggr.concat( - columnAggregations(queries.results.columns[name]) + columnAggregations(columns[name]) .map(fn => `${fn}(${name}) AS ${name}_${fn}`) ); - if (queries.results.columns[name].type === 'string') { + if (columns[name].type === 'string') { const topN = ctx.metaOptions.columnStats.topCategories || 1024; // TODO: ctx.metaOptions.columnStats.maxCategories // => use PG stats to dismiss columns with more distinct values - addStat( - queries, - ctx, - 'pre', 0, - sql => queryUtils.getQueryTopCategories(sql, name, topN), - res => queries.results.columns[name].categories = res.rows + queries.push( + queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryTopCategories(sql, name, topN)), + res => ({ [name]: { categories: res.rows } }) + ) ); } }); - addStat( - queries, - ctx, - 'pre', 0, - sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`, - res => { - Object.keys(queries.results.columns).forEach(name => { - columnAggregations(queries.results.columns[name]).forEach(fn => { - queries.results.columns[name][fn] = res.rows[0][`${name}_${fn}`]; + queries.push( + queryPromise( + ctx.dbConnection, + _getSQL(ctx, 'pre', 0, sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`), + res => { + let stats = {}; + Object.keys(columns).forEach(name => { + stats[name] = {}; + columnAggregations(columns[name]).forEach(fn => { + stats[name][fn] = res.rows[0][`${name}_${fn}`]; + }); }); - }); - } + return stats; + } + ) ); + return Promise.all(queries).then(results => ({ columns: mergeColumns(results) })); } -} - -function secondPhaseQueries(queries, ctx) { - _sample(queries, ctx); - _columnStats(queries, ctx); + return Promise.resolve({ columns }); } // This is adapted from SQL API: @@ -278,27 +275,33 @@ function (layer, dbConnection, callback) { let aggrQuery = layer.options.sql; let preQuery = layer.options.sql_raw || aggrQuery; - let context = { + let ctx = { dbConnection, preQuery, aggrQuery, metaOptions: layer.options.metadata || {} }; - let queries = new PhasedExecution(); - // TODO: could save some queries if queryUtils.getAggregationMetadata() has been used and kept somewhere // we would set queries.results.estimatedFeatureCount and queries.results.geometryType // (if metaOptions.geometryType) from it. - // Queries will be executed in two phases, with results from the first phase needed - // to define the queries of the second phase - queries.phase(() => firstPhaseQueries(queries, context)); - queries.phase(() => secondPhaseQueries(queries, context)); - queries.run() - .then(results => callback(null, results)) - .catch(error => callback(error)); + // TODO: compute _sample with _featureCount when available + Promise.all([ + _estimatedFeatureCount(ctx).then( + ({ estimatedFeatureCount }) => _sample(ctx, estimatedFeatureCount) + .then(s => mergeResults([s, { estimatedFeatureCount }])) + ), + _featureCount(ctx), + _aggrFeatureCount(ctx), + _geometryType(ctx), + _columns(ctx).then(columns => _columnStats(ctx, columns)) + ]).then(results => { + callback(null, mergeResults(results)); + }).catch(error => { + callback(error); + }); }; module.exports = MapnikLayerStats; diff --git a/lib/cartodb/utils/phased-execution.js b/lib/cartodb/utils/phased-execution.js deleted file mode 100644 index 46dfbb39..00000000 --- a/lib/cartodb/utils/phased-execution.js +++ /dev/null @@ -1,94 +0,0 @@ -/** - * PhasedExecution handles the execution of async tasks (via Promises) - * which have dependencies between them in a simplified manner. - * Instead of using the complete task dependency graph, tasks - * are organized into execution phases. So that tasks from a latter - * phase will be initialized after tasks from previous phases have - * finished. - * - * All tasks place their results in a shared object to make them - * available to tasks of latter phases. - * - * Each phase is defined by a function that defines its tasks. - * - * Example: - * - * let p = new PhasedExecution(); - * // Define first phase with tasks 1 & 2 - * p.phase(() => { - * console.log('At phase I', p.results);* - * p.results.phase1 = 1 - * p.task(new Promise((resolve) => { - * setTimeout( () => { - * console.log('At task 1:', p.results); - * p.results.task1 = 100; - * resolve(); - * }, 400); - * })); - * p.task(new Promise((resolve) => { - * setTimeout( () => { - * console.log('At task 2:', p.results); - * p.results.task2 = 200; - * resolve(); - * }, 100); - * })); - * }); - * // Define second phase with tasks 3 & 4 - * p.phase(() => { - * console.log('At phase II', p.results); - * p.results.phase2 = 2 - * p.task(new Promise((resolve) => { - * setTimeout( () => { - * console.log('At task 3:', p.results); - * p.results.task3 = 300; - * resolve(); - * }, 50); - * })); - * p.task(new Promise((resolve) => { - * setTimeout( () => { - * console.log('At task 4:', p.results); - * p.results.task4 = 400; - * resolve(); - * }, 100); - * })); - * }); - * // Define third phase with task 5 - * p.phase(() => { - * console.log('At phase III', p.results); - * p.results.phase3 = 3 - * p.task(new Promise((resolve) => { - * setTimeout( () => { - * console.log('At task 5:', p.results); - * p.results.task5 = 500; - * resolve(); - * }, 50); - * })); - * }); - * // Execute all tasks - * p.run().then((results) => { - * console.log("RESULTS:", results); - * }).catch((err) => { - * console.log("ERROR:", error); - * }); - */ -module.exports = class PhasedExecution { - constructor() { - this.results = {}; - this.phases = []; - } - phase(phasegenerator) { - this.phases.push(phasegenerator); - } - task(promise) { - this.tasks.push(promise); - } - run() { - this.tasks = []; - let phase = this.phases.shift(); - if (phase) { - phase(this); - return Promise.all(this.tasks).then(() => this.run()); - } - return this.results; - } -}; diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js index 6aab8d95..96401f15 100644 --- a/test/acceptance/aggregation.js +++ b/test/acceptance/aggregation.js @@ -2261,7 +2261,7 @@ describe('aggregation', function () { assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); assert.ok(body.metadata.layers[0].meta.aggregation.mvt); - assert.equal(body.metadata.layers[0].meta.stats.aggrfeatureCount, 13); + assert.equal(body.metadata.layers[0].meta.stats.aggrFeatureCount, 13); done(); }); @@ -2302,7 +2302,7 @@ describe('aggregation', function () { assert.equal(typeof body.metadata, 'object'); assert.ok(Array.isArray(body.metadata.layers)); assert.ok(body.metadata.layers[0].meta.aggregation.mvt); - assert.equal(body.metadata.layers[0].meta.stats.aggrfeatureCount, 9); + assert.equal(body.metadata.layers[0].meta.stats.aggrFeatureCount, 9); done(); }); From 4e99ff1c39c7389008e886a519d3938bb217b089 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 18 May 2018 22:25:32 +0200 Subject: [PATCH 28/38] Fix token substitution for stat queries --- .../layer-stats/mapnik-layer-stats.js | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 36f4824a..e3d11070 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -58,14 +58,15 @@ function columnAggregations(field) { return []; } -function _getSQL(ctx, type, zoom, query) { +function _getSQL(ctx, query, type='pre', zoom=0) { let sql; if (type === 'pre') { sql = ctx.preQuery; } else { - sql = queryForZoom(ctx.aggrQuery, zoom); + sql = ctx.aggrQuery; } + sql = queryForZoom(sql, zoom || 0); return query(sql); } @@ -73,7 +74,7 @@ function _estimatedFeatureCount(ctx) { // TODO: restore -1 on errors behavior? return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, queryUtils.getQueryRowEstimation), + _getSQL(ctx, queryUtils.getQueryRowEstimation), res => ({ estimatedFeatureCount: res.rows[0].rows }) ); } @@ -83,7 +84,7 @@ function _featureCount(ctx) { // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, queryUtils.getQueryActualRowCount), + _getSQL(ctx, queryUtils.getQueryActualRowCount), res => ({ featureCount: res.rows[0].rows }) ); } @@ -97,7 +98,7 @@ function _aggrFeatureCount(ctx) { // return metadata for multiple levels. return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'post', ctx.metaOptions.aggrFeatureCount || 0, queryUtils.getQueryActualRowCount), + _getSQL(ctx, queryUtils.getQueryActualRowCount, 'post', ctx.metaOptions.aggrFeatureCount), res => ({ aggrFeatureCount: res.rows[0].rows }) ); } @@ -109,7 +110,7 @@ function _geometryType(ctx) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryGeometryType(sql, geometryColumn)), + _getSQL(ctx, sql => queryUtils.getQueryGeometryType(sql, geometryColumn)), res => ({ geometryType: res.rows[0].geom_type }) ); } @@ -121,7 +122,7 @@ function _columns(ctx) { // note: post-aggregation columns are in layer.options.columns when aggregation is present return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryLimited(sql, 0)), + _getSQL(ctx, sql => queryUtils.getQueryLimited(sql, 0)), res => formatResultFields(ctx.dbConnection, res.fields) ); } @@ -172,7 +173,7 @@ function _sample(ctx, numRows) { const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); return queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, sql => queryUtils.getQuerySample(sql, sampleProb)), + _getSQL(ctx, sql => queryUtils.getQuerySample(sql, sampleProb)), res => ({ sample: res.rows }) ); } @@ -199,7 +200,7 @@ function _columnStats(ctx, columns) { queries.push( queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, sql => queryUtils.getQueryTopCategories(sql, name, topN)), + _getSQL(ctx, sql => queryUtils.getQueryTopCategories(sql, name, topN)), res => ({ [name]: { categories: res.rows } }) ) ); @@ -208,7 +209,7 @@ function _columnStats(ctx, columns) { queries.push( queryPromise( ctx.dbConnection, - _getSQL(ctx, 'pre', 0, sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`), + _getSQL(ctx, sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`), res => { let stats = {}; Object.keys(columns).forEach(name => { From 38e55367b1ec179da31b6b9582c3d9c3bb0f0bfc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 11:44:52 +0200 Subject: [PATCH 29/38] Revert error behaviour for estimatedFeatureCount Keep current production behavior of ignoreing errors when computing this stat and returning -1. This is done as to no introduce any instability in production at the moment. --- .../backends/layer-stats/mapnik-layer-stats.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index e3d11070..9ce411ed 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -34,11 +34,16 @@ MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; -function queryPromise(dbConnection, query, adaptResults) { +function queryPromise(dbConnection, query, adaptResults, errorHandler) { return new Promise(function(resolve, reject) { dbConnection.query(query, function (err, res) { if (err) { - reject(err); + if (errorHandler) { + errorHandler(err); + } + else { + reject(err); + } } else { resolve(adaptResults(res)); @@ -71,11 +76,11 @@ function _getSQL(ctx, query, type='pre', zoom=0) { } function _estimatedFeatureCount(ctx) { - // TODO: restore -1 on errors behavior? return queryPromise( ctx.dbConnection, _getSQL(ctx, queryUtils.getQueryRowEstimation), - res => ({ estimatedFeatureCount: res.rows[0].rows }) + res => ({ estimatedFeatureCount: res.rows[0].rows }), + () => ({ estimatedFeatureCount: -1 }) ); } From fecd63e5824545ccb53b1954fa65d2bfe4d0afa0 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 11:59:49 +0200 Subject: [PATCH 30/38] Fix bug --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 9ce411ed..689edb9b 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -39,7 +39,7 @@ function queryPromise(dbConnection, query, adaptResults, errorHandler) { dbConnection.query(query, function (err, res) { if (err) { if (errorHandler) { - errorHandler(err); + resolve(errorHandler(err)); } else { reject(err); From 11cdcc65ad0e570e57ede263d65d82c0dd062899 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 12:45:16 +0200 Subject: [PATCH 31/38] Add safety limit to sample metadata The sampling probability is now being computed using an estimate of the table row count This could led to too high probabilities (to large samples) if the estimate is not accurate. To avoid potential problems with large samples we've added a LIMIT to the sampling queries. --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 5 +++-- lib/cartodb/utils/query-utils.js | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 689edb9b..0390536b 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -172,13 +172,14 @@ function mergeColumns(results) { } } - function _sample(ctx, numRows) { if (ctx.metaOptions.sample) { const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); + // We'll use a safety limit just in case numRows is a bad estimate + const limit = Math.ceil(ctx.metaOptions.sample * 1.5); return queryPromise( ctx.dbConnection, - _getSQL(ctx, sql => queryUtils.getQuerySample(sql, sampleProb)), + _getSQL(ctx, sql => queryUtils.getQuerySample(sql, sampleProb, limit)), res => ({ sample: res.rows }) ); } diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 0bbd38c0..a1276f5e 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -88,11 +88,12 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull `; }; -module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { +module.exports.getQuerySample = function(query, sampleProb, limit = null, randomSeed = 0.5) { const singleTable = simpleQueryTable(query); if (singleTable) { return getTableSample(singleTable.table, singleTable.columns, sampleProb, randomSeed); } + const limitClause = limit ? `LIMIT ${limit}` : ''; return ` WITH __cdb_rndseed AS ( SELECT setseed(${randomSeed}) @@ -100,14 +101,16 @@ module.exports.getQuerySample = function(query, sampleProb, randomSeed = 0.5) { SELECT * FROM (${query}) AS __cdb_query WHERE random() < ${sampleProb} + ${limitClause} `; }; -function getTableSample(table, columns, sampleProb, randomSeed) { +function getTableSample(table, columns, sampleProb, limit = null, randomSeed = 0.5) { + const limitClause = limit ? `LIMIT ${limit}` : ''; sampleProb *= 100; randomSeed *= Math.pow(2, 31) -1; return ` - SELECT ${columns} FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) + SELECT ${columns} FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) ${limitClause} `; } From 32092d212ebe4777683066ae3db0fce09b9de1ea Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 14:05:27 +0200 Subject: [PATCH 32/38] Fix bug --- lib/cartodb/utils/query-utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index a1276f5e..22d99eef 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -91,7 +91,7 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull module.exports.getQuerySample = function(query, sampleProb, limit = null, randomSeed = 0.5) { const singleTable = simpleQueryTable(query); if (singleTable) { - return getTableSample(singleTable.table, singleTable.columns, sampleProb, randomSeed); + return getTableSample(singleTable.table, singleTable.columns, sampleProb, limit, randomSeed); } const limitClause = limit ? `LIMIT ${limit}` : ''; return ` From b233f18a0ff36810a8b0707011f108575d4c8541 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 15:43:23 +0200 Subject: [PATCH 33/38] Modernize code copied from SQL API --- .../backends/layer-stats/mapnik-layer-stats.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 0390536b..ad415c55 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -260,19 +260,18 @@ function fieldType(cname) { // columns are returned as an object { columnName1: { type1: ...}, ..} // for consistency with SQL API -function formatResultFields(dbConnection, flds) { - flds = flds || []; - var nfields = {}; - for (var i=0; i Date: Mon, 21 May 2018 15:43:32 +0200 Subject: [PATCH 34/38] Update NEWS --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index 141e082e..9df94a67 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,11 +12,13 @@ New features: - Fix a bug with zero length lines not being rendered when using the marker symbolizer. - Upgrades Camshaft to [0.61.9](https://github.com/CartoDB/camshaft/releases/tag/0.61.9): - Use Dollar-Quoted String Constants to avoid Syntax Error while running moran analyses. +- Optional instantiation metadata stats (https://github.com/CartoDB/Windshaft-cartodb/pull/952) Bug Fixes: - Validates tile coordinates (z/x/y) from request params to be a valid integer value. - Static maps fails for unsupported formats - Handling errors extracting the column type on dataviews +- Fix `meta.stats.estimatedFeatureCount` for aggregations and queries with tokens ## 6.1.0 Released 2018-04-16 From d7a90e6be42b7e2a1001c92204a61ff92c6b5855 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 15:54:52 +0200 Subject: [PATCH 35/38] Remove debugging comment --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index ad415c55..40590156 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -264,7 +264,6 @@ function formatResultFields(dbConnection, fields = []) { let nfields = {}; for (let field of fields) { const cname = dbConnection.typeName(field.dataTypeID); - console.log("FIELD",field); let tname; if ( ! cname ) { tname = 'unknown(' + field.dataTypeID + ')'; From d828a92ea33b74598db8a1a0f1c7abc71efd11d7 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 16:59:36 +0200 Subject: [PATCH 36/38] Use ifError to check for errors --- test/acceptance/stats/mapnik_stats_layergroup.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index f74272d0..1ec7371f 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -346,7 +346,7 @@ describe('Create mapnik layergroup', function() { } testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { From befedfd80a62b63020b7d927ffe51cfeded3c74b Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 17:03:16 +0200 Subject: [PATCH 37/38] Use ifError to check for errors --- .../stats/mapnik_stats_layergroup.js | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 1ec7371f..f6cad84e 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -90,7 +90,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 1); testClient.drain(done); @@ -107,7 +107,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 1); assert.equal(layergroup.metadata.layers[1].id, mapnikBasicLayerId(1)); @@ -127,7 +127,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 1); assert.equal(layergroup.metadata.layers[1].id, mapnikBasicLayerId(1)); @@ -147,7 +147,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); testClient.drain(done); @@ -164,7 +164,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[1].id, mapnikBasicLayerId(1)); @@ -183,7 +183,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 3); assert.ok(!layergroup.metadata.layers[0].meta.stats[1]); @@ -204,7 +204,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].type, 'mapnik'); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 1); @@ -224,7 +224,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function (err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, typeLayerId('http', 0)); assert.equal(layergroup.metadata.layers[0].type, 'http'); assert.ok(!layergroup.metadata.layers[0].meta.cartocss); @@ -245,7 +245,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); // we don't care about stats here as is an aliased column assert.ok(layergroup.metadata.layers[0].meta.stats.hasOwnProperty('estimatedFeatureCount')); @@ -265,7 +265,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, typeLayerId('http', 0)); assert.equal(layergroup.metadata.layers[0].type, 'http'); assert.equal(layergroup.metadata.layers[1].id, mapnikBasicLayerId(0)); @@ -295,7 +295,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); const expectedColumns = { @@ -404,7 +404,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[0].meta.stats.featureCount, 5); @@ -423,7 +423,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, 'ST_Point'); @@ -442,7 +442,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert(layergroup.metadata.layers[0].meta.stats.sample.length > 0); @@ -464,7 +464,7 @@ describe('Create mapnik layergroup', function() { }); testClient.getLayergroup(function(err, layergroup) { - assert.ok(!err); + assert.ifError(err); assert.equal(layergroup.metadata.layers[0].id, mapnikBasicLayerId(0)); assert.equal(layergroup.metadata.layers[0].meta.stats.estimatedFeatureCount, 5); assert.equal(layergroup.metadata.layers[0].meta.stats.geometryType, 'ST_Point'); From 6384f5538cbd500188fa8013bc15e1426371ee78 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 17:06:53 +0200 Subject: [PATCH 38/38] Rename variable for clarity --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 40590156..d1e292de 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -296,7 +296,7 @@ function (layer, dbConnection, callback) { Promise.all([ _estimatedFeatureCount(ctx).then( ({ estimatedFeatureCount }) => _sample(ctx, estimatedFeatureCount) - .then(s => mergeResults([s, { estimatedFeatureCount }])) + .then(sampleResults => mergeResults([sampleResults, { estimatedFeatureCount }])) ), _featureCount(ctx), _aggrFeatureCount(ctx),