fix: change ReadFileExecute to handle reading from pipes #12128
Conversation
ReadFileExecute assumed that the size of the file could be determined then allocated a buffer and read the contents into that buffer. When using a pipe, the size of the input cannot be determined. Change the function to dynamically allocate a buffer to read from pipes up to the maximum size of the string.
| } | ||
| } while (read_bytes > 0); | ||
| content_string = | ||
| StringVector::AddStringOrBlob(file_content_vector, read_buffer.data(), total_bytes_read); |
There was a problem hiding this comment.
I wonder if we can do this incremental allocation directly on the StringVector, as that would save us from doing another allocation and copy of the data
But I'm not entirely sure that's feasible with the API right now
There was a problem hiding this comment.
Thanks for the PR, the changes make sense to me
I did find this condition to resize the buffer a bit curious
buffer_size - total_bytes_read < initial_buffer_size
If I understand correctly, if the remaining size is under 4096 we will grow the buffer, instead of letting it fill entirely and then growing.
I guess this means our reads will be more efficient though, as we are not having small (< 4096) reads if we are close to the end of the buffer
On another note, as we are very close to a release, it might make more sense to push this to the feature branch, especially as no tests are/can be(?) added for this feature
Yes you're right, I just thought 4k was a reasonable size to ensure that the buffer can always read at least a page on Linux.
Tests could be added, but there isn't a filesystem implementation that returns pipes right now. If you wanted to bring the shellfs extension into the main tree it the tests for it could test these changes. |
|
@rustyconover I believe reading from |
ReadFileExecute assumed that the size of the file could be determined then allocated a buffer and read the contents into that buffer. When using a pipe, the size of the input cannot be determined.
Change the function to dynamically allocate a buffer to read from pipes up to the maximum size of the string.
This change allows my new
shellfsfunction which returns FileHandles that are pipes to function with read_text() and read_blob().See the extension at:
https://github.com/rustyconover/duckdb-shellfs-extension