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

perf: add read(b,o,l) to BlobInputStream #2376

Merged
merged 13 commits into from Jan 3, 2022

Conversation

davecramer
Copy link
Member

fixes #2374

@inhabitantNewCity
Copy link

Hi,
Do we have to support Limit in this case?

And also some strange Case:
Steps:

  1. call read() (get one byte)
  2. call read(b,o,l) where len > bytesToCopy

in such case lo.read(b, off, len); should be executed with offest = 0,
because previous buffer has been already read to buffer.

@davecramer
Copy link
Member Author

Hi, Do we have to support Limit in this case?

Unfortunately we have exposed this API so we really can't change it.

I will test your other question

if (len > 0 ) {
bytesCopied += lo.read(b, off, len);
absolutePosition += bytesCopied;
if ( bytesCopied == 0 && (buffer == null || bufferPosition >= buffer.length) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, how could the second half of this condition ever be false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, pretty sure that should have been buffer != null thx, for the second set of eyes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think bytesCopied == 0 is the only thing to check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure one of the tests failed, let me check again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testGetBinaryStreamWithBoundaries fails without the second test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the test actually right?
I am not seeing how to get here with data remaining in the buffer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I more or less copied it from the read() function. You may be right that it will never be exercised.

}
}
} catch (SQLException ex ) {
throw new IOException(ex.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why call toString on the causing exception?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants