add bulkDelete_underscoreAPI.js and count_underscoreAPI.js to stored procedures samples #5
Conversation
Adds two stored procedures to the samples.
|
Just following up - any movement on this PR? Thanks! |
|
@dwhieb : Sorry for the delay. I've asked the respective owners to take a look at it. |
|
No worries - thanks! |
|
Thanks, @dwhieb! These samples are great. Our bank of stored procedure samples currently includes Count.js which performs a document count with an optional filter and bulkDelete.js which deletes documents for a given query. What additional functionality did you want to add with clear.js and length.js? |
|
Well for both sprocs I added the option to pass a key and value to filter on. And for count.js I used the collection's .filter() method, which seemed a little more straightforward since I was in fact filtering. I'd be happy to incorporate these changes into the sprocs already in the repo if that'd be better. And in retrospect I think passing a filter object rather than two arguments would be a better way to structure the filter option. |
|
Thanks for reaching out! |
| @@ -0,0 +1,121 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can this be renamed to bulkDelete_underscoreAPI.js to make it clear this is similar to bulkDelete.js but using underscore API?
| }; | ||
|
|
||
| // filter the collection for sessions using the filter function | ||
| const accepted = __.filter(filter, { continuation: continuationToken }, handler); |
There was a problem hiding this comment.
Perf: the predicate function needs to be inlined in order to take advantage of index (otherwise it will be full scan).
| /** | ||
| * A stored procedure for Azure DocumentDB which gets a count of the session documents using the .filter() method of the collection. | ||
| * @function | ||
| * @param {String} filterOn = 'type' The key to filter documents on for deletion. |
There was a problem hiding this comment.
Should say for counting instead of deletion.
| @@ -0,0 +1,90 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can this be renamed to count_underscoreAPI.js to make it clear this is similar to count.js but using underscore API?
| }; | ||
|
|
||
| // filter the collection for sessions using the filter function | ||
| const accepted = __.filter(filter, { continuation: continuationToken }, handler); |
There was a problem hiding this comment.
Perf: the predicate function needs to be inlined in order to take advantage of index (otherwise it will be full scan).
|
|
||
| } | ||
|
|
||
| module.exports = length; |
There was a problem hiding this comment.
I don't think module is currently supported (will be soon).
| * @param {String} info.continuation The continuation token, if any was passed | ||
| * @return {responseBody} | ||
| */ | ||
| const handler = function handler(err, sessions, info) { |
There was a problem hiding this comment.
Can we rename 'sessions' parameter to something more generic, like docs, or documents, or docFeed?
| /** | ||
| * Recursively deletes each session in an array, and then attempts to get more to delete | ||
| * @function | ||
| * @param {Array} sessions The array of session documents to delete |
There was a problem hiding this comment.
can we rename sessions for function name and function parameter to more generic? Like deleteDocuments etc.
|
Great! I'll tackle these changes now. |
* also camelCase file names for consistency
|
@koltachev Ok I've added my revisions to the PR and addressed each of your comments (I noted the commit for each), as well as changed the way the filter parameter is structured, so that the sprocs now take a single (optional) filter object as an argument. |
dwhieb
changed the title
| // NB: The filter function must be inlined in order to take advantage of index | ||
| // (otherwise it will be a full scan). | ||
| const accepted = __.filter(function filter(doc) { | ||
| return Object.keys(filterObj).every(function checkProperty(filterKey) { |
There was a problem hiding this comment.
Function calls are not yet supported for optimized index, so this will work but again, will be full scan - slow. This scenario will work when we have __.where implemented. For now, I guess, let's change the sample to the version it was before -- simple filtering by property name and property value passed from outside.
| * @param {String} info.continuation The continuation token, if any was passed | ||
| * @return {responseBody} | ||
| */ | ||
| const handler = function handler(err, docs, info) { |
There was a problem hiding this comment.
Can we change this to filterHandler to be more explicit?
| * @param {String} info.continuation The continuation token, if any was passed | ||
| * @return {responseBody} | ||
| */ | ||
| const handler = function handler(err, docs, info) { |
There was a problem hiding this comment.
Can we change this to filterHandler to be more explicit? Same for the count sproc.
| // NB: The filter function must be inlined in order to take advantage of index | ||
| // (otherwise it will be a full scan). | ||
| const accepted = __.filter(function filter(doc) { | ||
| return Object.keys(filterObj).every(function checkProperty(filterKey) { |
There was a problem hiding this comment.
Function calls are not yet supported for optimized index, so this will work but again, will be full scan - slow. This scenario will work when we have __.where implemented. For now, I guess, let's change the sample to the version it was before -- simple filtering by property name and property value passed from outside. Same for the count sproc.
|
@mkolt Should be all set! |
| @@ -1,59 +1,59 @@ | |||
| /** | |||
There was a problem hiding this comment.
Is this file getting modified? Looks like it's not. Can we remove if it from the change?
| @@ -1,66 +1,66 @@ | |||
| /** | |||
There was a problem hiding this comment.
Is this file getting modified? Looks like it's not. Can we remove if it from the change?
| @@ -1,26 +1,26 @@ | |||
| /** | |||
There was a problem hiding this comment.
Does the change need to modify this file?
| // filter the collection for documents using the filter object | ||
| const accepted = __.filter(function filter(doc) { | ||
|
|
||
| if (filterValue) { |
There was a problem hiding this comment.
Sorry :) Same perf issue. For optimized index, function calls and if-statements are currently not supported. How about if we change it to be like this: return filterKey === undefined || doc[filterKey] === doc[filterValue];
And same for the other sample...
No description provided.