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.

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

ARROW-4997: [C#] ArrowStreamReader doesn't consume whole stream and doesn't implement sync read. #4017

Closed
wants to merge 2 commits into from

Conversation

@eerhardt
Copy link
Contributor

@eerhardt eerhardt commented Mar 22, 2019

When reading from a network stream, ArrowStreamReader doesn't check how many bytes were read, and instead assumes the whole buffer was filled with a single read call. Fixing this to read multiple times to fill the whole buffer.

Also, when consuming a stream only the async read method works. The synchronous method throws NotImplementedException. Implementing the sync method.

Note: this implements a lot of the underlying functionality from #3925. The difference here is that this change doesn't attempt to solve the perf issues with allocating and copying memory multiples times. #3925 is specifically solving that perf issue.

@stephentoub @pgovind @chutchinson

…sync read.

When reading from a network stream, ArrowStreamReader doesn't check how many bytes were read, and instead assumes the whole buffer was filled with a single read call. Fixing this to read multiple times to fill the whole buffer.

Also, when consuming a stream only the async read method works. The synchronous method throws NotImplementedException. Implementing the sync method.
Make EnsureRead method not async.

Use ValueTask internally where appropriate.
kou
kou approved these changes Mar 22, 2019
Copy link
Member

@kou kou left a comment

+1
I'll merge this.

@kou kou closed this in 80cb11a Mar 22, 2019
@eerhardt eerhardt deleted the ArrowReaderBlockingIssues branch Mar 22, 2019
kszucs added a commit that referenced this issue Mar 24, 2019
…oesn't implement sync read.

When reading from a network stream, ArrowStreamReader doesn't check how many bytes were read, and instead assumes the whole buffer was filled with a single read call. Fixing this to read multiple times to fill the whole buffer.

Also, when consuming a stream only the async read method works. The synchronous method throws NotImplementedException. Implementing the sync method.

Note: this implements a lot of the underlying functionality from #3925. The difference here is that this change doesn't attempt to solve the perf issues with allocating and copying memory multiples times. #3925 is specifically solving that perf issue.

@stephentoub @pgovind @chutchinson

Author: Eric Erhardt <eric.erhardt@microsoft.com>

Closes #4017 from eerhardt/ArrowReaderBlockingIssues and squashes the following commits:

806a0d3 <Eric Erhardt> PR feedback
356a861 <Eric Erhardt> ArrowStreamReader doesn't consume whole stream and doesn't implement sync read.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants