Skip to content
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(orm): add API for batching updates and deletes while iterating #11156

Closed
wants to merge 5 commits into from

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Feb 9, 2022

Description

Adds Update, Delete and Write methods to Iterator to allow batching updates and deletes while iterating. Updates and Deletes get applied after iteration is done using Write.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added the C:orm label Feb 9, 2022
@aaronc aaronc marked this pull request as ready for review February 10, 2022 18:20
@aaronc aaronc requested a review from alexanderbez as a code owner February 10, 2022 18:20
PK testpb.ExampleTable 4/-1/abc -> {"u32":4,"u64":16,"str":"abc","bz":"abc","i64":-1}
KEY 010000047fffffffffffffff616263 10102203616263
PK testpb.ExampleTable 4/-1/abc -> {"u32":4,"u64":16,"str":"abc","bz":"abc","i64":-1}
ORM UPDATE testpb.ExampleTable {"u32":4,"u64":16,"str":"abc","bz":"abc","i64":-1} -> {"u32":4,"u64":17,"str":"abc","bz":"abc","i64":-1}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fdymylja one question I have is about the ordering of calls to Hooks. You can see in the log here that the hook is called as soon as the write is batched, even though the actually SET and DELETE's are not applied later

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #11156 (08c7e56) into master (fa8099d) will increase coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11156      +/-   ##
==========================================
+ Coverage   65.76%   65.81%   +0.04%     
==========================================
  Files         654      693      +39     
  Lines       66985    70149    +3164     
==========================================
+ Hits        44052    46166    +2114     
- Misses      20389    21152     +763     
- Partials     2544     2831     +287     
Impacted Files Coverage Δ
orm/model/ormtable/table_impl.go 50.00% <ø> (ø)
orm/model/ormtable/index_impl.go 46.47% <33.33%> (ø)
orm/model/ormtable/unique.go 40.45% <50.00%> (ø)
orm/model/ormtable/iterator.go 66.49% <58.13%> (ø)
orm/model/ormtable/primary_key.go 52.42% <62.10%> (ø)
orm/model/ormtable/build.go 77.20% <100.00%> (ø)
x/distribution/simulation/operations.go 80.54% <0.00%> (-9.73%) ⬇️
crypto/keys/internal/ecdsa/privkey.go 82.45% <0.00%> (-1.76%) ⬇️
orm/encoding/ormkv/entry.go 77.77% <0.00%> (ø)
errors/errors.go 85.04% <0.00%> (ø)
... and 37 more

if hooks := writer.Hooks(); hooks != nil {
err := hooks.OnInsert(new)
if err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so hooks can stop the whole process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I am going to separate before and after hooks though, and will put this in draft and add that in a separate PR

@aaronc aaronc marked this pull request as draft February 11, 2022 15:29
@aaronc aaronc mentioned this pull request Feb 25, 2022
29 tasks
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tac0turtle
Copy link
Member

closing for time being, won't delete the branch so we can come back to it later

@tac0turtle tac0turtle closed this Jun 13, 2022
@robert-zaremba
Copy link
Collaborator

It's the PR ready for review? If yes then it's good to keep it open and call for reviews . I can do review if it's ready

@tac0turtle tac0turtle deleted the aaronc/orm-iterator-mutation branch February 13, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants