Skip to content

Conversation

@tvincentNuoDB
Copy link
Contributor

Decimal sequences longer than 9 bytes are now handled by putScaledCount2, before they were sent under the decimal protocol which would cause an invalid utf-8 sequence upon receiving. A few minor Python 3 fixes to putting data were made as well

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 8b50611 on tvincentNuoDB:master into 53ef0ce on nuodb:master.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. Let's talk more about it tomorrow. valueStr is a string of digits right? So we are sending edsScaledCount2 for a string longer than 9? Normally it takes I think 20 digits to need an edsScaledCount2. Maybe I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, it would seem the issue is the way python handles sockets as byte strings are the only thing a socket can send. Before they used

     chr(protocol.SCALEDLEN0 + len(valueStr))

This starts us off with the base value of ScaledLen0 and you will add 1 for each character present in the string. The issue is they never checked overflow with ScaledLen8. So if you put something 10 numbers long or longer, it will overflow into the next protocol. I'm not sure if this is the best solution for sending numeric values in the first place

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 0217e00 on tvincentNuoDB:master into 53ef0ce on nuodb:master.

mjmichaels added a commit that referenced this pull request Jun 12, 2015
Fixed invalid UTF-8 sequence in tests -- all tests passing
@mjmichaels mjmichaels merged commit 846c6ab into nuodb:master Jun 12, 2015
@tvincentNuoDB
Copy link
Contributor Author

Fixes issue 65

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants