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

Introduce filter in Fixed/Variable sized Array types #2678

Merged
merged 7 commits into from
Aug 14, 2023

Conversation

darkdrag00nv2
Copy link
Contributor

@darkdrag00nv2 darkdrag00nv2 commented Jul 24, 2023

Work towards #2605

Description

Introduce filter function for creating a copy of an Array value with its entries filtered via predicate.

This function would be unavailable to resource arrays since resources cannot be copied

Will send docs PR post the merge.


  • Targeted PR against master 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

@darkdrag00nv2 darkdrag00nv2 changed the title Introduce filter in VariableSizedArray type Introduce filter in VariableSizedArray type Jul 24, 2023
Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

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

Please add some tests here to check that mutation in the array-filtering predicate function is properly prevented here.

@darkdrag00nv2
Copy link
Contributor Author

Please add some tests here to check that mutation in the array-filtering predicate function is properly prevented here.

@dsainati1 Didn't get it completely. Is it relevant also for non-resource arrays?

I based the implementation on forEachKey of Dictionary and we seem to be doing mutation prevention only for resource kinded values. I thought this won't apply here since we're not allowing filter on resource kinded arrays.

interpreter.withMutationPrevention(v.StorageID(), iterate)

@dsainati1
Copy link
Contributor

Ah good point, I missed that this was not implemented for resource arrays. That makes sense.

@SupunS
Copy link
Member

SupunS commented Jul 26, 2023

FixedSizedArrays since we don't know the size of the output array.

Can't we still make this function available for fixed-sized arrays? The return type of the function would always be variable-sized.

@darkdrag00nv2
Copy link
Contributor Author

@SupunS Yeah, sure. Can be done. I'll update the PR tomorrow.

@SupunS SupunS self-assigned this Jul 27, 2023
@SupunS SupunS added the Feature label Jul 27, 2023
@darkdrag00nv2 darkdrag00nv2 changed the title Introduce filter in VariableSizedArray type Introduce filter in Fixed/Variable sized Array types Jul 29, 2023
@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #2678 (9c2eb9d) into master (181924e) will increase coverage by 0.55%.
Report is 30 commits behind head on master.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
+ Coverage   78.57%   79.12%   +0.55%     
==========================================
  Files         338      333       -5     
  Lines       78216    78306      +90     
==========================================
+ Hits        61456    61962     +506     
+ Misses      14474    14047     -427     
- Partials     2286     2297      +11     
Flag Coverage Δ
unittests 79.12% <92.30%> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
runtime/interpreter/value.go 67.02% <87.83%> (-1.94%) ⬇️
runtime/sema/type.go 89.75% <100.00%> (+0.10%) ⬆️

... and 21 files with indirect coverage changes

@darkdrag00nv2
Copy link
Contributor Author

@SupunS This is ready for your review now.

runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/sema/type.go Outdated Show resolved Hide resolved
runtime/sema/type.go Show resolved Hide resolved
runtime/interpreter/value.go Show resolved Hide resolved
runtime/interpreter/value.go Outdated Show resolved Hide resolved
@darkdrag00nv2
Copy link
Contributor Author

@SupunS @turbolent Gentle ping for review

runtime/interpreter/value.go Outdated Show resolved Hide resolved
runtime/sema/type.go Show resolved Hide resolved
runtime/tests/interpreter/interpreter_test.go Outdated Show resolved Hide resolved
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Nice!

@SupunS SupunS merged commit af119ed into onflow:master Aug 14, 2023
12 of 14 checks passed
@darkdrag00nv2 darkdrag00nv2 deleted the filter_array branch August 16, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants