Add grain_index_storage_option - #4916
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option, grain_index_storage_option, to control the index storage of the ArrayRecord reader (supporting in_memory or offloaded). However, two critical issues were identified in the review: first, passing a dictionary directly as reader_options to grain.ArrayRecordDataSource will cause a runtime TypeError because the underlying C++ implementation expects an ArrayRecordReaderOptions object; second, the unit test uses pytest.mark.parametrize on a class inheriting from unittest.TestCase, which is unsupported and will prevent the parametrized tests from running correctly. Both issues should be addressed using the provided code suggestions.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a65d654 to
9a2d6c7
Compare
Description
Grain reader by default uses "in_memory" to store index, this is for the best performance. But when the dataset has many shards, the index may take too much memory. The "offload" option saves index on disk, I added comment on potential perf impact. This solve customer issue previously in b/422533532.
Tests
Tested on single VM for perf impact
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.