-
Notifications
You must be signed in to change notification settings - Fork 44
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: Add document delete mechanics #1263
Conversation
planner/select.go
Outdated
@@ -307,6 +308,14 @@ func (n *selectNode) initFields(selectReq *mapper.Select) ([]aggregateNode, erro | |||
//nolint:errcheck | |||
n.addTypeIndexJoin(f) // @TODO: ISSUE#158 |
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.
way out of scope, but I just noticed this is not linked to an open ticket, but the one partially responsible for its introduction. We need to open a ticket for this.
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.
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.
Got a few items that I think should be discussed before merge.
I'm sorry as I feel like you checked in with me on this quite early, I misunderstood you then without realising and now it is needed to be talked about within the PR.
I haven't fully reviewed the internals/tests for this either, I'll go over them once we've all agreed on the high level discussion items.
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.
Some issues/thoughts.
Haven't gone through everything yet, but I need to go to sleep :D
Ill ping when im up and we can keep progressing.
eef6249
to
40765a0
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1263 +/- ##
===========================================
- Coverage 70.27% 70.22% -0.05%
===========================================
Files 184 184
Lines 17700 17791 +91
===========================================
+ Hits 12438 12494 +56
- Misses 4324 4346 +22
- Partials 938 951 +13
|
c5d1254
to
f77bb70
Compare
f77bb70
to
ef651b7
Compare
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.
Really great stuff! Super excited to see this land.
Small number of todos/suggestions, few more questions/thoughts.
Going to approve now, so you dont need to play another round of reviewer ping pong, but do go through the various issues 👍
68b4fd1
to
9bdac20
Compare
5063e33
to
da7d94f
Compare
This PR covers deleting (not purging) of documents. It adds a status value to the composite block and allows the merkle DAG to keep track of deleted documents. It also moves the fields from value instances to deleted instances to easily filter deleted documents and avoid extra overhead from the deleted documents on normal queries. We can thus select to show deleted documents when querying and are able to show the status of the document (_isDeleted: true/false) in the response.
This PR covers deleting (not purging) of documents. It adds a status value to the composite block and allows the merkle DAG to keep track of deleted documents. It also moves the fields from value instances to deleted instances to easily filter deleted documents and avoid extra overhead from the deleted documents on normal queries. We can thus select to show deleted documents when querying and are able to show the status of the document (_isDeleted: true/false) in the response.
Relevant issue(s)
Resolves #1262
Resolves #867
Description
This PR covers deleting (not purging) of documents. It adds a
status
value to the composite block and allows the merkle DAG to keep track of deleted documents. It also moves the fields from value instances to deleted instances to easily filter deleted documents and avoid extra overhead from the deleted documents on normal queries. We can thus select to show deleted documents when querying and are able to show the status of the document (_isDeleted: true/false
) in the response.Tasks
How has this been tested?
(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.
Specify the platform(s) on which this was tested: