search

CartoDB / Windshaft-cartodb

Windshaft tailored for CARTO
BSD 3-Clause "New" or "Revised" License
72 stars 58 forks source link

Bad aggregation method for overview dataview #1137

Closed dgaubert closed 5 years ago

dgaubert commented 5 years ago

From logs:

[2019-11-07 13:50:44.067] [FATAL] [default] - TypeError: aggregationFnQueryTpl[this.aggregation] is not a function
    at Aggregation.getAggregationSql (/home/ubuntu/www/node-windshaft/releases/20191029111604/lib/models/dataview/overviews/aggregation.js:223:51)
    at Aggregation.sql (/home/ubuntu/www/node-windshaft/releases/20191029111604/lib/models/dataview/overviews/aggregation.js:191:40)
    at /home/ubuntu/www/node-windshaft/releases/20191029111604/lib/models/dataview/overviews/aggregation.js:127:18
    at psql.query (/home/ubuntu/www/node-windshaft/releases/20191029111604/lib/models/dataview/base.js:75:13)
    at queryArray (/home/ubuntu/www/node-windshaft/releases/20191029111604/node_modules/cartodb-psql/lib/psql.js:359:24)
    at Result.client.query (/home/ubuntu/www/node-windshaft/releases/20191029111604/node_modules/cartodb-psql/lib/psql.js:321:24)
    at Result.Query.handleReadyForQuery (/home/ubuntu/www/node-windshaft/releases/20191029111604/node_modules/pg/lib/query.js:144:10)
    at Connection.<anonymous> (/home/ubuntu/www/node-windshaft/releases/20191029111604/node_modules/pg/lib/client.js:184:19)
    at Connection.emit (events.js:189:13)
    at Socket.<anonymous> (/home/ubuntu/www/node-windshaft/releases/20191029111604/node_modules/pg/lib/connection.js:135:12)

We should validate the aggregation method.

dgaubert commented 5 years ago

Pease @CartoDB/rt-managers, give to this issue max priority to this bug as Tilers are crashing because of this.

ztephm commented 5 years ago

@alrocar @esloho labeling this critical per @dgaubert comment above

esloho commented 5 years ago

@dgaubert the function that should validate the aggregation is getAggregationSql:

var aggregationFnQueryTpl = {
    count: dot.template('sum(_feature_count)'),
    sum:   dot.template('sum({{=it._aggregationColumn}}*_feature_count)')
};

Aggregation.prototype.getAggregationSql = function() {
    return aggregationFnQueryTpl[this.aggregation]({
        _aggregationFn: this.aggregation,
        _aggregationColumn: this.aggregationColumn || 1
    });
};

what should be the expected return of this function when this.aggregation is missing? Null? empty object? Error?

dgaubert commented 5 years ago

We should validate this. aggregation is either count or sum, otherwise, we should throw an error in the same way we do for regular aggregations, see: https://github.com/CartoDB/Windshaft-cartodb/blob/master/lib/models/dataview/aggregation.js#L197.

Please add some tests here to validate the new behaviour.

HTH