From 25e33955804cdbf0a815a777708795a7534bbeb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:09:24 +0200 Subject: [PATCH 01/12] Modify sample metadata --- .../layer-stats/mapnik-layer-stats.js | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index a8a181cc..d4d033ca 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -125,22 +125,28 @@ function mergeColumns(results) { } } -const SAMPLE_SEED = 0.5; const DEFAULT_SAMPLE_ROWS = 100; -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; - return queryUtils.queryPromise(ctx.dbConnection, _getSQL( - ctx, - sql => queryUtils.getQuerySample(sql, sampleProb, limit, SAMPLE_SEED, columns) - )).then(res => ({ sample: res.rows })); +function _sample(ctx) { + if (!ctx.metaOptions.sample) { + return Promise.resolve(); } - return Promise.resolve(); + + // 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); + const columns = ctx.metaOptions.sample.include_columns; + + const sqlMaxMin = _getSQL(ctx, sql => queryUtils.getMaxMinColumnQuery(sql)); + return queryUtils.queryPromise(ctx.dbConnection, sqlMaxMin) + .then(maxMinRes => { + const { min_id: min, id_span: span } = maxMinRes.rows[0]; + const ids = queryUtils.getSampleValuesFromRange(min, span, limit); + const sqlSample = _getSQL(ctx, sql => queryUtils.getSampleFromIdsQuery(sql, ids, columns)); + + return queryUtils.queryPromise(ctx.dbConnection, sqlSample); + }) + .then(res => ({ sample: res.rows })); } function _columnsMetadataRequired(options) { From 850bda9669601721e559695cac1313a4ce625383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:09:53 +0200 Subject: [PATCH 02/12] Use modified sample method --- 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 d4d033ca..dc49691a 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -300,8 +300,8 @@ function (layer, dbConnection, callback) { Promise.all([ _estimatedFeatureCount(ctx).then( - ({ estimatedFeatureCount }) => _sample(ctx, estimatedFeatureCount) - .then(sampleResults => mergeResults([sampleResults, { estimatedFeatureCount }])) + ({ estimatedFeatureCount }) => _sample(ctx) + .then(sampleResults => mergeResults([ sampleResults, { estimatedFeatureCount }] )) ), _featureCount(ctx), _aggrFeatureCount(ctx), From 2a8a8f6e6a7cf0e6a184c59a8bf5be693473ee69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:10:46 +0200 Subject: [PATCH 03/12] Export methods to get max, min, values od a column and sample based on a range --- lib/cartodb/utils/query-utils.js | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index f5daf0e1..63d66110 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -150,6 +150,36 @@ function simpleQueryTable(sql) { return false; } +module.exports.getMaxMinColumnQuery = function (query, column = 'cartodb_id') { + return ` + SELECT + min(${column}) AS min_id, + max(${column}) AS max_id, + (max(${column}) - min(${column})) AS id_span + FROM (${query}) _cdb_metadata_max_min; + `; +}; + +module.exports.getSampleValuesFromRange = function getSampleValuesFromRange (min, span, limit) { + const sample = new Set(); + + limit = limit < span ? limit : span; + + while (sample.size < limit) { + sample.add(Math.floor(min + Math.random() * span)); + } + + return Array.from(sample); +}; + +module.exports.getSampleFromIdsQuery = function (query, ids, columns, column = 'cartodb_id') { + return ` + SELECT + ${columnSelector(columns)} + FROM (${query}) _cdb_metadata_sample + WHERE ${column} IN (${ids.join(',')}) + `; +}; function getQueryLimited(query, limit = 0) { return ` From 780cb80c8cfb75681ff63a119c10f8f38b570966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:16:35 +0200 Subject: [PATCH 04/12] Move method --- .../backends/layer-stats/mapnik-layer-stats.js | 16 ++++++++++++++-- lib/cartodb/utils/query-utils.js | 12 ------------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index dc49691a..d8b01837 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -141,14 +141,26 @@ function _sample(ctx) { return queryUtils.queryPromise(ctx.dbConnection, sqlMaxMin) .then(maxMinRes => { const { min_id: min, id_span: span } = maxMinRes.rows[0]; - const ids = queryUtils.getSampleValuesFromRange(min, span, limit); - const sqlSample = _getSQL(ctx, sql => queryUtils.getSampleFromIdsQuery(sql, ids, columns)); + const values = _getSampleValuesFromRange(min, span, limit); + const sqlSample = _getSQL(ctx, sql => queryUtils.getSampleFromIdsQuery(sql, values, columns)); return queryUtils.queryPromise(ctx.dbConnection, sqlSample); }) .then(res => ({ sample: res.rows })); } +function _getSampleValuesFromRange (min, span, limit) { + const sample = new Set(); + + limit = limit < span ? limit : span; + + while (sample.size < limit) { + sample.add(Math.floor(min + Math.random() * span)); + } + + return Array.from(sample); +} + function _columnsMetadataRequired(options) { // We need determine the columns of a query // if either column stats or dimension stats are required, diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index 63d66110..b6e6d6bc 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -160,18 +160,6 @@ module.exports.getMaxMinColumnQuery = function (query, column = 'cartodb_id') { `; }; -module.exports.getSampleValuesFromRange = function getSampleValuesFromRange (min, span, limit) { - const sample = new Set(); - - limit = limit < span ? limit : span; - - while (sample.size < limit) { - sample.add(Math.floor(min + Math.random() * span)); - } - - return Array.from(sample); -}; - module.exports.getSampleFromIdsQuery = function (query, ids, columns, column = 'cartodb_id') { return ` SELECT From b2adb8f05831ea3659924851d77902047a047a6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:21:01 +0200 Subject: [PATCH 05/12] Use substituteDummyTokens --- lib/cartodb/utils/query-utils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index b6e6d6bc..a5bb4482 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -156,7 +156,7 @@ module.exports.getMaxMinColumnQuery = function (query, column = 'cartodb_id') { min(${column}) AS min_id, max(${column}) AS max_id, (max(${column}) - min(${column})) AS id_span - FROM (${query}) _cdb_metadata_max_min; + FROM (${substituteDummyTokens(query)}) _cdb_metadata_max_min; `; }; @@ -164,7 +164,7 @@ module.exports.getSampleFromIdsQuery = function (query, ids, columns, column = ' return ` SELECT ${columnSelector(columns)} - FROM (${query}) _cdb_metadata_sample + FROM (${substituteDummyTokens(query)}) _cdb_metadata_sample WHERE ${column} IN (${ids.join(',')}) `; }; From 16035131bc57ac4ab5db0abb75e2a5ddbe2ce22d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:25:37 +0200 Subject: [PATCH 06/12] Rename method --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 2 +- lib/cartodb/utils/query-utils.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index d8b01837..b55ead8c 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -137,7 +137,7 @@ function _sample(ctx) { const limit = Math.ceil(requestedRows * 1.5); const columns = ctx.metaOptions.sample.include_columns; - const sqlMaxMin = _getSQL(ctx, sql => queryUtils.getMaxMinColumnQuery(sql)); + const sqlMaxMin = _getSQL(ctx, sql => queryUtils.getMaxMinSpanColumnQuery(sql)); return queryUtils.queryPromise(ctx.dbConnection, sqlMaxMin) .then(maxMinRes => { const { min_id: min, id_span: span } = maxMinRes.rows[0]; diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index a5bb4482..e288cb32 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -150,13 +150,13 @@ function simpleQueryTable(sql) { return false; } -module.exports.getMaxMinColumnQuery = function (query, column = 'cartodb_id') { +module.exports.getMaxMinSpanColumnQuery = function (query, column = 'cartodb_id') { return ` SELECT min(${column}) AS min_id, max(${column}) AS max_id, (max(${column}) - min(${column})) AS id_span - FROM (${substituteDummyTokens(query)}) _cdb_metadata_max_min; + FROM (${substituteDummyTokens(query)}) _cdb_metadata_max_min_span; `; }; From d4d981909b9002e2556a5e50f6b688af9cc82d0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 17:43:20 +0200 Subject: [PATCH 07/12] Remove unused code --- lib/cartodb/utils/query-utils.js | 52 -------------------------------- 1 file changed, 52 deletions(-) diff --git a/lib/cartodb/utils/query-utils.js b/lib/cartodb/utils/query-utils.js index e288cb32..8ef03c34 100644 --- a/lib/cartodb/utils/query-utils.js +++ b/lib/cartodb/utils/query-utils.js @@ -98,58 +98,6 @@ function columnSelector(columns) { throw new TypeError(`Bad argument type for columns: ${typeof columns}`); } -module.exports.getQuerySample = function (query, sampleProb, limit = null, randomSeed = 0.5, columns = null) { - const singleTable = simpleQueryTable(query); - if (singleTable) { - return getTableSample(singleTable.table, columns || singleTable.columns, sampleProb, limit, randomSeed); - } - const limitClause = limit ? `LIMIT ${limit}` : ''; - return ` - WITH __cdb_rndseed AS ( - SELECT setseed(${randomSeed}) - ) - SELECT ${columnSelector(columns)} - FROM (${substituteDummyTokens(query)}) AS __cdb_query - WHERE random() < ${sampleProb} - ${limitClause} - `; -}; - -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 ${columnSelector(columns)} - FROM ${table} - TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) ${limitClause} - `; -} - -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) { - const columns = match[1]; - const schema = match[3]; - const table = match[4]; - return { table: schema ? `${schema}.${table}` : table, columns }; - } - return false; -} - module.exports.getMaxMinSpanColumnQuery = function (query, column = 'cartodb_id') { return ` SELECT From 9a85b661b0353d49021f9ac4ddc979ceaccc62f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Fri, 23 Aug 2019 18:04:19 +0200 Subject: [PATCH 08/12] Remove safe limit --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index b55ead8c..7212eccb 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -132,9 +132,7 @@ function _sample(ctx) { return Promise.resolve(); } - // 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); + const limit = ctx.metaOptions.sample.num_rows || DEFAULT_SAMPLE_ROWS; const columns = ctx.metaOptions.sample.include_columns; const sqlMaxMin = _getSQL(ctx, sql => queryUtils.getMaxMinSpanColumnQuery(sql)); From 5a01c1c5eb4305240d6064bf1ce5905be5317ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 2 Sep 2019 13:38:28 +0200 Subject: [PATCH 09/12] Going red: test to avoid error when the source table is empty --- .../stats/mapnik_stats_layergroup.js | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index dde4f059..76d8770b 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -583,6 +583,38 @@ describe(`[${desc}] Create mapnik layergroup`, function() { }); }); + it('should not provide a sample when the source table is empty', function (done) { + var testClient = new TestClient({ + "version": "1.4.0", + "layers": [ + { + "type": "mapnik", + "options": { + "sql": "SELECT * FROM test_table_100 limit 0", + "cartocss_version": "2.3.0", + "cartocss": "#layer { line-width:16; }", + "metadata": { + "sample": { + "num_rows": 30 + }, + "sql": "select * from test_table_100 limit 0" + } + } + } + ] + }); + + 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, 100); + assert(layergroup.metadata.layers[0].meta.stats.sample.length > 0); + const expectedCols = [ 'cartodb_id', 'value', 'the_geom', 'the_geom_webmercator' ].sort(); + assert.deepEqual(Object.keys(layergroup.metadata.layers[0].meta.stats.sample[0]).sort(), expectedCols); + testClient.drain(done); + }); + }); + it('can specify sample columns', function(done) { var testClient = new TestClient({ version: '1.4.0', From 2fbd9893bd4b8fc97b1383317adc750b69a791d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 2 Sep 2019 14:09:13 +0200 Subject: [PATCH 10/12] Going green: do not fail when source is empty --- lib/cartodb/backends/layer-stats/mapnik-layer-stats.js | 5 +++++ test/acceptance/stats/mapnik_stats_layergroup.js | 9 ++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js index 7212eccb..3d229799 100644 --- a/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js +++ b/lib/cartodb/backends/layer-stats/mapnik-layer-stats.js @@ -139,6 +139,11 @@ function _sample(ctx) { return queryUtils.queryPromise(ctx.dbConnection, sqlMaxMin) .then(maxMinRes => { const { min_id: min, id_span: span } = maxMinRes.rows[0]; + + if (!min || !span) { + return { rows: {} }; + } + const values = _getSampleValuesFromRange(min, span, limit); const sqlSample = _getSQL(ctx, sql => queryUtils.getSampleFromIdsQuery(sql, values, columns)); diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 76d8770b..1f8ca067 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -596,8 +596,7 @@ describe(`[${desc}] Create mapnik layergroup`, function() { "metadata": { "sample": { "num_rows": 30 - }, - "sql": "select * from test_table_100 limit 0" + } } } } @@ -606,11 +605,7 @@ describe(`[${desc}] Create mapnik layergroup`, function() { 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, 100); - assert(layergroup.metadata.layers[0].meta.stats.sample.length > 0); - const expectedCols = [ 'cartodb_id', 'value', 'the_geom', 'the_geom_webmercator' ].sort(); - assert.deepEqual(Object.keys(layergroup.metadata.layers[0].meta.stats.sample[0]).sort(), expectedCols); + assert.deepStrictEqual(layergroup.metadata.layers[0].meta.stats.sample, {}); testClient.drain(done); }); }); From 7bf5deb4c1304ba33501cb9e99e6658a8a7c0153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Mon, 2 Sep 2019 16:40:43 +0200 Subject: [PATCH 11/12] Styling --- .../stats/mapnik_stats_layergroup.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/acceptance/stats/mapnik_stats_layergroup.js b/test/acceptance/stats/mapnik_stats_layergroup.js index 1f8ca067..55ca7a64 100644 --- a/test/acceptance/stats/mapnik_stats_layergroup.js +++ b/test/acceptance/stats/mapnik_stats_layergroup.js @@ -585,17 +585,17 @@ describe(`[${desc}] Create mapnik layergroup`, function() { it('should not provide a sample when the source table is empty', function (done) { var testClient = new TestClient({ - "version": "1.4.0", - "layers": [ + version: "1.4.0", + layers: [ { - "type": "mapnik", - "options": { - "sql": "SELECT * FROM test_table_100 limit 0", - "cartocss_version": "2.3.0", - "cartocss": "#layer { line-width:16; }", - "metadata": { - "sample": { - "num_rows": 30 + type: "mapnik", + options: { + sql: "SELECT * FROM test_table_100 limit 0", + cartocss_version: "2.3.0", + cartocss: "#layer { line-width:16; }", + metadata: { + sample: { + num_rows: 30 } } } From 8f6447b67e7bfd0bfd94b801dff7e7acb48f775b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Sep 2019 12:30:12 +0200 Subject: [PATCH 12/12] Update NEWS --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 2190f597..ea9f57c0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,7 @@ Released 2019-XX-XX Announcements: +- Improved efficiency of query samples while instatiating a map (#1120). - Cache control header fine tuning. Set a shorter value for "max-age" directive if there is no way to know when to trigger the invalidation. - Update deps: - Update `cartodb-query-tables` to version [`0.5.0`](https://github.com/CartoDB/node-cartodb-query-tables/releases/tag/0.5.0)