feat: Add support for views with Lens transforms #2311
Conversation
AndrewSisley
added
feature
AndrewSisley
force-pushed
the
2070-lens-views
branch
2 times, most recently
from
bf5d3ef
to
0c6e63c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2311 +/- ##
===========================================
- Coverage 74.12% 74.07% -0.05%
===========================================
Files 259 260 +1
Lines 25796 25946 +150
===========================================
+ Hits 19119 19217 +98
- Misses 5341 5383 +42
- Partials 1336 1346 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Mostly looks good, just the one main concern regarding the explain debug test similar to this comment when view node was added: #2114 (comment)
There was a problem hiding this comment.
todo: Since a new plan node is being added, I would make the same comment as: #2114 (comment)
tldr: Please add the new name to allPlanNodeNames variable in the tests/integration/explain.go file for debug node and add a test for the debug explain under the folder tests/integration/explain/debug similar to tests/integration/explain/debug/with_view_test.go. You don't have to make it explainable however in this PR, that discussion can be had elsewhere but would suggest opening an issue for it.
Edit: I crossed-out the suggestion of opening an explain issue, as @fredcarle has already made explain issue(s).
There was a problem hiding this comment.
- Add explain test stuff
Ahh cheers for making the explain issues. Are the issues for discussion if we need lens and view nodes, or we already decided that we need explain for them? |
AndrewSisley
force-pushed
the
2070-lens-views
branch
from
0c6e63c
to
aa21960
Compare
I don't think that they needing explain is a given, although I think the fact that we have this question and hide some nodes from our users is an unwanted complication for both us and them and plan on chatting about this in the standup (it came up in some of my other work). |
AndrewSisley
force-pushed
the
2070-lens-views
branch
from
aa21960
to
3bb134d
Compare
Cheers Fred :) |
| } | ||
|
|
||
| func (n *lensNode) Next() (bool, error) { | ||
| hasNext, err := n.output.Next() |
There was a problem hiding this comment.
nitpick: var with name hasValue would fit better here
| // the resultant field set. We cannot use [append] because the index of each field | ||
| // must still correspond to it's field ID. | ||
| originalProps := properties | ||
| properties = make([]any, fieldIndex+1) |
There was a problem hiding this comment.
suggestion: it will be more efficient to run through indexes to find the maximum value and then allocate properties. It can be also done before the loop.
Otherwise there might be scenarios when properties can be allocated several times.
There was a problem hiding this comment.
Which one is more efficient depends on the two sizes really. Not every indexes key will be present in the map and your suggestion will over-allocate when that happens.
Your suggestion is probably better more of the time, but I don't think it is worth a new PR.
## Relevant issue(s) Resolves sourcenetwork#2070 ## Description Adds support for views with Lens transforms.
Relevant issue(s)
Resolves #2070
Description
Adds support for views with Lens transforms.