refactor: Rework core.Spans #3210
Conversation
AndrewSisley
added
area/query
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3210 +/- ##
==========================================
Coverage ? 77.34%
==========================================
Files ? 376
Lines ? 34791
Branches ? 0
==========================================
Hits ? 26906
Misses ? 6263
Partials ? 1622
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
| "spans": []dataMap{ | ||
| { | ||
| "end": "/4", | ||
| "start": "/3", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
question: So this was the bug that was caused due to the way HasValue was being used?
There was a problem hiding this comment.
Yeah, there was something fishy going on here with HasValue. The newly expected result is correct, and the old property/type has died so I didn't bother chasing down exactly where the bug was
| } else { | ||
| valueSpans[i] = core.NewSpan(span.Start.WithValueFlag(), span.End.WithValueFlag()) | ||
| // DocumentFetcher only ever recieves document keys | ||
| //nolint:forcetypeassert |
There was a problem hiding this comment.
suggestion: Have a must DataStoreKey function for the casting? and do this lint ignore there once for all these 4 casts?
There was a problem hiding this comment.
I do not think moving a simple cast into a function would improve the readability of the code here.
There was a problem hiding this comment.
Can also move span.Start.(keys.DataStoreKey) and span.End.(keys.DataStoreKey) out of if-else blocks into a variable, and then use that in their respective occurrences. Feel free to ignore as this seems like subjective pref
| @@ -41,7 +41,7 @@ type DataStoreKey struct { | |||
| FieldID string | |||
| } | |||
|
|
|||
| var _ Key = (*DataStoreKey)(nil) | |||
| var _ Walkable = (*DataStoreKey)(nil) | |||
There was a problem hiding this comment.
question: What inspired the name change, and the context for the name "walkable"? I am guessing it's the key that can be accessed.
EDIT: Was answered by documentation on Walkable ignore this question.
| // Walkable represents a key in the database that can be 'walked along' | ||
| // by prefixing the end of the key. | ||
| type Walkable interface { | ||
| Key | ||
| PrefixEnd() Walkable | ||
| } |
There was a problem hiding this comment.
praise: Answered my question in another comment, ignore the previous question please.
| @@ -142,13 +144,14 @@ func (n *dagScanNode) simpleExplain() (map[string]any, error) { | |||
|
|
|||
| // Build the explanation of the spans attribute. | |||
| spansExplainer := []map[string]any{} | |||
| undefinedHsKey := keys.HeadStoreKey{} | |||
There was a problem hiding this comment.
nitpick:
| undefinedHsKey := keys.HeadStoreKey{} | |
| undefinedHSKey := keys.HeadStoreKey{} |
There was a problem hiding this comment.
Headstore is one word, the current capitalization of HeadStoreKey is incorrect and has been fixed in the branchable collections branch (is bundled with some other branchable-specific changes, which is why it isnt in this PR).
Is not required, and actually seems to be buggy
Type alias is temporary, I'll remove the Spans type soon
There is only one implementation, and given the consuming code structure in the planner it is highly likely that there will only ever be one Spans implementation.
This is the primary goal of the PR, as otherwise I'd have to extend the hacks even further in order to query collection commits.
AndrewSisley
force-pushed
the
3209-rework-spans
branch
from
6496449 to
6eb7987
Compare
Relevant issue(s)
Resolves #3209
Description
Simplifies core.Spans a little bit, before removing some old hacky code allowing a nicer way of handling headstore keys within the planner/fetcher system.
Is probably easier to review this commit by commit, the changes are documented by the commit messages.