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

Add feature to support mutation for array and map iterators #359

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Nov 30, 2023

Closes #356 #357
Updates #292 onflow/flow-go#4633

Add support for mutable iterators, including support for atree splitting/merging during iteration from inlining, uninlining, and mutated values).

This feature was discussed on Discord:

Mutable iterator for array and map supports:

  • indirect element mutation, such as modifying nested container
  • direct element mutation, such as overwriting existing element with new element

Mutable iterator for array and map doesn't support:

  • inserting new elements into the array/map
  • removing existing elements from the array/map

NOTE: for better performance, use readonly iterator if mutation is not needed.

This PR:

  • adds new interfaces ArrayIterator and MapIterator
  • decouples implementation of mutable and readonly iterators
  • refactors related functions

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Mutable iterator for array and map supports:
- indirect element mutation, such as modifying nested container
- direct element mutation, such as overwriting existing element
  with new element

Mutable iterator for array and map doesn't support:
- inserting new elements into the array/map
- removing existing elements from the array/map

NOTE: use readonly iterator if mutation is not needed for
better performance.

This commit:
- adds new interfaces ArrayIterator and MapIterator
- decouples implementation of mutable and readonly iterators
- refactors related functions
@fxamacker fxamacker added enhancement New feature or request breaking change changes to API that can break programs using Atree's API labels Nov 30, 2023
@fxamacker fxamacker self-assigned this Nov 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

Attention: 175 lines in your changes are missing coverage. Please review.

Comparison is base (cb82995) 62.52% compared to head (5e67357) 62.45%.

Files Patch % Lines
map.go 61.06% 113 Missing and 33 partials ⚠️
array.go 67.04% 22 Missing and 7 partials ⚠️
Additional details and impacted files
@@                      Coverage Diff                       @@
##           feature/array-map-inlining     #359      +/-   ##
==============================================================
- Coverage                       62.52%   62.45%   -0.07%     
==============================================================
  Files                              15       15              
  Lines                           10599    10919     +320     
==============================================================
+ Hits                             6627     6820     +193     
- Misses                           3021     3119      +98     
- Partials                          951      980      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fxamacker fxamacker marked this pull request as draft December 1, 2023 21:40
@fxamacker fxamacker marked this pull request as ready for review December 2, 2023 21:53
Copy link
Contributor

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

Looks good to me

@turbolent
Copy link
Member

@fxamacker Do we still need this for onflow/cadence#2882?

@fxamacker
Copy link
Member Author

Do we still need this for onflow/cadence#2882?

@turbolent Yes, we need this for integration PR.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

LGTM! I mainly reviewed it from a Go perspective, as in general, I lack a bit of understanding of the inner working of Atree to fully review if the changes are correct or if there is something missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change changes to API that can break programs using Atree's API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants