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.

Sign up for GitHub

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

Jump to bottom

Introduce filter in Fixed/Variable sized Array types #2678

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Introduce filter in Fixed/Variable sized Array types #2678

merged 7 commits into from
Aug 14, 2023

Conversation

darkdrag00nv2
Copy link
Contributor

@darkdrag00nv2 darkdrag00nv2 commented Jul 24, 2023 •
edited
Loading

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
dsainati1
dsainati1 reviewed Jul 24, 2023
View reviewed changes
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.

cadence/runtime/interpreter/value.go

Line 17038 in 181924e

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.

@darkdrag00nv2 darkdrag00nv2 requested a review from dsainati1 July 25, 2023 20:53
@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 •
edited
Loading

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.

turbolent
turbolent reviewed Jul 31, 2023
View reviewed changes
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
@darkdrag00nv2 darkdrag00nv2 requested a review from turbolent July 31, 2023 19:11
SupunS
SupunS reviewed Aug 2, 2023
View reviewed changes
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

SupunS
SupunS reviewed Aug 11, 2023
View reviewed changes
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
@darkdrag00nv2 darkdrag00nv2 requested a review from SupunS August 12, 2023 14:27
SupunS
SupunS approved these changes Aug 14, 2023
View reviewed changes
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
@darkdrag00nv2 darkdrag00nv2 deleted the filter_array branch August 16, 2023 16:42
@darkdrag00nv2 darkdrag00nv2 mentioned this pull request Sep 14, 2023
Merged
@turbolent turbolent mentioned this pull request Oct 18, 2023
Closed
7 tasks
@SupunS SupunS mentioned this pull request Oct 19, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SupunS SupunS SupunS approved these changes

@turbolent turbolent Awaiting requested review from turbolent turbolent is a code owner

@dsainati1 dsainati1 Awaiting requested review from dsainati1

Assignees

@SupunS SupunS

Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@darkdrag00nv2 @dsainati1 @SupunS @turbolent