-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Explain cid
& field
attributes for dagScanNode
#598
feat: Explain cid
& field
attributes for dagScanNode
#598
Conversation
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
explainerMap["cid"] = nil | ||
} | ||
|
||
// Build the explaination of the spans attribute. | ||
spansExplainer := []map[string]interface{}{} | ||
// Note: n.headset is `nil` for single commit selection query, so must check for it. | ||
if n.headset != nil && n.headset.spans.HasValue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: For the future (not) now, it might be worth refactoring explain-spans at some point, as it would be good to help encourage consistency here across all nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Perhaps we can initiate a ticket to put any specific kind of refactor you might have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you prefer, I probably wound't bother with a ticket and just do it as the first commit in the next explain PR
…work#598) ## Relevant issue(s) Resolves sourcenetwork#597 ## Description Adds two more attributes to be included in the explain graph for `dagScanNode`.
Relevant issue(s)
Resolves #597
Description
Adds two more attributes to be included in the explain graph for
dagScanNode
.Tasks
How has this been tested?
CI and
make test
(updated integration tests.)Specify the platform(s) on which this was tested: