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

SQL value more efficient at bytes #20

Closed
japettyjohn opened this issue Sep 13, 2017 · 9 comments
Closed

SQL value more efficient at bytes #20

japettyjohn opened this issue Sep 13, 2017 · 9 comments

Comments

@japettyjohn
Copy link

The SQL Value() gives a string representation of UUID, if this is unindexed data this may make little difference in the performance of the data but where this is either an index or even the PK this not efficient storage. Here's one of many articles on it.

This could either have a breaking change and make this provide a []byte type. If there is some more dominant SQL DB being targeted which does not benefit from this, then we could minimally have a BytesValuer() function to return a Valuer which does do this.

@ricardofandrade
Copy link

I have an actual issue with the fact String() is used: I support multiple databases and one of them only takes []byte for UUID.
In the interest of maintaining backwards compatibility, would a patch introducing a global option (yeah, I know...) be accept to return either String() or byte[]?
Something along the lines of uuid.ForceBinaryOutputForSQL(true)

@pborman
Copy link
Collaborator

pborman commented Aug 28, 2018

A global option would be bad. It would affect every package's use. You never know who might be depending on this. I will admit I am not a database person and this function was implemented by someone who knew more about databases than me. It seems like maybe a new type is needed that embeds a UUID, though it would break the ability to directly access values:

// A UUID Bytes is a UUID whose sql Value function returns a byte slice rather than a string.
type UUIDBytes struct {
    UUID
}

func (uuid UUIDBytes) Value() (driver.Value, error) {
        return uuid[:], nil
}

@ricardofandrade
Copy link

@pborman thanks for the reply. Would you propose this new wrapper type to be part of uuid?

@pborman
Copy link
Collaborator

pborman commented Aug 29, 2018

It actually does not need to be part of the UUID package at all. Before adding it to the package it would be good to get some feedback on how easy/hard such a thing would be to use. I think if you never sliced the uuid then you would have no problem. You might also want:

func (uuid UUIDBytes) Bytes() []byte {
    return uuid.UUID[:]
}

@ricardofandrade
Copy link

I am doing some experimentation currently. I'll report my findings here.
Not sure in which context slicing an UUID prior to storing it to the database would make any sense.

@pborman
Copy link
Collaborator

pborman commented Aug 29, 2018

It would be more for passing the UUID to something other than a database function, though I guess the code could simply say id.UUID[:] rather than id.Bytes() to get the byte slice.

@ghost
Copy link

ghost commented Sep 2, 2018

Bytes() seems to be pretty common in the Go ecosystem - i'll cast a gentle vote in favor of the above as well

@ricardofandrade
Copy link

ricardofandrade commented Sep 4, 2018

I held a similar discussion over gofrs/uuid#48 and a lot has been taken into account. The resulting decision was to not take any action since no solution seems adequate to be adopted long term by a UUID package.
If you all agree with considerations taken over there, feel free to close this issue.

@pborman
Copy link
Collaborator

pborman commented Feb 14, 2019

Thanks Ricardo, I think I will close this.

@pborman pborman closed this as completed Feb 14, 2019
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

No branches or pull requests

3 participants