fix: always wait_for_io to prevent crash when rows are scheduled but not loaded in decoder #5341
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently, my collaborator was testing the Lance file format and encountered a crash with the following error message:
After some investigation, I found that this should be a bug.
The test used both LanceFileVersion
V2_0andV2_1. This issue occurred withV2_0. If data is written in theV2_0format, there is a chance this bug will be triggered, even though the reading comes from the current latest Lance reader. Considering thatV2_1was only stable after v0.38.0, this bug should be worth fixing.The cause of this bug is in the
next_batch_taskfunction inrust/lance-encoding/src/decoder.rs, whererows_scheduledonly represents the scheduled length, not the length of completed I/O. When a piece of data is only scheduled but has not completed I/O, the above error occurs.The specific flow of how the bug occurs is as follows:
BatchDecodeIteratorcallsnext_batch_taskfor the first time, at which pointrows_scheduledis initially zero, entering thescheduled_need > 0branch.wait_for_io,rows_scheduledgets updated and synchronously waits for the data needed (to_take). Note there is a key difference here.rows_scheduledis the scheduled length, but only waits forto_takelength of data.rows_scheduledmay far exceedsto_take, and the data exceedingto_takemay not have completed I/O yet.next_batch_taskagain,rows_scheduledmay already be a large value. Ifto_takeis small, it may miss thescheduled_need > 0branch, completely skippingwait_for_io.drain_batch, it crashes.The fix for this bug is also in the
next_batch_taskfunction. The logic is to performwait_for_ioon theelsebranch. If the data is actually ready,wait_for_ioshould not cause new I/O or context switching, thus having almost no negative impact.I have added a new UT
test_blocking_take_with_many_rowsinrust/lance-file/src/reader.rs. When the version isV2_0, this bug can be reproduced without this fix.