-
-
Notifications
You must be signed in to change notification settings - Fork 230
make sql.HashOf()
collation aware
#3027
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
…rver into james/insubq
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.
LGTM!
sql/hash/hash.go
Outdated
// TODO: this should use types.ExtendedType.SerializeValue, but there are some doltgres conversion issues | ||
// we need to address. Resort to old behavior for now. |
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.
I'd change this to say something more along the lines of:
Doltgres follows Postgres conventions which don't align with the expectations of MySQL, so we're using the old (probably incorrect) behavior for now
SerializeValue
isn't guaranteed to create comparably-unique values, which are different than truly unique values. For example, 1.0
and 1.00
are the same value via comparison, but they may be serialized differently depending on the type and whether the number of significant digits is important. In Postgres, we'd use the =
operator, but GMS/MySQL doesn't have a concept of bespoke equality functions like that, hence why this doesn't really work.
This PR adds type/collation information to
HashOf
.Additionally, it refactors
HashOf
to avoid import cycles and hasgroupingKey
use the function.fix for: dolthub/dolt#9049
doltgres fix: dolthub/doltgresql#1548