-
-
Notifications
You must be signed in to change notification settings - Fork 234
Better types compliance #33
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is looking good, nothing major to comment on. All these definitions look pretty reasonable. Get these merged back to ld-master as soon as you can -- don't wait to implement any additional types.
Watch your file name convention: use snake_case.go instead of CamelCase.go
|
||
// Convert implements Type interface. | ||
func (t bitType) Convert(v interface{}) (interface{}, error) { | ||
value, err := cast.ToUint64E(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that this library silently converts nil to zero. This doesn't work in a lot of contexts (e.g. 0 in (null)
returns true in some contexts). Nothing to do at the moment, just keep this in mind going forward. We want to be super specific about nullness.
@zachmu My 2nd commit has all of the changes since your first review. |
Signed-off-by: Daylon Wilkins <[email protected]>
Signed-off-by: Daylon Wilkins <[email protected]>
22de54a
to
c56c974
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, especially for exhaustive detail in tests. Lots of small comments, nothing too major.
The main thing I want to see, either before you merge this or immediately afterward, is that you run the dolt sqllogictests and verify that this doesn't cause a regression. I don't see anything that indicates it will, but this is a big change to a fundamental part of the package.
Signed-off-by: Daylon Wilkins <[email protected]>
First off, I'm going to list the types that are missing from this PR:
The first three mentioned are the last ones I'm going to implement for now, as the
GEOMETRY
types are very, very complex. However,DECIMAL
proved to be a bit tougher than expected, and I didn't want to delay getting this PR out since I'll be gone over the entire Thanksgiving holiday (technically I'm on it now but I promised a PR by the 25th). Besides the ones listed above, every other type is in this PR in some capacity (NCHAR
and friends are missing, but that type can be replicated as it's just a charset/collation shortcut).Some notes, because they're important. There are no tests. Changing the types has broken a fair amount, and I haven't bothered to fix those things yet before I can get started writing tests.
I'm making use of higher-level interfaces that are specific to some type or group of types. I played around with the idea of adding a new function to the
Type
interface that returned an attribute map, but decided against it since I wanted to be able to add more than raw attributes, and a map of functions just seemed too complex for the tradeoff.Character sets and collations are complete, in that every single supported one in MySQL is here...which is why there are so many. I had many ideas on how to model this, but decided on using string constants as the base so that it's easy to reference them from outside code, and then have multiple maps for each specific attribute of a charset/collation. For example, if an implementer wants to specifically support the
hebrew
character set, then they can just referenceCharacterSet_hebrew
, rather than defining their own variable withcustom_var, err := ParseCharacterSet("hebrew")
.JSON is actually a very complex type, so I didn't really change anything from the logic that was already present before. This does mean that we don't truly support
JSON
, but implementing it right may take 1-2 full weeks, which I don't feel is worth it right now.It pains me to say, but in the effort of full compliancy, I am modeling a
BOOLEAN
as anint8
. But that's how MySQL does it, so it's probably for the best.You'll notice that some types have global variables while others don't, and that's because some types don't make sense to reference without other information.
Int8
can be a global type because it is fully self-contained in its information.Varchar
, OTOH, requires a length to be meaningful.On the topic of the string types, if any logic looks weird, then it's to mimic something that MySQL is doing. I did a lot of testing to see what MySQL actually did when the documentation wasn't clear, so some rules seem inconsistent by MySQL is apparently inconsistent. I'm also doing nothing with the character set and collation stuff regarding comparisons, so we just support
utf8mb4
as a character set (which mimics Go's string type implementation to the best of my knowledge), and changing it to something else doesn't change any actual behavior. That can come later because that's A LOT to do...The
TIME
type is weird, in that the minimum precision is microseconds, so I chose to base the entire type around that. All of the parseable variations are present as well. I was initially going to usetime.Duration
from Go, but it actually didn't suit the requirements. Working with it turned out to be more difficult than just using microseconds. For example, the only way to make a duration is to parse a string of a specific format, which would be parsing a string, then recreating a different string to parse, which is too much.YEAR
is the most useless type I've ever encountered in anything, but hey it's included and it's fully supported now, hooray...