Skip to content
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

perf: use prepared statment cache #241

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"jsftp": "^2.0.0",
"lodash": "^4.17.4",
"morgan": "^1.9.0",
"node-postal": "^1.0.0",
"node-postal": "https://github.com/imothee/node-postal.git",
Copy link
Member Author

@missinglink missinglink Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I needed this for testing nodejs@v12, @orangejulius what should we do about this?
it's only tangentially related to the PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to merge it. I hadn't merged it yet because I figured we'd be merging #146, but its been a while now and Node.js 12 is still not technically supported.

"pbf2json": "^6.4.0",
"pelias-config": "^4.0.0",
"pelias-logger": "^1.2.1",
Expand Down
69 changes: 42 additions & 27 deletions query/search.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@

// maximum names to match on
var MAX_NAMES = 10;
const MAX_NAMES = 10;

// maximum address records to return
var MAX_MATCHES = 20;
const MAX_MATCHES = 20;

/**
this query should only ever return max 3 rows.
note: the amount of rows returned does not adequently indicate whether an
exact match was found or not.
**/

var SQL = [
const SQL = [
'WITH base AS (',
'SELECT id, housenumber, rowid',
'FROM address',
Expand All @@ -22,8 +21,8 @@ var SQL = [
'SELECT id',
'FROM street.rtree',
'WHERE (',
'street.rtree.minX<=?1 AND street.rtree.maxX>=?1 AND',
'street.rtree.minY<=?2 AND street.rtree.maxY>=?2',
'street.rtree.minX<=$lon AND street.rtree.maxX>=$lon AND',
'street.rtree.minY<=$lat AND street.rtree.maxY>=$lat',
')',
')',
'AND ( %%NAME_CONDITIONS%% )',
Expand All @@ -33,50 +32,66 @@ var SQL = [
'WHERE rowid IN (',
'SELECT rowid FROM (',
'SELECT * FROM base',
'WHERE housenumber < "%%TARGET_HOUSENUMBER%%"',
'WHERE housenumber < $housenumber',
'GROUP BY id HAVING( MAX( housenumber ) )',
'ORDER BY housenumber DESC',
')',
'UNION',
'SELECT rowid FROM (',
'SELECT * FROM base',
'WHERE housenumber >= "%%TARGET_HOUSENUMBER%%"',
'WHERE housenumber >= $housenumber',
'GROUP BY id HAVING( MIN( housenumber ) )',
'ORDER BY housenumber ASC',
')',
')',
'ORDER BY housenumber ASC', // @warning business logic depends on this
'LIMIT %%MAX_MATCHES%%;'
`LIMIT ${MAX_MATCHES};`
].join(' ');

var NAME_SQL = '(street.names.name=?)';
// SQL prepared statements dont easily support variable length inputs.
// This function dynamically generates a SQL query based on the number
// of 'name' conditions required.
function generateDynamicSQL(count){
const conditions = new Array(count)
.fill('(street.names.name=?)')
.map((sql, pos) => sql.replace('?', `$name${pos}`));

module.exports = function( db, point, number, names, cb ){
return SQL.replace('%%NAME_CONDITIONS%%', conditions.join(' OR '));
}

// Reusing prepared statements can have a ~10% perf benefit
const cache = [];
function statementCache(db, count){
if (!cache[count]) {
cache[count] = db.prepare(generateDynamicSQL(count));
}
return cache[count];
}

module.exports = function( db, point, number, names, cb ){
// error checking
if( !names || !names.length ){
return cb( null, [] );
}

// max conditions to search on
var max = { names: Math.min( names.length, MAX_NAMES ) };

// use named parameters to avoid sending coordinates twice for rtree conditions
var position = 3; // 1 and 2 are used by lon and lat.
const max = { names: Math.min( names.length, MAX_NAMES ) };

// add name conditions to query
var nameConditions = Array.apply(null, new Array(max.names)).map( function(){
return NAME_SQL.replace('?', '?' + position++);
});
// use a prepared statement from cache (or generate one if not yet cached)
const stmt = statementCache(db, max.names);

// build unique sql statement
var sql = SQL.replace( '%%NAME_CONDITIONS%%', nameConditions.join(' OR ') )
.replace( '%%MAX_MATCHES%%', MAX_MATCHES )
.split( '%%TARGET_HOUSENUMBER%%' ).join( number );
// query params
const params = {
$lon: point.lon,
$lat: point.lat,
$housenumber: number
};

// create a variable array of params for the query
var params = [ point.lon, point.lat ].concat( names.slice(0, max.names) );
// each name is added in the format: $name0=x, $name1=y
names.slice(0, max.names).forEach((name, pos) => {
params[`$name${pos}`] = name;
});

// execute query
db.all( sql, params, cb );
};
stmt.all(params, cb);
};