refactor: Rework planNode Next and Value(s) function #374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewSisley
added
area/query
AndrewSisley
requested review from
orpheuslummis,
jsimnz,
fredcarle and
shahzadlone
Codecov Report
@@ Coverage Diff @@
## develop #374 +/- ##
===========================================
- Coverage 65.26% 65.20% -0.07%
===========================================
Files 80 80
Lines 9251 9251
===========================================
- Hits 6038 6032 -6
- Misses 2595 2599 +4
- Partials 618 620 +2
|
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Moves Next to next to Values, and Spans next to Init and Start - feels a more natural grouping based on when/where they will be called.
I'm guessing this is out of date, as it doesnt make sense to me
Singular removes suggestion that it might return an entire iteration, and more strongly suggests that it is just a getter with the bulk of the work taking place in Next.
Also handles the previously ignored error
Also handles the previously ignored errors
Also handles the previously ignored errors
AndrewSisley
force-pushed
the
sisley/refactor/I180-node-values-value
branch
from
9dfbb95
to
144c4e1
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jsimnz
approved these changes
Apr 27, 2022
There was a problem hiding this comment.
Few notes, no notable issues since its mostly just renaming the interface funcs and their references. As far as I can tell, any of the nodes which had functionality in the Values() method has been accurately ported to the Next() call instead.
Made a comment about the documentIterator just from a naming perspective.
Comment on lines
-402
to
+398
| m := ¶llelNode{p: s.p, doc: make(map[string]interface{})} | ||
| m := ¶llelNode{p: s.p} |
There was a problem hiding this comment.
Are we sure we don't need to instanciate the map here?
There was a problem hiding this comment.
No it is it always assigned to before access (first line of Next)
| @@ -423,7 +419,6 @@ func (s *selectNode) addSubPlan(field string, plan planNode) error { | |||
| multinode := ¶llelNode{ | |||
| p: s.p, | |||
| multiscan: multiscan, | |||
| doc: make(map[string]interface{}), | |||
There was a problem hiding this comment.
Same as before, do we not need to instantiate the map?
There was a problem hiding this comment.
Same as other comment: No it is it always assigned to before access (first line of Next)
| @@ -131,8 +128,8 @@ func (n *selectNode) Next() (bool, error) { | |||
| return false, err | |||
| } | |||
|
|
|||
| n.doc = n.source.Values() | |||
| passes, err := parser.RunFilter(n.doc, n.filter, n.p.evalCtx) | |||
| n.currentValue = n.source.Value() | |||
There was a problem hiding this comment.
should the n.source.Value() go into a temp variable until we know the filter passes? Instead of directly being assigned to the currentValue var?
There was a problem hiding this comment.
Doesnt really matter - if Next returns false this value is invalid/irrelevant anyway - and unless adding some logic to explicitly set it to nil if returning false (which I see as a valid, but wasteful option, as it would never be used)
| return false, nil | ||
| } | ||
|
|
||
| passed, err := parser.RunFilter(n.doc, n.filter, n.p.evalCtx) | ||
| passed, err := parser.RunFilter(n.currentValue, n.filter, n.p.evalCtx) |
There was a problem hiding this comment.
Same comment as before about assigning a value to currentValue before running the Filter. I don't think technically it could be an issue since the planner is executed serially in a single goroutine, just a thought
There was a problem hiding this comment.
See other comment to my response to this, also worth noting is style (Next/Value) that (normal) iterators are inherently thread-unsafe as there will always be a race condition unless a lock is exposed that can wrap Next+Value calls
| func (n *baseNode) Close() error { return n.plan.Close() } //nolint:unused | ||
| func (n *baseNode) Source() planNode { return n.plan } //nolint:unused | ||
|
|
||
| type documentIterator struct { |
There was a problem hiding this comment.
The naming here might not be quite right. If you have anything off the top of your head that might be more approprite to its use (embedded doc map and Value() method) as itself isn't doing anything w.r.t the "Iterator" pattern, but is just an internal utility to avoid code duplication.
If nothing immediately comes to mind, you can merge as is.
There was a problem hiding this comment.
Yeah I agree with you, the name is a bit off as it itself isn't a complete iterator. Nothing comes to mind atm, but will very happily change retrospectively if I (or anyone else) comes up with a better idea
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Reorder plannode interface funcs Moves Next to next to Values, and Spans next to Init and Start - feels a more natural grouping based on when/where they will be called. * Remove ambiguous comment-line I'm guessing this is out of date, as it doesnt make sense to me * Rename planNode.Values to Value Singular removes suggestion that it might return an entire iteration, and more strongly suggests that it is just a getter with the bulk of the work taking place in Next. * Introduce documentIterator * Move logic from count.Values into count.Next * Move logic from create.Values into create.Next Also handles the previously ignored error * Move logic from dagscan.Values into dagscan.Next * Move logic from commitSelect.Values into commitSelect.Next * Remove misleading comment from delete.Next * Refactor group node to use document iterator * Move logic from limit.Values into limit.Next * Refactor parallel node to use document iterator * Move logic from pipe.Values into pipe.Next * Move logic from render.Values into render.Next * Refactor scan node to use document iterator * Refactor select node to use document iterator * Move logic from sum.Values into sum.Next * Move logic from typeJoinOne.Values into typeJoinOne.Next * Move logic from typeJoinMany.Values into typeJoinMany.Next Also handles the previously ignored errors * Remove commented out code * Move logic from update.Values into update.Next Also handles the previously ignored errors * Refactor versionScan node to use document iterator
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #180
As discussed over discord, renames the planNode.Values function to Value and moves any logic held within the Values implementations into the Next implementations. Makes use of a new
documentIteratorstruct to shrink the implementation effort for individual node types.A handful of nodes do not bother using the documentIterator as their Value(s) function just calls a child planNode.