Conversation
This change defines the new types, how they are derived from MySQL, and tests to make sure the conversions work.
So far, unit tests are passing. Still need to look at clients.
Tiny, Long, etc. are not very understandable. I've now changed the types to intXX, which is more intuitive.
There was a problem hiding this comment.
It took me a long time to understand what you're doing here... ;-).
Can you add a comment describing your method for converting MySQL types to a Vitess Type? (It would be helpful to mention that you first convert to a base type and then modify based on mysql flags)
Also, don't you need to shift by 2 bytes here? Seems your encoding of vitess types takes 2 bytes.
There was a problem hiding this comment.
Yeah. Playing with bits is always unreadable. These flags are actually used only against mysql types, and their max val is 255. So, we're safe.
I'm basically trying to build an "a.b" key using field & flags. This is to avoid a 2-level map. I think it's worth the efficiency because this is low level code.
I'll add better comments.
There was a problem hiding this comment.
But you're also combining these flags with vitess types which are more than 1 byte long in lines 95--104.
By the way, the explanation that you wrote in your answer to my comment would make a great source code comment ;-).
There was a problem hiding this comment.
Oops! Good catch. I'll work on a fix.
I've done enough to get the existing unit tests to pass, but I think they're insufficient. There also a few TODOs sprayed around.
|
Bind vars are now done. I've done enough to get the unit tests passing, but more work is needed. |
Float -> Float32 Double -> Float64 IsNumber -> IsIntegral Also addressed some code review comments.
|
All my parts for this branch are done: I've converted all protos to the new vitess type system.
|
|
LGTM for the proto definitions. Using an enum that is a bit field but has discrete values is a great idea for this. I only look over the code quickly. I agree we should submit this ASAP. Let's sync up at our team meeting, and see how we can finish this up quickly. |
|
LGTM. I'd like to gate the mapess code using the new C++ client behind a flag, so I could submit it and we can wait with its activation in prod. So if we're willing to tolerate the open source clients not working for a few days, we can submit now. |
|
It's a good idea to submit this PR. Maybe we can temporarily disable the Java and PHP tests till they're fixed. That way, we can stay on top of possible new breakages. |
|
Submitting this PR would break our import to google3, and I'm currently Cheers, Erez. On Mon, Oct 26, 2015 at 11:02 PM, sougou notifications@github.com wrote:
|
|
@sougou You can disable both tests in https://github.com/youtube/vitess/blob/master/test/config.json by setting "Manual" to "True". As we discussed yesterday, disabling the tests for several days in open-source is fine. |
|
I'm starting work on the PHP client now. @erzel if you become ready to import before I'm done, feel free to merge. If not, I'll add my commits to this PR branch. |
|
PHP is fixed. I'm going to start fixing the Java client now since no one else has claimed it. |
|
The google3 CL is out and awaiting review, hopefully I'll be able to submit it tomorrow. Meanwhile, I'll work on changing the C++ client to the new proto3 structure. |
|
Java client fix is ready for review. |
|
Looks good functionally, and thanks for the Rows cleanup. |
5 participants
WIP: DO NOT SUBMIT
This is to allow people to start reviewing work done so far: