search

dgraph-io / ristretto

A high performance memory-bound Go cache
https://dgraph.io/blog/post/introducing-ristretto-high-perf-go-cache/
Apache License 2.0
5.54k stars 364 forks source link

[BUG]: Can't use Google's uuid.UUID (a byte array rather than slice) as a key #370

Closed pete-woods closed 8 months ago

pete-woods commented 8 months ago

What version of Ristretto are you using?

e7380b4f3a4387dfa8d1223966a66f636422bf5f

What version of Go are you using?

1.21.5

Have you tried reproducing the issue with the latest release?

None

What is the hardware spec (RAM, CPU, OS)?

N/A

What steps will reproduce the bug?

Try using a uuid.UUID from Google's UUID library as the key. You get an error about it not being a valid type, despite being a byte array under the hood. If you cast the uuid to a byte slice like c.Set(myUUID[:]) it works.

It seems like the right type should relatively easily be addable here: https://github.com/dgraph-io/ristretto/blob/main/z/z.go#L43

Expected behavior and actual result.

No response

Additional information

No response

matthewmcneely commented 8 months ago

This is probably by-design. Adding support for types not part of Go's "Basic types" would result in a pretty long switch statement, and add unneeded dependencies from hundreds of packages. The c.Set(id[:]) operation you describe is idiomatic in Ristretto.

pete-woods commented 8 months ago

I guess I thought a byte array was the same kind of "basic type" as byte slice.

matthewmcneely commented 8 months ago

Yeah, arrays and slices are distinct types in Go. The type UUID [16][byte that Google's lib defines is of the former type.

I'm closing this, if you think it's still a problem you can reopen.

pete-woods commented 8 months ago

I was trying to say that byte arrays seem just as valid a "basic type" as a byte slice, not that they're literally the same type.

At any rate, if you're set on not adding byte arrays to that func, instead I think it'd improve the developer experience a fair bit to update the restriction on your generic type for key to limit to the set of types that are actually supported.

At the minute it's "any", which will happily accept a whole bunch of types you don't support. Then of course you get panics, which could have been caught at compile time.

Another alternative could be to add support for types implementing encoding.BinaryMarshaler (which many value types like Google's UUID) do.

pete-woods commented 8 months ago

Something like this: https://github.com/dgraph-io/ristretto/pull/371

matthewmcneely commented 8 months ago

I agree that would be an improvement. Feel free to submit a PR!