Conversation
This 'view' was not really a view, because it did not produce results based on the variable store. Instead it simply took parameters and returned an object that was a valid terms query. This is a good feature to have, but it should be internal to this library only.
This doesn't change the output at all, but makes the code simpler
orangejulius
force-pushed
the
leaf-queries-and-libs
branch
from
678c037 to
83f27f5
Compare
orangejulius
force-pushed
the
leaf-queries-and-libs
branch
from
83f27f5 to
74d86c1
Compare
missinglink
left a comment
missinglink
left a comment
There was a problem hiding this comment.
Looks good! I added a few comments.
lib/leaf/match.js
Outdated
| } | ||
| }; | ||
|
|
||
| const extra_params = ['boost', |
There was a problem hiding this comment.
can we define this array out of the function scope to avoid re-allocating memory on every invocation?
also, unusual formatting 🤷♂ almost missed the first element
There was a problem hiding this comment.
I can't believe that V8 wouldn't be able to perform this optimization itself, and either way, I feel like the readability benefit of having the array close to where it is used outweighs any potential performance changes, so I'd like to leave this code as is.
lib/leaf/match.js
Outdated
| 'minimum_should_match', | ||
| 'zero_terms_query']; | ||
|
|
||
| extra_params.forEach(function(param) { |
There was a problem hiding this comment.
sorry for sounding pedantic, could we do the if(extra) check before the iteration, in the most simple use case this would perform 10 iterations unnecessarily.
it's not really a huge deal, but it's nice to try to optimize performance on primitive building blocks like this because the effects will propagate up and multiply.
There was a problem hiding this comment.
could use fat arrow function syntax here for succinctness?
There was a problem hiding this comment.
Same as above: I bet V8 can do this for us, and wrapping the entire loop in another if statement will make it that much harder to read. I'd rather not change it.
| @@ -0,0 +1,17 @@ | |||
| module.exports = function( property, value, parameters ){ | |||
There was a problem hiding this comment.
here it's called parameters while in the others it's called extra, is that intentional?
There was a problem hiding this comment.
Yeah, i meant to rename them both to parameters, which is a better name. This is now fixed.
This is a reusable view to build `match_phrase` queries
This one's real simple
This view can be instantiated many times
orangejulius
force-pushed
the
leaf-queries-and-libs
branch
from
74d86c1 to
b650df8
Compare
|
Here's the perf test https://jsperf.com/leaf-queries-perf/1 It so fast that even though the code calls for an array memory allocation on every function call, the effect is so tiny that it's not worth worrying about. The tests also highlight that there is a minor perf benefit to having the immutable data defined outside the function scope, I suspect that the v8 engine is still having to do the memory allocation and deallocation on every entry/exit to the code path. Looking at how fast it is, It's not worth making a fuss over, but I'm going to remain sceptical that v8 will be able to optimize things like this down to a no-op 😄 |
|
Sorry for hijacking this awesome PR to rant about js performance 🙇 |
This has been shown to slightly improve performance in microbenchmarks https://jsperf.com/leaf-queries-perf/1
|
Interesting, I'm really surprised that V8 (also Spidermonkey) isn't smart enough to handle that. Looks like most of the perf gain comes from moving the array out of the function, so I've done that now. They're just microbenchmark numbers, but we might as well go with them. |
This uses the match_phrase helper function defined in #109 to cut down on duplication when creating `match_phrase` queries. Since the structure of the `match_phrase` query is defined in one place, it makes the resulting view code a bit more concise. It will also make optional parameter handling easier and more consistent.
2 participants
This PR constitutes a major rearrangement of the pelias/query module internals to have a bit more consistency. It introduces a couple new concepts, but makes no breaking changes.
Major changes
Commonly used Elasticsearch leaf queries now all have their own views
It took some time to come up with a word to explain what a
match/match_phrase/termquery is, in comparison to say, a Pelias-specific query generated by the autocomplete endpoint.The 'leaf' terminology comes from the main Elasticsearch Query DSL docs page, and seems fits well. It sets us up to create well organized views for 'compound' queries like
boolin the future too.These new 'leaf' views also have the ability to be instantiated multiple times by configuring where they look in the VariableStore for configuration settings. This means a lot of sophisticated query construction can now be done without creating task-specific views at all.
Each of these views will accept only parameters that are allowed for the given query type, and will omit parameters that are unset. Therefore, this PR closes #101 and addresses the feedback given there regarding default values.
Leaf query views have been created for
match,match_phrase, andmatch_allNew Internal functions to encapsulate leaf view functionality for reuse
There was at least one "view" (for the terms query) that did not use variable store values, and merely returned a query object directly. This is nice to have to reduce duplication, but exposing it to consumers of the
pelias/querymodule would result in queries that do not change from request to request. This could have lead to confusing and hard to debug issues, so having a clear separation of functionality is important.All leaf queries now have these functions but they are not exported by the module. Instead they are used in other, more Pelias specific views (such as
sources,layers,categories, etc), and will hopefully be used even more over time.Better separation between
matchandmatch_phraseviewsIn Elasticsearch 6 and beyond, Elasticsearch does not support conflating the
matchandmatch_phrasequeries as we currently do, so these new leaf views implement separate functionality formatchandmatch_phrase, and will generate queries that are compatible with ES6.Next steps
This PR itself does not make any breaking changes, but it does allow a new set of tools to be used in pelias/api that can move us forward.
There is currently a large amount of domain-specific, ES6 incompatible query generation code in pelias/API. Once this PR is merged, the API can start leveraging these new query types to reduce code complexity and improve Elasticsearch compatibility in follow up work.
Connects pelias/pelias#719