Merge pull request #1120 from CartoDB/improve-metadata-sample

Improve efficiency of query samples (esp. for FDW's)

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)

lib/cartodb/backends/layer-stats/mapnik-layer-stats.js

@@ -125,22 +125,43 @@ 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();
+
+    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));
+    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));
+
+            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) {
@@ -294,8 +315,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),

lib/cartodb/utils/query-utils.js

@@ -98,58 +98,24 @@ 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}` : '';
+module.exports.getMaxMinSpanColumnQuery = function (query, column = 'cartodb_id') {
     return `
-        WITH __cdb_rndseed AS (
-            SELECT setseed(${randomSeed})
-        )
-        SELECT ${columnSelector(columns)}
-            FROM (${substituteDummyTokens(query)}) AS __cdb_query
-            WHERE random() < ${sampleProb}
-            ${limitClause}
+        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_span;
     `;
 };
 
-function getTableSample(table, columns, sampleProb, limit = null, randomSeed = 0.5) {
-    const limitClause = limit ? `LIMIT ${limit}` : '';
-    sampleProb *= 100;
-    randomSeed *= Math.pow(2, 31) - 1;
+module.exports.getSampleFromIdsQuery = function (query, ids, columns, column = 'cartodb_id') {
     return `
-        SELECT ${columnSelector(columns)}
-        FROM ${table}
-        TABLESAMPLE BERNOULLI (${sampleProb}) REPEATABLE (${randomSeed}) ${limitClause}
+        SELECT
+            ${columnSelector(columns)}
+        FROM (${substituteDummyTokens(query)}) _cdb_metadata_sample
+        WHERE ${column} IN (${ids.join(',')})
     `;
-}
-
-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;
-}
-
+};
 
 function getQueryLimited(query, limit = 0) {
     return `

test/acceptance/stats/mapnik_stats_layergroup.js

@@ -583,6 +583,33 @@ 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
+                            }
+                        }
+                    }
+                }
+            ]
+        });
+
+        testClient.getLayergroup(function(err, layergroup) {
+            assert.ifError(err);
+            assert.deepStrictEqual(layergroup.metadata.layers[0].meta.stats.sample, {});
+            testClient.drain(done);
+        });
+    });
+
     it('can specify sample columns', function(done) {
         var testClient = new TestClient({
             version: '1.4.0',