feat: Make dockey optional for allCommits queries #847
Conversation
AndrewSisley
added
feature
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
from
f7100a3
to
4805a3f
Compare
Codecov Report
@@ Coverage Diff @@
## develop #847 +/- ##
===========================================
- Coverage 59.81% 59.81% -0.01%
===========================================
Files 157 157
Lines 17403 17425 +22
===========================================
+ Hits 10410 10422 +12
- Misses 6056 6066 +10
Partials 937 937
|
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
added
the
action/no-benchmark
|
I'm not actually 100% sure we want this as a feature :/. |
We discussed this a while ago: #549 https://discord.com/channels/427944769851752448/988568170677796905 EDIT: To expand, two points to consider:
|
Apologies, I understood the sentiment of that thread at the time as unifying the commits queries under a single query type, not that we would also support querying for commits across documents via optional dockey selector/filter. I'm just thinking through the implications of this change/feature. As technically there is very little coupling between commit data across documents, want to make sure that we don't imply there is a stronger link between them then there is. For Branchable Collections, this makes sense, but as that would just technically be a re-use of the compositeDAG system, it would already be supported by the current commit system. Give me a minute to think through this. Apologies! |
| Results: []map[string]any{ | ||
| { | ||
| "cid": "bafybeidst2mzxhdoh4ayjdjoh4vibo7vwnuoxk3xgyk5mzmep55jklni2a", | ||
| }, | ||
| { | ||
| "cid": "bafybeihhypcsqt7blkrqtcmpl43eo3yunrog5pchox5naji6hisdme4swm", | ||
| }, | ||
| { | ||
| "cid": "bafybeid57gpbwi4i6bg7g357vwwyzsmr4bjo22rmhoxrwqvdxlqxcgaqvu", | ||
| }, |
There was a problem hiding this comment.
thought: I feel like this is a litle confusing. The CIDs are for the document, Name and Age right? Name and Age are ususally under links of the document commits. If we have allCommits without dockeys, I would expect to just have the list of CIDs for documents (and their links). Not their fields as well in the same level list. But as I write this I'm thinking it's also very confusing to see a list of CIDs of documents (certainly multiple CIDs for the same document) belonging to different collections.
There was a problem hiding this comment.
yeah there is a ticket to expose dockey here #846 and we should also add one for collection (will create ticket)
For the record John and I chatted on discord and we are happy adding this feature |
I actually didn't even think of the possibility of field and doc level being intermixed, let alone across collections. Makes me feel uneasy about this again :/. |
|
Similar to docs, it is trivial to sort or group by those if the user wants |
|
Or filter by them |
|
Has there been a final decision on if we want this feature? |
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
5 times, most recently
from
d4794de
to
fe07cc3
Compare
|
At the moment, if you don't specify field, it assumes a Field ID of |
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
from
fe07cc3
to
19319e6
Compare
Only true if you specify a dockey, otherwise it will yield field commits. I'll fix the dockey behaviour here |
|
I struggle to understand why we would want to dump all CIDs like this. In what case is this useful? Why provide a feature to our users that (to me at least) provides an ambiguous output. |
Auditing/compliance would be the most obvious use-case for this on it's own. But also api/query consistency (commits should work exactly like any other query IMO, and allowing that makes it much easier for users to learn I think), and the extras (field,dockey, collection fields, and group, join etc can be added later - doing everything in one PR would be an awful way of working). And then as noted somewhere earlier forcing users to specific a dockey and a cid is totally pointless and will just be a nuisance for them). |
I guess it brings consistency but using |
Why not? It could be very handy if they want a cross-object history.
I very strongly believe in this, minus the really big red lines such as corrupt the db/security-stuff/etc IMO the users should be free to use Defra however they think is most useful, and to actively block them to doing so is somewhat arrogant of us |
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
4 times, most recently
from
bfa8801
to
0669b5f
Compare
Dockey stuff removed in commit 'Remove dockey commit returning only composite commit magic' |
|
To add some extra context, as people seem to have been caught off-guard a bit with this, the main goals of the Epic are to make the commit query(s) consistent with the object queries, preferably to the point at which the two are indistinguishable. The sub-tasks can be roughly lumped into the below categories:
IMO the less different the commits queries are to normal queries, the less unusual and complex they will seem. The system will be more intuitive to our users and more powerful. |
query/graphql/planner/dagscan.go
Outdated
| @@ -56,6 +57,7 @@ type headsetScanNode struct { | |||
| key core.DataStoreKey | |||
|
|
|||
| spans core.Spans | |||
| fieldId client.Option[string] | |||
There was a problem hiding this comment.
question: not sure I understand why we would want fieldId when parsed.FieldName is the same Option value. The only advantage I see is that instead of h.parsed.FieldName we can have a shorter version which is h.fieldId. My question is, do you think it's worth the duplication?
There was a problem hiding this comment.
This was done very deliberately (I think it got its own commit), as it was impossible to distinguish from a user requested field and default. Memory is very hazy here now though
There was a problem hiding this comment.
It's set in HeadScan and used only in initScan. Not sure where that ambiguity would come from. Can you clarify?
There was a problem hiding this comment.
I'll try get back to you on this, I wrote this a very long time ago and I'd need to re-read/learn the PR again in order to give you a better answer than the above
There was a problem hiding this comment.
Ah I see, I think the the prop's reason-to-be got removed at somepoint during dev, but the prop stayed. Has been removed.
query/graphql/planner/dagscan.go
Outdated
| cid *cid.Cid | ||
| currentCid *cid.Cid |
There was a problem hiding this comment.
suggestion: A short comment here that can make clear what's the difference between cid and currentCid would be appreciated. When I look at it I think cid == root cid and currentCid == node currently being scanned. But I may be wrong and it's not obvious.
There was a problem hiding this comment.
yes, this is not obvious and the code in this file could be simplified. There is a ticket for this in the epic (to be done after/as the query-syntax gets refactored)
There was a problem hiding this comment.
Since this field was added in this PR I think it would be appropriate to have the comment added now though.
There was a problem hiding this comment.
who would that comment target? This code is unlikely to be read by anyone besides myself.
Will add though if I end up modifying this PR before merge though
- add comment if here anyway
There was a problem hiding this comment.
it would target future you or someone else eventually needing to work on that code.
There was a problem hiding this comment.
The ticket mentioned is likely to be completed within the next month, probably within a shorter time-span than the span between (original) authoring and now.
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
from
1dcd77e
to
3798783
Compare
|
Note: In the future, it would be good to be able to use actual field names instead of IDs. Obviously this was a symptom of the functionality before this PR, that is just re-used here. But that can be a seperate issue. |
Yeah there are existing tests that document this behaviour, would for sure be good to sort that out |
Is inconsistent and likely to supprise users. If they only want the composite commits they can specify so.
AndrewSisley
force-pushed
the
sisley/feat/I844-allcommits-dockey-optional
branch
from
3798783
to
25b7271
Compare
* Make dockey optional for all commits * Remove dockey commit returning only composite commit magic
Relevant issue(s)
Resolves #844
Description
Makes dockey optional for allCommits queries.
Specify the platform(s) on which this was tested: