refactor: Rework update and delete node to remove secondary planner #571
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
bug
Codecov Report
@@ Coverage Diff @@
## develop #571 +/- ##
===========================================
- Coverage 56.98% 55.84% -1.14%
===========================================
Files 121 121
Lines 14243 14169 -74
===========================================
- Hits 8116 7913 -203
- Misses 5413 5558 +145
+ Partials 714 698 -16
|
AndrewSisley
force-pushed
the
sisley/refactor/I534-update-delete-node
branch
2 times, most recently
from
36e3f76
to
24880e6
Compare
AndrewSisley
added
the
action/no-benchmark
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fredcarle
reviewed
Jun 29, 2022
db/fetcher/fetcher.go
Outdated
| @@ -114,22 +114,24 @@ func (df *DocumentFetcher) Start(ctx context.Context, txn datastore.Txn, spans c | |||
| } | |||
| //@todo: Handle fields Description | |||
There was a problem hiding this comment.
question: is there an issue open for this?
There was a problem hiding this comment.
No idea, would guess that there isn't, and would suggest that finding out/removing-comment is out of scope
There was a problem hiding this comment.
I think I removed it anyway :)
db/fetcher/fetcher.go
Outdated
| numspans := len(spans) | ||
| var uniqueSpans core.Spans | ||
| if numspans == 0 { // no specified spans so create a prefix scan key for the entire collection | ||
| uniqueSpans := core.Spans{ |
There was a problem hiding this comment.
suggestion: to avoid a partially initialized variable declaration (setting HasValue puts it in a confusing state) you could operate directly on df.spans. There is no error return route so there is no chance of leaving it in a wrong state. So on line 123 you could do df.spans = ... directly and on 131 you could do:
df.spans = core.Spans{
HasValue = true,
Value = core.MergeAscending(valueSpans),
}There was a problem hiding this comment.
Agree that this was done a bit strangely and could be improved, will revisit
- fetcher df.Spans oddness
| "github.com/sourcenetwork/defradb/client" | ||
| "github.com/sourcenetwork/defradb/core" | ||
| "github.com/sourcenetwork/defradb/query/graphql/mapper" | ||
| ) | ||
|
|
||
| type deleteNode struct { | ||
| documentIterator |
There was a problem hiding this comment.
question: I know this is not new as it's also in all the other node types but why not use just core.Doc here instead of a struct with one value which is of type core.Doc with only one method which is just returning the core.Doc?
There was a problem hiding this comment.
documentIterator provides the Values() func
There was a problem hiding this comment.
You could have Values() on core.Doc and you'd remove the confusion no?
There was a problem hiding this comment.
Same question as fred again
There was a problem hiding this comment.
core.Doc is 100% not the place for a Values() function lol
There was a problem hiding this comment.
Maybe. But I find this pattern kinda weird so that's why I brought it up. Do we even need a Values function? Can we not get core.Doc directly?
There was a problem hiding this comment.
Values() is a very important function on the planNode interface :) docIterator just saves having to define exactly the same thing for every node
Would be no way to get the current value without that func existing
There was a problem hiding this comment.
I see what you mean. Also we keep saying Values but it's Value lol. I just think of docIterator as something you would call Next on but it only has Value which is weird to me. Maybe changing the name to something else would resolve this.
There was a problem hiding this comment.
Yeah, I think someone (John?) pointed that out when it first went in, but we shrugged it off as unimportant
| @@ -129,9 +82,9 @@ func (n *updateNode) Next() (bool, error) { | |||
|
|
|||
| func (n *updateNode) Kind() string { return "updateNode" } | |||
|
|
|||
| func (n *updateNode) Spans(spans core.Spans) { /* no-op */ } | |||
| func (n *updateNode) Spans(spans core.Spans) { n.results.Spans(spans) } | |||
There was a problem hiding this comment.
thought (non-blocking): again knowing that this is not new to this PR, I still want to point out the name of this methods makes me think that by calling it I'll be receiving the Spans. If core.Spans had a method Set, this could then become:
func (n *updateNode) Spans() core.Spans { n.results.Spans() }Then you could call something like:
n.subType.Spans().Set(n.spans)I think it would make it much easier to understand just by reading it.
Although it's out of scope here, I'd like us to consider that for a future change.
There was a problem hiding this comment.
Makes sense, but very much out of scope IMO
shahzadlone
reviewed
Jun 30, 2022
| Value []Span | ||
| } | ||
|
|
||
| func NewSpans(spans ...Span) Spans { |
There was a problem hiding this comment.
thought: the name NewSpans makes me feel like I would get an Spans struct that's empty and just initialized. Maybe NewFromSpans or CloneSpans (I don't like these much either) haha just a thought incase you have a better name in mind.
There was a problem hiding this comment.
actually nevermind I see a similar NewSpan function so it's conisistent.
There was a problem hiding this comment.
We also have many other 'constuctor-like' functions using this naming convention. IMO if you want an empty you'd just do Foo{}
shahzadlone
reviewed
Jun 30, 2022
| @@ -154,7 +154,17 @@ func isAdjacent(this DataStoreKey, other DataStoreKey) bool { | |||
| } | |||
|
|
|||
| // Spans is a collection of individual spans. | |||
| type Spans []Span | |||
| type Spans struct { | |||
There was a problem hiding this comment.
question: What's the benefit of doing this? Now I see some places use Spans and some use []Span is there a need for the differentiation between the two?
There was a problem hiding this comment.
Yes, Spans contains a HasValue property that allows differentiation between an explicitly empty collection, and one that is just default.
shahzadlone
reviewed
Jun 30, 2022
| @@ -171,12 +181,12 @@ type HeadKeyValue struct { | |||
| // a unique set in ascending order, where overlapping spans are merged into a single span. | |||
| // Will handle spans with keys of different lengths, where one might be a prefix of another. | |||
| // Adjacent spans will also be merged. | |||
| func (spans Spans) MergeAscending() Spans { | |||
| func MergeAscending(spans []Span) []Span { | |||
There was a problem hiding this comment.
question: why not MergeAscending(spans Spans)?
There was a problem hiding this comment.
this is more convenient for the places that use it
shahzadlone
reviewed
Jun 30, 2022
| @@ -55,7 +55,7 @@ type Mutation struct { | |||
| // if this mutation is on an object. | |||
| Schema string | |||
|
|
|||
| IDs []string | |||
| IDs parserTypes.OptionalDocKeys | |||
There was a problem hiding this comment.
question: Sorry I know this was named this way from before, but since we are making it an OptionalDocKeys type, wondering why we don't change it from IDs to DocKeys like in the Select.
There was a problem hiding this comment.
Yeah that bugs me a too, but would say it is out of scope (I think the query spec if inconsistent between the two too)
shahzadlone
reviewed
Jun 30, 2022
| @@ -81,8 +81,8 @@ func (h *headsetScanNode) Start() error { | |||
| } | |||
|
|
|||
| func (h *headsetScanNode) initScan() error { | |||
| if len(h.spans) == 0 { | |||
| h.spans = append(h.spans, core.NewSpan(h.key, h.key.PrefixEnd())) | |||
| if len(h.spans.Value) == 0 { | |||
There was a problem hiding this comment.
question: why not !h.spans.HasValue
There was a problem hiding this comment.
The bug is in select/scan - this preserves the existing behaviour, which until otherwise shown should be assumed to be the correct behaviour. Using !h.spans.HasValue would be a behaviour change.
shahzadlone
reviewed
Jun 30, 2022
| @@ -187,10 +140,11 @@ func (p *Planner) UpdateDocs(parsed *mapper.Mutation) (planNode, error) { | |||
| update.collection = col.WithTxn(p.txn) | |||
|
|
|||
| // create the results Select node | |||
| slctNode, err := p.Select(&parsed.Select) | |||
| resultsNode, err := p.Select(&parsed.Select) | |||
There was a problem hiding this comment.
question: Why the name change? I think select node makes more sense no? or maybe just results.
There was a problem hiding this comment.
Depends how you look at it I think. n.results existed before this PR, and TBH I don't think the name matters at all - it only lives for a couple of lines.
shahzadlone
reviewed
Jun 30, 2022
| "selectTopNode": dataMap{ | ||
| "selectNode": dataMap{ | ||
| "deleteNode": dataMap{ | ||
| "deleteNode": dataMap{ |
There was a problem hiding this comment.
suggestion: change the name of this test function as this isn't a failure case anymore. Or move this case to a Success case now.
Same for some other queries that now shouldn't be returning error.
There was a problem hiding this comment.
Will do
- Test names etc
There was a problem hiding this comment.
I'm guessing your comment is on the wrong line/file - adjusting other tests
There was a problem hiding this comment.
github wouldn't let me comment of the function name lines as it's too far up. Bassically just need to check the test cases in the functions that end with _Failure (if they still fail or should now be under the _Success function).
There was a problem hiding this comment.
Have done - sorry I squashed the commits that did this, as I incorrectly thought Fred had approved and was about to merge lol
shahzadlone
reviewed
Jun 30, 2022
| @@ -118,9 +118,8 @@ func TestDeletionOfADocumentUsingSingleKey_Failure(t *testing.T) { | |||
| _key | |||
There was a problem hiding this comment.
suggestion: Similar to previous comment the test function name suggests this is a failure case still, but it not anymore.
shahzadlone
reviewed
Jun 30, 2022
query/graphql/planner/select.go
Outdated
| @@ -137,11 +137,6 @@ func (n *selectNode) Start() error { | |||
| // remaining top level filtering, and | |||
| // renders the doc. | |||
| func (n *selectNode) Next() (bool, error) { | |||
| //todo - this is a bit lazy | |||
There was a problem hiding this comment.
question: Was this the hack you mentioned in the commit title?
shahzadlone
approved these changes
Jun 30, 2022
AndrewSisley
force-pushed
the
sisley/refactor/I534-update-delete-node
branch
4 times, most recently
from
89c1e4e
to
7ecb358
Compare
Update included properties from a different schema to the test target. There is a bug in both test framework and collection.update that allows this to happen, but that should be dealt with later.
Previous code would spawn a new planner and construct a new select node chain in collection.delete. This feels much nicer to me, and removes the circular dependency that impedes further cleanup within collection.delete
Previous code would spawn a new planner and construct a new select node in collection.update. This feels much nicer to me, and removes the circular dependency that impedes further cleanup within collection.update
AndrewSisley
force-pushed
the
sisley/refactor/I534-update-delete-node
branch
from
7ecb358
to
bf9b8e6
Compare
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
…ourcenetwork#571) * Update test with valid fields Update included properties from a different schema to the test target. There is a bug in both test framework and collection.update that allows this to happen, but that should be dealt with later. * Remove duplicate test case * Correctly handle empty collection of ids in selects * Rework delete node to remove secondary planner Previous code would spawn a new planner and construct a new select node chain in collection.delete. This feels much nicer to me, and removes the circular dependency that impedes further cleanup within collection.delete * Rework update node to remove secondary planner Previous code would spawn a new planner and construct a new select node in collection.update. This feels much nicer to me, and removes the circular dependency that impedes further cleanup within collection.update
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.
Relevant issue(s)
Resolves #534
Description
Fixes an explain test that was incorrectly updating test data. Fixes (and tests) a bug in select where providing an empty collection of ids would result in the full collection being returned. Reworks delete and update node to remove some circular logic.
Recommend reviewing commit by commit.
PROPOSAL/FIXUP - Amends spans to properly handle explicitly empty spansimproves upon (IMO) the somewhat hacky fix inCorrectly handle empty collection of ids in selects, but is a bit more complex - might want to review those commits together depending on preference.Update and delete are not benchmarked, however I would expect gains in delete, unsure about update as there remains some inefficiency there. Performance not a goal of this PR though.
Note: Code coverage of collection.delete/update has dropped significantly as the collection abi is extremely poorly tested, and previously gained some coverage due to the unusual code paths taken prior to this PR.
Tasks
Amends spans to properly...before merge depending on reviewer feedbackSpecify the platform(s) on which this was tested: