optimize sort #55
optimize sort #55
Conversation
| addSorting() { | ||
| if (this.sortFields) { | ||
| // adds multiple sort orders | ||
| this.sortFields.forEach((sortObj) => { |
There was a problem hiding this comment.
I would put this down by line 128 in addOptimizations, seems a little scattered to have it here.
There was a problem hiding this comment.
Oh. I thought having all the order by statements together would be easiest to read since this function determines sort order. No?
There was a problem hiding this comment.
You're the boss! In my head even though they both use the order by clause they're pretty unrelated, I'd rather have all the code that deals with sort optimization together.
|
(also - I'm aware I need to rebase with master on this one - will do so after the +1 so that the latest commit acts as a diff to the previous code that you've already looked at.) |
| if (this.sortBorderValue) { | ||
| query = query.where( | ||
| this.sortFields[0].field, | ||
| this.sortFields[0].direction === 'desc' ? '<=' : '>=', |
There was a problem hiding this comment.
How are we making sure we don't get duplicate points if there's a bunch of points with sortBorderValue as their value spanning two pages?
There was a problem hiding this comment.
So the way this works is that each result set consists of all values of the border value. For example, lets say the borderVals were 1,2,2,3,3,4,5 and the fetchSize is 4 then only 1,2,2 would be returned: the last value becomes the border value none of those will be in the result set. Then the next result set is >=3. In other words, points with the border value are not included in result set. If the entire fetch has the same border value then we throw an error.
There was a problem hiding this comment.
I see, and that's in processPaginatedResults called with the sort field.
1. array must be provided to sortBy method. 2. Take out head proc from test juttles because head cuts off the result set before the massaging which makes the massaging ineffective
| } | ||
|
|
||
| let groupby = graph.node_get_option(sort, 'groupby'); | ||
| if (groupby) { |
There was a problem hiding this comment.
It's a little more canonical to say if (graph.node_has_option(sort, 'groupby')) { in cases like this.
|
+1 |
|
still requires a bit of work to clean up. Let me know if someone wants this finished. |
|
What's missing? We've come too far to give up now! |
Sort optimization with Juttle 7 bump.
fixes #36