Add optional layer metadata at instantiation

[jgoizueta]

This is experimental to be used by CartoVL. It shouldn't be public/documented at the moment.

Fixes #948

[jgoizueta]

Notes:

This allows obtaining additional metadata in map instantiation. When aggregation occurs, the metadata is about the original, unaggregated data source.
This is so to fullfill current Carto VL needs, but we can discuss about having aggregation metadata too in the future.

The metadata can be requested adding a metadata entry to the layer options, which can have this properties:

• featureCount: (true/false) actual row count (as opposed to the default estimated row count)
• geometryType (true/false) type of the geometry column
• columns: (true/false) name and types of functions
• columnStats: (true/false/object) same as columns and also some basic stats: max, min, avg, sum for numbers, max, min for dates, cand top categories for text columns. The number of top categories is 1024 by default and can be controlled by passing an object like { topCategories: 100 } here.
• sample (integer) returns a sample of the table of the approximate size (number of rows).

The requested metadata is returned in the response for each layer (metadata.layers) in meta.stats, in addition to estimatedFeatureCount which we already had.

I've preserved the existing behaviour for estimatedFeatureCount, which takes the value -1 if any SQL error occurs when computing it. I think this is questionable, as may be hiding unexpected problems. For the new metadata I'm not filtering SQL errors.

[jgoizueta]

I'm reviewing this because of a major problem: at the point where metadata is being computed now only the aggregated query (in case of aggregation) is available.
We could extract the original query since it appears as ( ... ) _cdb_query but a less hacky approach would be desirable.

Also the previously existing stat estimatedFeatureCount refers to the aggregated results in case of aggregation. So, to be consistent we should be explicit in the metadata parameters about pre/post aggregation metadata and support both in some cases (like count).

Tests for metadata in the presence of aggregation must be added.

[jgoizueta]

Well, regarding the existing stat estimatedFeatureCount it never really worked with aggregation because it executes the aggregation query without performing Mapnik token substitution (scale_denominator, etc.) This wasn't noticed because as stated in my previous comment we were silently returning -1 in case of error.

So, now we should be free to change the behaviour of this for aggregation and return the estimate count for the pre-aggregation query in this default stat.

[jgoizueta]

Hey @dgaubert can you review this again?

There was an interesting problem with the tests (well, "interesting" wasn't the word I had in mind while I was pulling my hair out trying to figure it out).

The test "layergroup can hold substitution tokens" was failing but only on travis. (not even using the docker tests locally). I finally was able to reproduce it locally by setting global.environment.enabledFeatures.layerStats = true; in the test.
So we have some test-order issue with some test leaving an indeterminate state in layerStats.

Now, the problem, which has existed for a long time, and has been reveled now, is this: we always compute the row count estimate stat. But this has been failing if the sql query contains Mapnik tokens (because we make no substitution before executing it).
But since we were ignoring any SQL errors for that query (setting row count to -1) we didn't notice.

I've fixed the substitution problem, but I haven't look at the layerStats configuration problem

[jgoizueta]

I've reverted the behaviour in case of error when computing estimatedFeatureCount (computed in each instantiation) to ignore errors and return -1, so we can safely deploy this in production without introducing new risks.

@oleurud do you think worth to make that conditional on the environment (so that in development, staging, etc the errors aren't ignored)? (if (process.env.NODE_ENV === 'production') ...)

[simon-contreras-deel]

Maybe an environment configuration parameter will be the best option (easy and fast to enable/disable)

[simon-contreras-deel]

Looks good, but let me check again this afternoon (i need to read some things)

[simon-contreras-deel]

It could be a else if?

[jgoizueta]

Yep, I think I've omitted lately quite a few elses because of returns inside conditions.
🤔 do you think the explicit else is preferable for clarity?

[simon-contreras-deel]

Forget if, you are right.

[simon-contreras-deel]

It could be prettier (no required, but it will be easy to understand):

function formatResultFields(dbConnection, fields = []) {
    let nfields = {};
    for (let field in fields) {
      const cname = dbConnection.typeName(field.dataTypeID);
      let tname;
      if ( ! cname ) {
        tname = 'unknown(' + field.dataTypeID + ')';
      } else {
        tname = fieldType(cname);
      }
      nfields[field.name] = { type: tname };
    }
    return nfields;
}

[jgoizueta]

You catched me copy-pasting from SQL API!! 😊

[simon-contreras-deel]

Looks good :)

[simon-contreras-deel]

A question to understand it: If some of _featureCount, _aggrFeatureCount, _geometryType or _columns fails, we will return an error as a response. I know that the user must request for it expressly, but I am not sure if metadata should do to fail a request. What is your point of view?

[simon-contreras-deel]

It is not a reason to stop the PR and also, I am not saying to change it. Only it raises doubts.

[dgaubert]

IMHO, If the user requests for any specific metadata and an error happens, we should return that error because we weren't able to process the request completely.

[simon-contreras-deel]

You have my blessings ;)

Thanks for a very nice issue (and the comments on the PR) allowing everyone to understand very well the use case, the solution, and the caveats

[dgaubert]

10 is the zoom value??

[jgoizueta]

Exactly

[dgaubert]

Use assert.ifError(). It gives a proper feedback of the actual error. However assert.ok() wraps the error behind the scenes.

[dgaubert]

Use assert.ifError()

[dgaubert]

Use assert.ifError()

[dgaubert]

Use assert.ifError()

[dgaubert]

Use assert.ifError()

[dgaubert]

Use assert.ifError()

[dgaubert]

🙏 rename s by sample

[dgaubert]

Isn't it a little bit of complication here?

We can simply return a promise:

function queryPromise(dbConnection, query) {
  return new Promise((resolve, reject) => { 
    dbConnection.query(query, (err, res) => err ? reject(err) : resolve(res))   
  })
}

And then, for each specific metadata function:

function _estimatedFeatureCount (ctx) {
  return queryPromise(ctx.dbConnection, _getSQL(ctx, queryUtils.getQueryRowEstimation))
    .then(res => ({ estimatedFeatureCount: res.rows[0].rows }))
    .catch(() => ({ estimatedFeatureCount: -1 }))
}

Note: Remember, .then and .catch return a promise as well.

[dgaubert]

Indeed, we can avoid the ctx object and pass only the arguments that each metadata function needs.

[jgoizueta]

I have fixed the other issues you mentioned (except the queryPromise refactor), as they were very simple.

Regarding queryPromise, I think you're absolutely right, but I don't want to delay the deploy today, so I'll open a separate PR to do it.

[dgaubert]

Fair enough!

Please, don't miss that refactor; you'll end up loving promises and their composability!

💋

[dgaubert]

I leave some comments that I would like you to take into account.