From ebc086106f651f545f8d19fddda4452519b0b424 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 21 May 2018 17:33:03 +0200 Subject: [PATCH 1/7] Refactor metadata queryPromise helper This function to adapt query execution as a Promise was unnecessary complex. --- .../layer-stats/mapnik-layer-stats.js | 89 ++++++------------- 1 file changed, 29 insertions(+), 60 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index d1e292de..69336147 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -34,24 +34,11 @@ MapnikLayerStats.prototype.is = function (type) { return this._types[type] ? this._types[type] : false; }; -function queryPromise(dbConnection, query, adaptResults, errorHandler) { - return new Promise(function(resolve, reject) { - dbConnection.query(query, function (err, res) { - if (err) { - if (errorHandler) { - resolve(errorHandler(err)); - } - else { - reject(err); - } - } - else { - resolve(adaptResults(res)); - } - }); - +function queryPromise(dbConnection, query) { + return new Promise((resolve, reject) => { + dbConnection.query(query, (err, res) => err ? reject(err) : resolve(res)); }); -} + } function columnAggregations(field) { if (field.type === 'number') { @@ -76,22 +63,16 @@ function _getSQL(ctx, query, type='pre', zoom=0) { } function _estimatedFeatureCount(ctx) { - return queryPromise( - ctx.dbConnection, - _getSQL(ctx, queryUtils.getQueryRowEstimation), - res => ({ estimatedFeatureCount: res.rows[0].rows }), - () => ({ estimatedFeatureCount: -1 }) - ); + return queryPromise(ctx.dbConnection, _getSQL(ctx, queryUtils.getQueryRowEstimation)) + .then(res => ({ estimatedFeatureCount: res.rows[0].rows })) + .catch(() => ({ estimatedFeatureCount: -1 })); } function _featureCount(ctx) { if (ctx.metaOptions.featureCount) { // TODO: if ctx.metaOptions.columnStats we can combine this with column stats query - return queryPromise( - ctx.dbConnection, - _getSQL(ctx, queryUtils.getQueryActualRowCount), - res => ({ featureCount: res.rows[0].rows }) - ); + return queryPromise(ctx.dbConnection, _getSQL(ctx, queryUtils.getQueryActualRowCount)) + .then(res => ({ featureCount: res.rows[0].rows })); } return Promise.resolve(); } @@ -103,9 +84,8 @@ function _aggrFeatureCount(ctx) { // return metadata for multiple levels. return queryPromise( ctx.dbConnection, - _getSQL(ctx, queryUtils.getQueryActualRowCount, 'post', ctx.metaOptions.aggrFeatureCount), - res => ({ aggrFeatureCount: res.rows[0].rows }) - ); + _getSQL(ctx, queryUtils.getQueryActualRowCount, 'post', ctx.metaOptions.aggrFeatureCount) + ).then(res => ({ aggrFeatureCount: res.rows[0].rows })); } return Promise.resolve(); } @@ -113,11 +93,8 @@ function _aggrFeatureCount(ctx) { function _geometryType(ctx) { if (ctx.metaOptions.geometryType) { const geometryColumn = AggregationMapConfig.getAggregationGeometryColumn(); - return queryPromise( - ctx.dbConnection, - _getSQL(ctx, sql => queryUtils.getQueryGeometryType(sql, geometryColumn)), - res => ({ geometryType: res.rows[0].geom_type }) - ); + return queryPromise(ctx.dbConnection, _getSQL(ctx, sql => queryUtils.getQueryGeometryType(sql, geometryColumn))) + .then(res => ({ geometryType: res.rows[0].geom_type })); } return Promise.resolve(); } @@ -125,11 +102,8 @@ function _geometryType(ctx) { function _columns(ctx) { if (ctx.metaOptions.columns || ctx.metaOptions.columnStats) { // note: post-aggregation columns are in layer.options.columns when aggregation is present - return queryPromise( - ctx.dbConnection, - _getSQL(ctx, sql => queryUtils.getQueryLimited(sql, 0)), - res => formatResultFields(ctx.dbConnection, res.fields) - ); + return queryPromise(ctx.dbConnection, _getSQL(ctx, sql => queryUtils.getQueryLimited(sql, 0))) + .then(res => formatResultFields(ctx.dbConnection, res.fields)); } return Promise.resolve(); } @@ -177,11 +151,8 @@ function _sample(ctx, numRows) { 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, limit)), - res => ({ sample: res.rows }) - ); + return queryPromise(ctx.dbConnection, _getSQL(ctx, sql => queryUtils.getQuerySample(sql, sampleProb, limit))) + .then(res => ({ sample: res.rows })); } return Promise.resolve(); } @@ -206,27 +177,25 @@ function _columnStats(ctx, columns) { queries.push( queryPromise( ctx.dbConnection, - _getSQL(ctx, sql => queryUtils.getQueryTopCategories(sql, name, topN)), - res => ({ [name]: { categories: res.rows } }) - ) + _getSQL(ctx, sql => queryUtils.getQueryTopCategories(sql, name, topN)) + ).then(res => ({ [name]: { categories: res.rows } })) ); } }); queries.push( queryPromise( ctx.dbConnection, - _getSQL(ctx, 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}`]; - }); + _getSQL(ctx, sql => `SELECT ${aggr.join(',')} FROM (${sql}) AS __cdb_query`) + ).then(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 stats; + }) ); return Promise.all(queries).then(results => ({ columns: mergeColumns(results) })); } From 4bb8914d9a02488ddf37bbc726eff74ed30ef4fd Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 28 May 2018 16:08:31 +0200 Subject: [PATCH 2/7] Add parameters to select metadata sample columns --- .../layer-stats/mapnik-layer-stats.js | 35 ++++++++++++++++--- lib/cartodb/utils/query-utils.js | 20 ++++++++--- 2 files changed, 46 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 3a7a3358..0caf2d0e 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -146,13 +146,36 @@ function mergeColumns(results) { } } -function _sample(ctx, numRows) { +const SAMPLE_SEED = 0.5; +const DEFAULT_SAMPLE_ROWS = 100; + +function exclude(items, excludedItems) { + if (excludedItems) { + return items.filter(item => !excludedItems.includes(item)); + } + return items; +} + +function _sample(ctx, numRows, availableColumns = null) { if (ctx.metaOptions.sample) { - const sampleProb = Math.min(ctx.metaOptions.sample / numRows, 1); + const sampleProb = Math.min(ctx.metaOptions.sample.num_rows / 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, limit))) - .then(res => ({ sample: res.rows })); + const requestedRows = ctx.metaOptions.sample.num_rows || DEFAULT_SAMPLE_ROWS; + const limit = Math.ceil(requestedRows * 1.5); + let columns = ctx.metaOptions.sample.include_columns; + if (columns) { + columns = exclude(columns, ctx.metaOptions.sample.exclude_columns); + } + else if (ctx.metaOptions.sample.exclude_columns) { + if (availableColumns === null) { + return Promise.reject(new Error('Column stats are needed to use the sample exclude columns options')); + } + columns = exclude(availableColumns, ctx.metaOptions.sample.exclude_columns); + } + return queryPromise(ctx.dbConnection, _getSQL( + ctx, + sql => queryUtils.getQuerySample(sql, sampleProb, limit, SAMPLE_SEED, columns) + )).then(res => ({ sample: res.rows })); } return Promise.resolve(); } @@ -265,6 +288,8 @@ function (layer, dbConnection, callback) { // (if metaOptions.geometryType) from it. // TODO: compute _sample with _featureCount when available + // TODO: add support for sample.exclude option by, in that case, forcing the columns query and + // passing the results to the sample query function. Promise.all([ _estimatedFeatureCount(ctx).then( diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 22d99eef..ae1a2e2d 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -88,17 +88,27 @@ module.exports.getQueryTopCategories = function(query, column, topN, includeNull `; }; -module.exports.getQuerySample = function(query, sampleProb, limit = null, randomSeed = 0.5) { +function columnSelector(columns) { + if (!columns) { + return '*'; + } + if (typeof(columns) === 'string') { + return columns; + } + return columns.map(name => `"${name}"`).join(', '); +} + +module.exports.getQuerySample = function(query, sampleProb, limit = null, randomSeed = 0.5, columns = null) { const singleTable = simpleQueryTable(query); if (singleTable) { - return getTableSample(singleTable.table, singleTable.columns, sampleProb, limit, randomSeed); + return getTableSample(singleTable.table, columns || singleTable.columns, sampleProb, limit, randomSeed); } const limitClause = limit ? `LIMIT ${limit}` : ''; return ` WITH __cdb_rndseed AS ( SELECT setseed(${randomSeed}) ) - SELECT * + SELECT ${columnSelector(columns)} FROM (${query}) AS __cdb_query WHERE random() < ${sampleProb} ${limitClause} @@ -110,7 +120,9 @@ function getTableSample(table, columns, sampleProb, limit = null, randomSeed = 0 sampleProb *= 100; randomSeed *= Math.pow(2, 31) -1; return ` - SELECT ${columns} FROM ${table} TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) ${limitClause} + SELECT ${columnSelector(columns)} + FROM ${table} + TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) ${limitClause} `; } From 34a2f3b32bde01c847683fa3c8faea7874487680 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 28 May 2018 16:50:53 +0200 Subject: [PATCH 3/7] Tests were missing in previous commit --- .../stats/mapnik_stats_layergroup.js | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 46c4368a..4e8413ae 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -513,7 +513,7 @@ describe('Create mapnik layergroup', function() { version: '1.4.0', layers: [ layerWithMetadata(mapnikLayer4, { - sample: 3 + sample: { num_rows: 3 } }) ] }); @@ -529,6 +529,31 @@ describe('Create mapnik layergroup', function() { }); }); + it('can specify sample columns', function(done) { + var testClient = new TestClient({ + version: '1.4.0', + layers: [ + layerWithMetadata(mapnikLayer4, { + sample: { + num_rows: 3, + include_columns: [ 'cartodb_id', 'address', 'the_geom' ] + } + }) + ] + }); + + testClient.getLayergroup(function(err, layergroup) { + 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); + const expectedCols = [ 'cartodb_id', 'address', 'the_geom' ].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', From 8a1d5d3a489cc26e94a4c181f99c64f33adea303 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 28 May 2018 17:36:58 +0200 Subject: [PATCH 4/7] Be careful and detect type invalid types --- lib/cartodb/utils/query-utils.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index ae1a2e2d..867db0cd 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -92,10 +92,13 @@ function columnSelector(columns) { if (!columns) { return '*'; } - if (typeof(columns) === 'string') { + if (typeof columns === 'string') { return columns; } - return columns.map(name => `"${name}"`).join(', '); + if (Array.isArray(columns)) { + return columns.map(name => `"${name}"`).join(', '); + } + throw new TypeError(`Bad argument type for columns: ${typeof columns}`); } module.exports.getQuerySample = function(query, sampleProb, limit = null, randomSeed = 0.5, columns = null) { From 26da872704afdbb89c82d8f241c76a3921b5ee12 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Mon, 28 May 2018 17:37:45 +0200 Subject: [PATCH 5/7] Leave sample exclude_columns for later --- .../backends/layer-stats/mapnik-layer-stats.js | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 0caf2d0e..d8320334 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -149,29 +149,13 @@ function mergeColumns(results) { const SAMPLE_SEED = 0.5; const DEFAULT_SAMPLE_ROWS = 100; -function exclude(items, excludedItems) { - if (excludedItems) { - return items.filter(item => !excludedItems.includes(item)); - } - return items; -} - -function _sample(ctx, numRows, availableColumns = null) { +function _sample(ctx, numRows) { if (ctx.metaOptions.sample) { const sampleProb = Math.min(ctx.metaOptions.sample.num_rows / numRows, 1); // We'll use a safety limit just in case numRows is a bad estimate const requestedRows = ctx.metaOptions.sample.num_rows || DEFAULT_SAMPLE_ROWS; const limit = Math.ceil(requestedRows * 1.5); let columns = ctx.metaOptions.sample.include_columns; - if (columns) { - columns = exclude(columns, ctx.metaOptions.sample.exclude_columns); - } - else if (ctx.metaOptions.sample.exclude_columns) { - if (availableColumns === null) { - return Promise.reject(new Error('Column stats are needed to use the sample exclude columns options')); - } - columns = exclude(availableColumns, ctx.metaOptions.sample.exclude_columns); - } return queryPromise(ctx.dbConnection, _getSQL( ctx, sql => queryUtils.getQuerySample(sql, sampleProb, limit, SAMPLE_SEED, columns) From 794c3efb7d7d9663f7ab583db41b46a3d6ab772e Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Mon, 28 May 2018 12:52:03 +0200 Subject: [PATCH 6/7] Update Windshaft, camshaft and cartodb-psql to use cartodb-psql 0.11.0 --- NEWS.md | 5 +++-- package.json | 6 +++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 02c66777..fb2d8ea2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,16 +5,17 @@ Released 2018-mm-dd New features: - CI tests with Ubuntu Xenial + PostgreSQL 10.1 and Ubuntu Precise + PostgreSQL 9.5 -- Upgrades Windshaft to [4.8.0](https://github.com/CartoDB/Windshaft/blob/4.8.0/NEWS.md#version-480) which includes: +- Upgrades Windshaft to [4.8.1](https://github.com/CartoDB/Windshaft/blob/4.8.1/NEWS.md#version-481) which includes: - Update internal deps. - A fix in mapnik-vector-tile to avoid grouping together properties with the same value but a different type. - Performance improvements in the marker symbolizer (local cache, avoid building the collision matrix when possible). - MVT: Disable simplify_distance to avoid multiple simplifications. - 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): +- Upgrades Camshaft to [0.61.10](https://github.com/CartoDB/camshaft/releases/tag/0.61.10): - Use Dollar-Quoted String Constants to avoid Syntax Error while running moran analyses. - Update other deps: - body-parser: 1.18.3 + - cartodb-psql: 0.11.0 - dot: 1.1.2 - express: 4.16.3 - lru-cache: 4.1.3 diff --git a/package.json b/package.json index 40d75a38..488b59c3 100644 --- a/package.json +++ b/package.json @@ -26,8 +26,8 @@ "dependencies": { "basic-auth": "2.0.0", "body-parser": "1.18.3", - "camshaft": "0.61.9", - "cartodb-psql": "0.10.2", + "camshaft": "0.61.10", + "cartodb-psql": "0.11.0", "cartodb-query-tables": "0.3.0", "cartodb-redis": "1.0.0", "debug": "3.1.0", @@ -48,7 +48,7 @@ "step-profiler": "0.3.0", "turbo-carto": "0.20.2", "underscore": "1.6.0", - "windshaft": "4.8.0", + "windshaft": "4.8.1", "yargs": "11.1.0" }, "devDependencies": { From 9c3b3e698e97abe54f22a96b3dfeab517e912d9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 29 May 2018 16:28:23 +0200 Subject: [PATCH 7/7] Update yarn.lock --- yarn.lock | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/yarn.lock b/yarn.lock index deb13479..4f98787a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -261,13 +261,13 @@ camelcase@^4.1.0: version "4.1.0" resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-4.1.0.tgz#d545635be1e33c542649c69173e5de6acfae34dd" -camshaft@0.61.9: - version "0.61.9" - resolved "https://registry.yarnpkg.com/camshaft/-/camshaft-0.61.9.tgz#f3d399dfacf51b6a492c579e925c8b1141b22974" +camshaft@0.61.10: + version "0.61.10" + resolved "https://registry.yarnpkg.com/camshaft/-/camshaft-0.61.10.tgz#9055bbc577dbd87d38a0e712bf6157afd7704dca" dependencies: async "^1.5.2" bunyan "1.8.1" - cartodb-psql "^0.10.1" + cartodb-psql "0.11.0" debug "^3.1.0" dot "^1.0.3" request "2.85.0" @@ -312,12 +312,12 @@ cartocolor@4.0.0: dependencies: colorbrewer "1.0.0" -cartodb-psql@0.10.2, cartodb-psql@^0.10.1: - version "0.10.2" - resolved "https://registry.yarnpkg.com/cartodb-psql/-/cartodb-psql-0.10.2.tgz#8c505066e4a635cfa0ee4c603769c83f6e2187dd" +cartodb-psql@0.11.0: + version "0.11.0" + resolved "https://registry.yarnpkg.com/cartodb-psql/-/cartodb-psql-0.11.0.tgz#6b4eae0876ee56944a61fe5f4acc6a8b0b11233f" dependencies: debug "^3.1.0" - pg cartodb/node-postgres#6.1.6-cdb1 + pg CartoDB/node-postgres#6.4.2-cdb1 underscore "~1.6.0" cartodb-query-tables@0.3.0: @@ -1804,9 +1804,9 @@ p-try@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/p-try/-/p-try-1.0.0.tgz#cbc79cdbaf8fd4228e13f621f2b1a237c1b207b3" -packet-reader@0.2.0: - version "0.2.0" - resolved "https://registry.yarnpkg.com/packet-reader/-/packet-reader-0.2.0.tgz#819df4d010b82d5ea5671f8a1a3acf039bcd7700" +packet-reader@0.3.1: + version "0.3.1" + resolved "https://registry.yarnpkg.com/packet-reader/-/packet-reader-0.3.1.tgz#cd62e60af8d7fea8a705ec4ff990871c46871f27" parse-json@^2.2.0: version "2.2.0" @@ -1885,20 +1885,20 @@ pg-types@1.*: postgres-date "~1.0.0" postgres-interval "^1.1.0" -pg@cartodb/node-postgres#6.1.6-cdb1: - version "6.1.6" - resolved "https://codeload.github.com/cartodb/node-postgres/tar.gz/3eef52dd1e655f658a4ee8ac5697688b3ecfed44" +"pg@github:CartoDB/node-postgres#6.4.2-cdb1": + version "6.4.2" + resolved "https://codeload.github.com/CartoDB/node-postgres/tar.gz/449fac1d6da711ffcc6694ae3c89f85244f48bdc" dependencies: buffer-writer "1.0.1" js-string-escape "1.0.1" - packet-reader "0.2.0" + packet-reader "0.3.1" pg-connection-string "0.1.3" pg-pool "1.*" pg-types "1.*" - pgpass "1.x" + pgpass "1.*" semver "4.3.2" -pgpass@1.x: +pgpass@1.*: version "1.0.2" resolved "https://registry.yarnpkg.com/pgpass/-/pgpass-1.0.2.tgz#2a7bb41b6065b67907e91da1b07c1847c877b306" dependencies: @@ -2802,16 +2802,16 @@ window-size@^0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/window-size/-/window-size-0.2.0.tgz#b4315bb4214a3d7058ebeee892e13fa24d98b075" -windshaft@4.8.0: - version "4.8.0" - resolved "https://registry.yarnpkg.com/windshaft/-/windshaft-4.8.0.tgz#79ea03ee78fa68288e07c7aa01a9fdabe3cc5af0" +windshaft@4.8.1: + version "4.8.1" + resolved "https://registry.yarnpkg.com/windshaft/-/windshaft-4.8.1.tgz#7c1ef21e4f885643f6347bc5871274c533fce206" dependencies: "@carto/mapnik" "3.6.2-carto.10" "@carto/tilelive-bridge" cartodb/tilelive-bridge#2.5.1-cdb9 abaculus cartodb/abaculus#2.0.3-cdb10 canvas cartodb/node-canvas#1.6.2-cdb2 carto cartodb/carto#0.15.1-cdb3 - cartodb-psql "0.10.2" + cartodb-psql "0.11.0" debug "3.1.0" dot "1.1.2" grainstore "1.9.0"