Skip to content

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

Merged
merged 22 commits into from
Jun 18, 2025
Merged

make sql.HashOf() collation aware #3027

merged 22 commits into from
Jun 18, 2025

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Jun 11, 2025

This PR adds type/collation information to HashOf.
Additionally, it refactors HashOf to avoid import cycles and has groupingKey use the function.

fix for: dolthub/dolt#9049
doltgres fix: dolthub/doltgresql#1548

@jycor jycor requested a review from Hydrocharged June 16, 2025 22:59
Copy link
Contributor

@Hydrocharged Hydrocharged left a 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
Comment on lines 63 to 64
// TODO: this should use types.ExtendedType.SerializeValue, but there are some doltgres conversion issues
// we need to address. Resort to old behavior for now.
Copy link
Contributor

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.

@jycor jycor merged commit 4810ae9 into main Jun 18, 2025
8 checks passed
@jycor jycor deleted the james/insubq branch June 18, 2025 06:02
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

Successfully merging this pull request may close these issues.

2 participants