From 28b76d9c3cf30a376f70edc12ffb26a1bfb90a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 24 Aug 2016 12:43:06 +0200 Subject: [PATCH 1/3] Fixes #127, validates analysis for invalid inputs, uses st_collect when nedded. --- lib/node/nodes/centroid.js | 75 ++++++++++--- test/acceptance/centroid.js | 217 ++++++++++++++++++++++++++++++++++++ 2 files changed, 277 insertions(+), 15 deletions(-) create mode 100644 test/acceptance/centroid.js diff --git a/lib/node/nodes/centroid.js b/lib/node/nodes/centroid.js index 20167a80..070f3b98 100644 --- a/lib/node/nodes/centroid.js +++ b/lib/node/nodes/centroid.js @@ -1,17 +1,32 @@ 'use strict'; var Node = require('../node'); +var debug = require('../../util/debug')('analysis:centroid'); var TYPE = 'centroid'; var PARAMS = { source : Node.PARAM.NODE(Node.GEOMETRY.ANY), category_column : Node.PARAM.NULLABLE(Node.PARAM.STRING()), - aggregation: Node.PARAM.NULLABLE(Node.PARAM.STRING(), 'count'), - aggregation_column: Node.PARAM.NULLABLE(Node.PARAM.STRING()) + aggregation: Node.PARAM.NULLABLE(Node.PARAM.ENUM('avg', 'count', 'max', 'min', 'sum'), null), + aggregation_column: Node.PARAM.NULLABLE(Node.PARAM.STRING()), }; -var Centroid = Node.create(TYPE, PARAMS); +var Centroid = Node.create(TYPE, PARAMS, { version: 1, + beforeCreate: function(node) { + debug(node); + + if (node.aggregation && node.aggregation !== 'count' && node.aggregation_column === null) { + throw new Error('Param `aggregation` != "count" requires an existent `aggregation_column` column'); + } + if (node.aggregation === null && node.aggregation_column !== null) { + throw new Error('Param `aggregation_column` requires an `aggregation` operation'); + } + if (node.aggregation === 'count' && node.aggregation_column === null) { + node.aggregation_column = '*'; + } + } +}); module.exports = Centroid; module.exports.TYPE = TYPE; @@ -20,22 +35,52 @@ module.exports.PARAMS = PARAMS; var centroidTemplate = Node.template([ 'SELECT', ' row_number() over() as cartodb_id,', - ' ST_Centroid(ST_Collect(the_geom)) as the_geom,', - ' {{? it.categoryColumn }}{{=it.categoryColumn}} as category,{{?}}', - ' {{=it.aggregation}} as value', + ' {{=it.columns}}', 'FROM ({{=it.query}}) q', - '{{? it.categoryColumn }}GROUP BY {{=it.categoryColumn}}{{?}}' + '{{? it.categoryColumn }} GROUP BY {{=it.categoryColumn}}{{?}}' ].join('\n')); -var aggregationFnQueryTpl = Node.template('{{=it._aggregationFn}}({{=it._aggregationColumn}})'); +var centroidColumnTemplate = Node.template([ + 'ST_Centroid({{? it.aggregation || it.categoryColumn }}ST_Collect(the_geom){{??}}the_geom{{?}}) as the_geom' +]); -Centroid.prototype.sql = function(){ - return centroidTemplate({ - query: this.source.getQuery(), - categoryColumn: this.category_column, - aggregation: aggregationFnQueryTpl({ - _aggregationFn: this.aggregation, - _aggregationColumn: this.aggregation_column || 1 +var categoryColumnTemplate = Node.template([ + '{{=it.categoryColumn}} as category' +]); + +var aggregationColumnTemplate = Node.template([ + '{{=it.aggregation}}({{=it.aggregationColumn}}) as value', +]); + +Centroid.prototype.sql = function() { + var columns = [ + centroidColumnTemplate({ + aggregation: this.aggregation, + aggregationColumn: this.aggregation_column, + categoryColumn: this.category_column }) + ]; + + if (this.category_column) { + columns.push(categoryColumnTemplate({ + categoryColumn: this.category_column + })); + } + + if (this.aggregation && this.aggregation_column) { + columns.push(aggregationColumnTemplate({ + aggregation: this.aggregation, + aggregationColumn: this.aggregation_column + })); + } + + var sql = centroidTemplate({ + query: this.source.getQuery(), + columns: columns.join(', '), + categoryColumn: this.category_column }); + + debug(sql); + + return sql; }; diff --git a/test/acceptance/centroid.js b/test/acceptance/centroid.js new file mode 100644 index 00000000..4f1acd4a --- /dev/null +++ b/test/acceptance/centroid.js @@ -0,0 +1,217 @@ +'use strict'; + +var assert = require('assert'); + +var Analysis = require('../../lib/analysis'); + +var testConfig = require('../test-config'); +var QueryRunner = require('../../lib/postgresql/query-runner'); + +describe('centroid analysis', function() { + + var queryRunner; + + before(function() { + queryRunner = new QueryRunner(testConfig.db); + }); + + var QUERY = 'select * from atm_machines'; + var CATEGORY_COLUMN = 'kind'; + var AGGREGATION = 'avg'; + var AGGREGATION_COLUMN = 'cartodb_id'; + + var sourceAtmMachines = { + type: 'source', + params: { + query: QUERY + } + }; + + var config = testConfig.create({ + batch: { + inlineExecution: true + } + }); + + function performAnalysis(definition, callback) { + Analysis.create(config, definition, function (err, analysis) { + if (err) { + return callback(err); + } + + queryRunner.run(analysis.getQuery(), function(err, result) { + if (err) { + return callback(err); + } + + assert.ok(Array.isArray(result.rows)); + var values = result.rows.map(function (value) { + return value; + }); + + callback(null, values); + }); + }); + } + + describe('non optional params', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + } + }; + + it('should create an analysis', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with category column', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + category_column: CATEGORY_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with category column and aggregation (miss aggregation_column)', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + category_column: CATEGORY_COLUMN, + aggregation: AGGREGATION + } + }; + + it('should return error', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + assert.ok(err); + assert.ok(!values); + done(); + }); + }); + }); + + describe('with category column, aggregation and aggregation column', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + category_column: CATEGORY_COLUMN, + aggregation: AGGREGATION, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + + describe('with aggregation (miss aggregation_colum for avg method)', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + aggregation: AGGREGATION + } + }; + + it('should return error', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + assert.ok(err); + assert.ok(!values); + done(); + }); + }); + }); + + + describe('with aggregation (miss aggregation_colum for avg method)', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + aggregation: 'count' + } + }; + + it('should create an analysis', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with aggregation and aggregation column', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + aggregation: AGGREGATION, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + + describe('with category_column and aggregation column', function () { + var centroidDefinition = { + type: 'centroid', + params: { + source: sourceAtmMachines, + category_column: CATEGORY_COLUMN, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should return error', function (done) { + performAnalysis(centroidDefinition, function (err, values) { + assert.ok(err); + assert.ok(!values); + done(); + }); + }); + }); +}); From 37fb2d8f15ccefaadd3f2b0ccbafb15b4b3f1288 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 24 Aug 2016 16:58:49 +0200 Subject: [PATCH 2/3] Uses debug in weighted-centroid and implemented acceptance test --- lib/node/nodes/weighted-centroid.js | 7 +- test/acceptance/weighted-centroid.js | 232 ++++++++++++++++++ .../cdb_crankshaft/cdb_crankshaft.sql | 43 +++- test/setup.js | 2 + 4 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 test/acceptance/weighted-centroid.js diff --git a/lib/node/nodes/weighted-centroid.js b/lib/node/nodes/weighted-centroid.js index 129a2b52..f944b572 100644 --- a/lib/node/nodes/weighted-centroid.js +++ b/lib/node/nodes/weighted-centroid.js @@ -1,6 +1,7 @@ 'use strict'; var Node = require('../node'); +var debug = require('../../util/debug')('analysis:weighted-centroid'); var TYPE = 'weighted-centroid'; @@ -31,7 +32,7 @@ var weightedCentroidTemplate = Node.template([ var aggregationFnQueryTpl = Node.template('{{=it._aggregationFn}}({{=it._aggregationColumn}})'); WeightedCentroid.prototype.sql = function(){ - return weightedCentroidTemplate({ + var sql = weightedCentroidTemplate({ query: this.source.getQuery(), weightColumn: this.weight_column, categoryColumn: this.category_column, @@ -40,4 +41,8 @@ WeightedCentroid.prototype.sql = function(){ _aggregationColumn: this.aggregation_column || 1 }) }); + + debug(sql); + + return sql; }; diff --git a/test/acceptance/weighted-centroid.js b/test/acceptance/weighted-centroid.js new file mode 100644 index 00000000..b381f118 --- /dev/null +++ b/test/acceptance/weighted-centroid.js @@ -0,0 +1,232 @@ +'use strict'; + +var assert = require('assert'); + +var Analysis = require('../../lib/analysis'); + +var testConfig = require('../test-config'); +var QueryRunner = require('../../lib/postgresql/query-runner'); + +describe('weighted centroid analysis', function() { + + var queryRunner; + + before(function() { + queryRunner = new QueryRunner(testConfig.db); + }); + + var QUERY = 'select * from atm_machines'; + var CATEGORY_COLUMN = 'bank'; + var AGGREGATION = 'avg'; + var AGGREGATION_COLUMN = 'cartodb_id'; + var WEIGHT_COLUMN = 'kind'; + + var sourceAtmMachines = { + type: 'source', + params: { + query: QUERY + } + }; + + var config = testConfig.create({ + batch: { + inlineExecution: true + } + }); + + function performAnalysis(definition, callback) { + Analysis.create(config, definition, function (err, analysis) { + if (err) { + return callback(err); + } + + queryRunner.run(analysis.getQuery(), function(err, result) { + if (err) { + return callback(err); + } + + assert.ok(Array.isArray(result.rows)); + var values = result.rows.map(function (value) { + return value; + }); + + callback(null, values); + }); + }); + } + + describe('non optional params', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with category column', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + category_column: CATEGORY_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with category column and aggregation (miss aggregation_column)', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + category_column: CATEGORY_COLUMN, + aggregation: AGGREGATION + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with category column, aggregation and aggregation column', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + category_column: CATEGORY_COLUMN, + aggregation: AGGREGATION, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + + describe('with aggregation (miss aggregation_colum for avg method)', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + aggregation: AGGREGATION, + weight_column: WEIGHT_COLUMN + } + }; + + it('should return error', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + + describe('with aggregation (miss aggregation_colum for avg method)', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + aggregation: 'count' + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + describe('with aggregation and aggregation column', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + aggregation: AGGREGATION, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should create an analysis', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); + + + describe('with category_column and aggregation column', function () { + var weightedCentroidDefinition = { + type: 'weighted-centroid', + params: { + source: sourceAtmMachines, + weight_column: WEIGHT_COLUMN, + category_column: CATEGORY_COLUMN, + aggregation_column: AGGREGATION_COLUMN + } + }; + + it('should return error', function (done) { + performAnalysis(weightedCentroidDefinition, function (err, values) { + if(err) { + return done(err); + } + assert.ok(values); + done(); + }); + }); + }); +}); diff --git a/test/fixtures/cdb_crankshaft/cdb_crankshaft.sql b/test/fixtures/cdb_crankshaft/cdb_crankshaft.sql index c1f2b8fb..f2d9b346 100644 --- a/test/fixtures/cdb_crankshaft/cdb_crankshaft.sql +++ b/test/fixtures/cdb_crankshaft/cdb_crankshaft.sql @@ -1,4 +1,4 @@ -create schema cdb_crankshaft; +create schema if not exists cdb_crankshaft; CREATE TYPE kmeans_type as (cartodb_id numeric, cluster_no numeric); @@ -12,3 +12,44 @@ CREATE OR REPLACE FUNCTION cdb_crankshaft.CDB_KMeans(query text, no_clusters int RETURN; END; $$ LANGUAGE plpgsql; + +CREATE OR REPLACE FUNCTION cdb_crankshaft.CDB_WeightedMeanS(state Numeric[],the_geom GEOMETRY(Point, 4326), weight NUMERIC) + RETURNS Numeric[] AS + $$ +DECLARE + newX NUMERIC; + newY NUMERIC; + newW NUMERIC; +BEGIN + IF weight IS NULL OR the_geom IS NULL THEN + newX = state[1]; + newY = state[2]; + newW = state[3]; + ELSE + newX = state[1] + ST_X(the_geom)*weight; + newY = state[2] + ST_Y(the_geom)*weight; + newW = state[3] + weight; + END IF; + RETURN Array[newX,newY,newW]; + +END +$$ LANGUAGE plpgsql; + +CREATE OR REPLACE FUNCTION cdb_crankshaft.CDB_WeightedMeanF(state Numeric[]) + RETURNS GEOMETRY AS + $$ +BEGIN + IF state[3] = 0 THEN + RETURN ST_SetSRID(ST_MakePoint(state[1],state[2]), 4326); + ELSE + RETURN ST_SETSRID(ST_MakePoint(state[1]/state[3], state[2]/state[3]),4326); + END IF; +END +$$ LANGUAGE plpgsql; + +CREATE AGGREGATE cdb_crankshaft.CDB_WeightedMean(geometry(Point, 4326), NUMERIC)( +SFUNC = cdb_crankshaft.CDB_WeightedMeanS, +FINALFUNC = cdb_crankshaft.CDB_WeightedMeanF, +STYPE = Numeric[], +INITCOND = "{0.0,0.0,0.0}" +); diff --git a/test/setup.js b/test/setup.js index 93f7d1c1..b255e39d 100644 --- a/test/setup.js +++ b/test/setup.js @@ -25,6 +25,8 @@ before(function setupTestDatabase(done) { fs.realpathSync('./test/fixtures/cdb_dataservices_client/cdb_route_with_waypoints.sql'), fs.realpathSync('./test/fixtures/cdb_dataservices_client/obs_getmeasure.sql'), + fs.realpathSync('./test/fixtures/cdb_crankshaft/cdb_crankshaft.sql'), + fs.realpathSync('./test/fixtures/table/madrid_districts.sql'), fs.realpathSync('./test/fixtures/table/atm_machines.sql'), fs.realpathSync('./test/fixtures/table/airbnb_rooms.sql') From c720f81d2bd7ea5f6f38ab13457c55bf27ba0eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 7 Sep 2016 19:54:36 +0200 Subject: [PATCH 3/3] Fix default value for `aggreation`, set to `count` by default --- lib/node/nodes/centroid.js | 20 +++++--------------- test/acceptance/centroid.js | 26 ++++++++++++++------------ 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/lib/node/nodes/centroid.js b/lib/node/nodes/centroid.js index 070f3b98..bbf050d8 100644 --- a/lib/node/nodes/centroid.js +++ b/lib/node/nodes/centroid.js @@ -8,23 +8,15 @@ var TYPE = 'centroid'; var PARAMS = { source : Node.PARAM.NODE(Node.GEOMETRY.ANY), category_column : Node.PARAM.NULLABLE(Node.PARAM.STRING()), - aggregation: Node.PARAM.NULLABLE(Node.PARAM.ENUM('avg', 'count', 'max', 'min', 'sum'), null), + aggregation: Node.PARAM.NULLABLE(Node.PARAM.ENUM('avg', 'count', 'max', 'min', 'sum'), 'count'), aggregation_column: Node.PARAM.NULLABLE(Node.PARAM.STRING()), }; var Centroid = Node.create(TYPE, PARAMS, { version: 1, beforeCreate: function(node) { - debug(node); - - if (node.aggregation && node.aggregation !== 'count' && node.aggregation_column === null) { + if (node.aggregation !== 'count' && node.aggregation_column === null) { throw new Error('Param `aggregation` != "count" requires an existent `aggregation_column` column'); } - if (node.aggregation === null && node.aggregation_column !== null) { - throw new Error('Param `aggregation_column` requires an `aggregation` operation'); - } - if (node.aggregation === 'count' && node.aggregation_column === null) { - node.aggregation_column = '*'; - } } }); @@ -41,7 +33,7 @@ var centroidTemplate = Node.template([ ].join('\n')); var centroidColumnTemplate = Node.template([ - 'ST_Centroid({{? it.aggregation || it.categoryColumn }}ST_Collect(the_geom){{??}}the_geom{{?}}) as the_geom' + 'ST_Centroid({{? it.categoryColumn }}ST_Collect(the_geom){{??}}the_geom{{?}}) as the_geom' ]); var categoryColumnTemplate = Node.template([ @@ -55,8 +47,6 @@ var aggregationColumnTemplate = Node.template([ Centroid.prototype.sql = function() { var columns = [ centroidColumnTemplate({ - aggregation: this.aggregation, - aggregationColumn: this.aggregation_column, categoryColumn: this.category_column }) ]; @@ -67,10 +57,10 @@ Centroid.prototype.sql = function() { })); } - if (this.aggregation && this.aggregation_column) { + if (this.aggregation && this.category_column) { columns.push(aggregationColumnTemplate({ aggregation: this.aggregation, - aggregationColumn: this.aggregation_column + aggregationColumn: this.aggregation_column || 1 })); } diff --git a/test/acceptance/centroid.js b/test/acceptance/centroid.js index 4f1acd4a..b2bc0019 100644 --- a/test/acceptance/centroid.js +++ b/test/acceptance/centroid.js @@ -54,7 +54,7 @@ describe('centroid analysis', function() { }); } - describe('non optional params', function () { + describe('w/o optional params', function () { var centroidDefinition = { type: 'centroid', params: { @@ -73,7 +73,7 @@ describe('centroid analysis', function() { }); }); - describe('with category column', function () { + describe('with `category_column`', function () { var centroidDefinition = { type: 'centroid', params: { @@ -93,7 +93,7 @@ describe('centroid analysis', function() { }); }); - describe('with category column and aggregation (miss aggregation_column)', function () { + describe('with `category_column` and `aggregation` (miss `aggregation_column`)', function () { var centroidDefinition = { type: 'centroid', params: { @@ -112,7 +112,7 @@ describe('centroid analysis', function() { }); }); - describe('with category column, aggregation and aggregation column', function () { + describe('with `category_column`, `aggregation` and `aggregation_column`', function () { var centroidDefinition = { type: 'centroid', params: { @@ -135,7 +135,7 @@ describe('centroid analysis', function() { }); - describe('with aggregation (miss aggregation_colum for avg method)', function () { + describe('with `aggregation` (miss `aggregation_colum` for `avg` method)', function () { var centroidDefinition = { type: 'centroid', params: { @@ -154,7 +154,7 @@ describe('centroid analysis', function() { }); - describe('with aggregation (miss aggregation_colum for avg method)', function () { + describe('with `aggregation` (miss `aggregation_colum` for `count` method)', function () { var centroidDefinition = { type: 'centroid', params: { @@ -163,7 +163,7 @@ describe('centroid analysis', function() { } }; - it('should create an analysis', function (done) { + it('should create an analysis `aggregation`', function (done) { performAnalysis(centroidDefinition, function (err, values) { if(err) { return done(err); @@ -174,7 +174,7 @@ describe('centroid analysis', function() { }); }); - describe('with aggregation and aggregation column', function () { + describe('with `aggregation` and `aggregation_column`', function () { var centroidDefinition = { type: 'centroid', params: { @@ -196,7 +196,7 @@ describe('centroid analysis', function() { }); - describe('with category_column and aggregation column', function () { + describe('with `category_column` and `aggregation_column`', function () { var centroidDefinition = { type: 'centroid', params: { @@ -206,10 +206,12 @@ describe('centroid analysis', function() { } }; - it('should return error', function (done) { + it('should create an analysis', function (done) { performAnalysis(centroidDefinition, function (err, values) { - assert.ok(err); - assert.ok(!values); + if(err) { + return done(err); + } + assert.ok(values); done(); }); });