-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mechanism to check analysis before it's executed #215
Changes from all commits
17925d1
ba97e6e
023a5d5
a6aae8d
74b1fad
6fc38a4
f38729e
9808d6c
a2e4520
f91a33d
86961cf
b5d2bbd
880fa16
3b40450
700a116
fa211d9
e36669e
6d6d71c
3d26bfe
e2e6e59
baf428c
722de54
1309a75
27782e6
4d97ee2
e88b88e
aa12fef
e931566
b247f61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
|
||
'use strict'; | ||
|
||
var util = require('util'); | ||
|
@@ -9,6 +10,7 @@ dot.templateSettings.strip = false; | |
var id = require('../util/id'); | ||
var QueryBuilder = require('../filter/query-builder'); | ||
var Validator = require('./validator'); | ||
var NodeRequirements = require('./requirements'); | ||
|
||
var TYPE = require('./type'); | ||
module.exports.TYPE = TYPE; | ||
|
@@ -28,7 +30,6 @@ var STATUS = { | |
|
||
module.exports.STATUS = STATUS; | ||
|
||
|
||
var NODE_RESERVED_KEYWORDS = { | ||
type: 1, | ||
typeSignature: 1, | ||
|
@@ -90,6 +91,8 @@ function Node() { | |
this.typeSignature = null; | ||
|
||
this.version = 0; | ||
|
||
this.requirements = new NodeRequirements(this); | ||
} | ||
|
||
Node.prototype.id = function(skipFilters) { | ||
|
@@ -468,3 +471,44 @@ function validate(validator, params, expectedParamName) { | |
|
||
return param; | ||
} | ||
|
||
Node.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
var default_limit = null; // no limit by default | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we use something like |
||
|
||
// The default comptueRequirements methos computes the number estimated of output rows of the node. | ||
// Two possible approaches to do this are: | ||
// 1. use the node's query, e.g. | ||
// this.requirements.setEstimatedNumberOfRowsFromQuery(databaseService, default_limit, limits, callback); | ||
// (we could also use setNumberOfRowsFromQuery for the exact number of rows) | ||
// 2. reduce overhead by basing the estimation on the previously computed estimations of the input nodes; | ||
// here we would use some simple, common heuristic, such as: | ||
// this.requirements.setMaxInputNumberOfRows(limits, default_limit); | ||
// return callback(null); | ||
// And we'd need to override this for node classes for which this not acceptable | ||
|
||
this.requirements.setMaxInputNumberOfRows(limits, default_limit); | ||
return callback(null); | ||
}; | ||
|
||
Node.prototype.requirementMessages = function() { | ||
var messages = []; | ||
|
||
// Note that any falsy value (null, 0, '', ...) means no limits: | ||
if (this.requirements.getLimit('maximumNumberOfRows')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would hide these details in |
||
if (this.requirements.getEstimatedRequirement('numberOfRows') > | ||
this.requirements.getLimit('maximumNumberOfRows')) { | ||
messages.push('too many result rows'); | ||
} | ||
} | ||
|
||
return messages; | ||
}; | ||
|
||
Node.prototype.validateRequirements = function() { | ||
var messages = this.requirementMessages(); | ||
var err = null; | ||
if (messages.length > 0) { | ||
err = new Error(messages.join('\n')); | ||
} | ||
return err; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,3 +77,9 @@ var queryAggregateTemplate = Node.template([ | |
'WHERE ST_Intersects(_cdb_analysis_source.the_geom, _cdb_analysis_target.the_geom)', | ||
'GROUP BY {{=it.groupByColumns}}' | ||
].join('\n')); | ||
|
||
AggregateIntersection.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
// we estimate the maximum possible number of rows of the result | ||
this.requirements.setProductInputNumberOfRows(limits, 1000000); | ||
return callback(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, not use callbacks with synchronous code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case we do need the callback parameter, so we have a common interface for all nodes, because some node classes need to perform asynchronous operations in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, but be careful with CPU intensive tasks and consider use |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,3 +56,8 @@ var queryTemplate = Node.template([ | |
' ) AS the_geom', | ||
'FROM ({{=it.source}}) AS _camshaft_georeference_admin_region_analysis' | ||
].join('\n')); | ||
|
||
GeoreferenceAdminRegion.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
this.requirements.setSingleInputNumberOfRows('source', limits, 1000); | ||
return callback(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No callback is necessary here |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,3 +33,10 @@ var queryTemplate = Node.template([ | |
' cdb_dataservices_client.cdb_geocode_admin0_polygon({{=it.country}}) AS the_geom', | ||
'FROM ({{=it.source}}) AS _camshaft_georeference_country_analysis' | ||
].join('\n')); | ||
|
||
GeoreferenceCountry.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
// given that country polygons are large and there are less than 200 countries in the world | ||
// we'll set a modest default limit for the number of resulting rows | ||
this.requirements.setSingleInputNumberOfRows('source', limits, 500); | ||
return callback(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,3 +77,15 @@ var queryTemplate = Node.template([ | |
' ) AS the_geom', | ||
'FROM ({{=it.source}}) AS _camshaft_georeference_postal_code_analysis' | ||
].join('\n')); | ||
|
||
GeoreferencePostalCode.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
var default_limit; | ||
if (getGecoderFunction(this.output_geometry_type) === 'cdb_geocode_postalcode_polygon') { | ||
default_limit = 10000; | ||
} | ||
else { | ||
default_limit = 1000000; | ||
} | ||
this.requirements.setSingleInputNumberOfRows('source', limits, default_limit); | ||
return callback(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. This makes code more complex than needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry that we need this at the moment 😬 , but we'll get rid of it in the next generation 😁 |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
'use strict'; | ||
|
||
var Node = require('../node'); | ||
var NodeRequirements = require('../requirements'); | ||
var debug = require('../../util/debug')('analysis:line-sequential'); | ||
|
||
var TYPE = 'line-sequential'; | ||
|
@@ -43,3 +44,33 @@ var routingSequentialQueryTemplate = Node.template([ | |
' {{? it.category_column }}GROUP BY {{=it.category_column}}{{?}}', | ||
') _cdb_analysis_line_sequential' | ||
].join('\n')); | ||
|
||
LineSequential.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
var inputRows = this.source.requirements.getEstimatedRequirement('numberOfRows'); | ||
var numCategories = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a big fan of closured variables, I'd pass them as parameter in function calls instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And receive them from callbacks (asynchronous) or from returned values of functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since: a) this code will be eventually replaced b) it seems to work c) the closure vars that bother you are encapsulated in a single function, I'll keep this as is, for fear of introducing new bugs. are you OK with it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. go ahead! |
||
var self = this; | ||
var rowsLimit = NodeRequirements.getNodeLimit(limits, TYPE, 'maximumNumberOfRows', 1000000); | ||
var pointsLimit = NodeRequirements.getNodeLimit(limits, TYPE, 'maximumNumberOfPointsPerLine', 100000); | ||
function storeRequirements(err) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd try to avoid closure functions. Extract it at module level or use private methods ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the same reasons mention above, I'd prefer to keep this as is (I consider this throw-away code have our limits working until we implement proper limit-checking) |
||
// we make a rough estimate of numCategories result rows and inputRows/numCategories points per line | ||
self.requirements.setEstimatedRequirement('numberOfRows', numCategories); | ||
self.requirements.setEstimatedRequirement('numberOfPointsPerLine', inputRows/(numCategories || 1)); | ||
self.requirements.setLimit('maximumNumberOfRows', rowsLimit); | ||
self.requirements.setLimit('maximumNumberOfPointsPerLine', pointsLimit); | ||
return callback(err); | ||
} | ||
storeRequirements(null); | ||
}; | ||
|
||
LineSequential.prototype.requirementMessages = function() { | ||
var messages = []; | ||
if (this.requirements.getEstimatedRequirement('numberOfRows') > | ||
this.requirements.getLimit('maximumNumberOfRows')) { | ||
messages.push('too many result rows'); | ||
} | ||
if (this.requirements.getEstimatedRequirement('numberOfPointsPerLine') > | ||
this.requirements.getLimit('maximumNumberOfPointsPerLine')) { | ||
messages.push('too many points per line'); | ||
} | ||
return messages; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,3 +59,15 @@ LineSourceToTarget.prototype.sql = function() { | |
|
||
return sql; | ||
}; | ||
|
||
LineSourceToTarget.prototype.computeRequirements = function(databaseService, limits, callback) { | ||
var default_limit = 1000000; | ||
if (this.closest) { | ||
this.requirements.setSingleInputNumberOfRows('source', limits, default_limit); | ||
} else { | ||
// this is the absolute maximum number of lines; it could be greatly reduced | ||
// if this.node.source_column && this.node.target_column | ||
this.requirements.setProductInputNumberOfRows(limits, default_limit); | ||
} | ||
return callback(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't use callbacks for synchronous functions. |
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈