return BLOB columns as Buffers instead of strings #124

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

You rejected my previous request because I didn't add a test.

So now, I'm back, with a test :)

You were quite right to reject my previous patch, and I would have added a test previously if I had realized there was a test suite. Mea culpa.

My previous patch failed some of the already existing tests, even.

This new patch is a little more 'intrusive' to the code. I reshaped the way the ROW_PACKET_DATA buffers are handled. But, everything works for me locally, the tests now pass, and I added a new test to cover the BLOB functionality.

Collaborator

felixge commented Sep 23, 2011

Thanks, this looks good! I'll need a few days to review / land this so, things are crazy busy right now.

@ghost

ghost commented Oct 1, 2011

Running into a problem with blobs and buffers and strings, oh my. Very happy to see that the wheels are already in motion!

What's the status of this? I dropped the query.js file directly into my app and it works great!

Contributor

mscdex commented Jan 29, 2012

Recreating the buffer on every chunk is inefficient. The total size is already known when the first chunk comes in (buffer.length + remaining). See this pull request: felixge#52

My request doesn't recreate the buffer on every chunk... It stores the chunk buffers in a list and then coalesces them at the end.

I agree that the way felixge#52 grabs the data looks nicer, I didn't know about the (buffer.length + remaining) invariant.

But, I doubt the commit as a whole can possibly be correct. I didn't try it out, but it looks like it's not checking for BINARY_FLAG (which I had to add a constant for), which would mean that it would return Buffers for TEXT fields as well as BLOB fields.

Contributor

mscdex commented Jan 29, 2012

You're right, it doesn't check for BINARY_FLAG, but it does restrict the field type to blobs only and should not affect any other fields (including TEXT).

I'm not a mysql protocol expert, so I could be wrong, but I think that there is no difference between BLOB and TEXT types except for the BINARY_FLAG.

For example, once you've identified field.type === FIELD_TYPE_MEDIUM_BLOB you must then check for the field.flags & BINARY_FLAG to see if it's "really" a BLOB or if it's actually "MEDIUM_TEXT" (which doesn't have a constant of its own).

I reached this conclusion when writing tests for my commit (I notice felixge#52 has no added tests).

I used http://forge.mysql.com/wiki/MySQL_Internals_ClientServer_Protocol for reference... If this reference is incomplete and there are actual constants for MEDIUM_TEXT etc, then I concluded wrongly.

The test I wrote, "BLOB fields returned as Buffers" also exercises a text field. If you would run that against pull52, I suspect it would fail. If it doesn't, then I'll be quiet :)

Collaborator

felixge commented May 15, 2012

@felixge felixge closed this May 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment