feat: Add group by support for commits #887
Conversation
AndrewSisley
added
feature
| cid, hasCid := n.docMapper.DocumentMap().FirstOfName(n.currentValue, "cid").(*cid.Cid) | ||
| if hasCid { | ||
| // dagScanNode yields cids, but we want to yield strings | ||
| n.docMapper.DocumentMap().SetFirstOfName(&n.currentValue, "cid", cid.String()) |
There was a problem hiding this comment.
I was tempted to do this in the ToMap func (newish equivalent of old renderNode), for all Cids, but decided to keep this local for now. Let me know you you feel.
AndrewSisley
force-pushed
the
sisley/feat/I886-commit-groupby
branch
from
26ef2ce
to
3cada1b
Compare
Codecov Report
@@ Coverage Diff @@
## develop #887 +/- ##
===========================================
+ Coverage 59.75% 59.80% +0.05%
===========================================
Files 157 157
Lines 17420 17452 +32
===========================================
+ Hits 10409 10438 +29
- Misses 6069 6071 +2
- Partials 942 943 +1
|
query/graphql/parser/commit.go
Outdated
| fields := make([]string, 0) | ||
| for _, v := range obj.Values { | ||
| fields = append(fields, v.GetValue().(string)) | ||
| } |
There was a problem hiding this comment.
nitpick(non-blocking): Since we know the size, can allocate upfront and just reassign, rather than append. Non-blocking because is not a hot path.
There was a problem hiding this comment.
I also think the size should be allocated according to the length of obj.Values. Specially since the effort was made to use make.
There was a problem hiding this comment.
was copy-pasted, and authored according to the original author's taste at the time. really doesn't matter here (RE performance or readability) but I'll change it if I'm about
There was a problem hiding this comment.
I changed it as I had to rebase anyway (got its own commit, the message highlights the reasoning behind my choice)
The non-deterministic mapping complicates the group-by code when adding support for commits as children and parent mappings will not always match.
Was previously declared and unset and unused
Was sometimes string, sometimes cid and sometimes *cid. Now it is always *cid
Declaring it suggests importance, and wastes developer attention on irrelevent stuff
AndrewSisley
force-pushed
the
sisley/feat/I886-commit-groupby
branch
from
3cada1b
to
1a6371b
Compare
| @@ -131,7 +131,7 @@ func parseCommitSelect(field *ast.Field) (*CommitSelect, error) { | |||
| commit.Depth = client.Some(depth) | |||
| } else if prop == parserTypes.GroupByClause { | |||
| obj := argument.Value.(*ast.ListValue) | |||
| fields := make([]string, 0) | |||
| fields := []string{} | |||
There was a problem hiding this comment.
thought: That's a weird change lol. Why not set the length to len(obj.Values) if you decided to make a change?
There was a problem hiding this comment.
reason noted in the commit message
There was a problem hiding this comment.
Just saw. I don't always think of looking at the commit message.
There was a problem hiding this comment.
I tend to put a fair amount of info in those when needed (and when I am not too lazy) - I see this kind of thing as a major part of the (line 1+) commit message's reason-to-be. It is a very good place to note down the reasons for making changes IMO.
* Simplify walkAndFindPlanType * Make commit field mapping deterministic The non-deterministic mapping complicates the group-by code when adding support for commits as children and parent mappings will not always match. * Set commit node docMapper Was previously declared and unset and unused * Return consistent type from dagscan Was sometimes string, sometimes cid and sometimes *cid. Now it is always *cid * Add group by support for commits
Relevant issue(s)
Resolves #886
Description
Adds group by support for commits.