-
Notifications
You must be signed in to change notification settings - Fork 40
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
Fix/large num chunks error #381
Fix/large num chunks error #381
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
===================================
Coverage 78% 78%
===================================
Files 34 34
Lines 5022 5031 +9
===================================
+ Hits 3925 3941 +16
+ Misses 1097 1090 -7 |
Hi @bhimrazy, Thanks a lot for taking this up! Work got really busy so I didn't get to implement a potential fix. I took a look at the current changes, I am not 100% confident I understand the changes fully, but it seems like we will close the mmap file after every item read (and then open it on every successive read because its not in the cache)? This is a bit excessive in terms of going the way of almost not caching anything right? Please let me know if I have misunderstood the implementation. Thanks again! Best, |
Yes, @fdalvi, you’re correct. Thanks for bringing this up! I’ll review the changes in more detail and make the necessary adjustments shortly. |
for more information, see https://pre-commit.ci
Before submitting
What does this PR do?
Fixes #366.
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃