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

let memoize cache null result values #2998

Merged
merged 6 commits into from
Mar 30, 2023
Merged

let memoize cache null result values #2998

merged 6 commits into from
Mar 30, 2023

Conversation

abendt
Copy link
Contributor

@abendt abendt commented Mar 27, 2023

the current implementation of memoize does not cache null result values.
In effect the memoized function will be called more than once for a given set of parameters, if the function returns null for that set.

this MR changed the cache in MemoizedHandler to ensure null values are also stored.

abendt and others added 2 commits March 27, 2023 10:03
@abendt
let memoize cache null result values
4546e6e 
the current implementation of memoize does not cache null result values. in effect the decorated function f is called multiple times
for a given set of parameters if f returns a null value.

this MR changed the cache in MemoizedHandler to ensure null values are also stored.
@abendt
Merge branch 'main' into gen_function_memo
029fb95
@serras
Copy link
Member

serras commented Mar 27, 2023

@abendt I don't fully understand why you need an additional Option here. I agree that the current approach is wrong, because cache.get()[k] returns null for both a non-existing key, and a key whose value is null. However, you can fix that problem by using containsKey, which returns true even if the value is null.

1 similar comment
@serras
Copy link
Member

serras commented Mar 27, 2023

@abendt I don't fully understand why you need an additional Option here. I agree that the current approach is wrong, because cache.get()[k] returns null for both a non-existing key, and a key whose value is null. However, you can fix that problem by using containsKey, which returns true even if the value is null.

@nomisRev
Copy link
Member

Yes, @serras is right. It would indeed by better to just us containsKey instead of nesting into Option.

@serras serras mentioned this pull request Mar 27, 2023
Closed
@abendt
Copy link
Contributor Author

abendt commented Mar 27, 2023

true, @serras i could have used containsKey

If i see it correctly your PR #3002 includes my changes right? so i will close this PR

@abendt abendt closed this Mar 27, 2023
@serras serras reopened this Mar 28, 2023
@serras
Copy link
Member

serras commented Mar 28, 2023

We were not confident of introducing a map in general in arrow-atomic (as done in #3002), so I've re-opened this issue and pushed the use of containsKey (in the form of in check) instead of get.

@serras serras requested review from nomisRev and a team March 28, 2023 11:38
abendt added 2 commits March 29, 2023 19:13
@abendt
Merge branch 'master' into gen_function_memo
a47d2a8 
# Conflicts:
#	arrow-libs/core/arrow-core/src/commonTest/kotlin/arrow/core/test/GeneratorsTest.kt
@abendt
merge latest main branch
d5945aa
nomisRev
nomisRev approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@nomisRev nomisRev 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. Thanks for the fix @abendt & @serras 🙏

@serras serras requested a review from a team March 30, 2023 09:20
@serras serras merged commit f7e41cd into arrow-kt:main Mar 30, 2023
@abendt abendt deleted the gen_function_memo branch March 30, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nomisRev nomisRev nomisRev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abendt @serras @nomisRev