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

fix: deterministic index ordering when migrating #7208

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bamo
Copy link

@bamo bamo commented Sep 27, 2024

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Change ParseIndexes to return a list of indices rather than a map, so the callers will iterate in deterministic order. This means when re-creating a database from the same schema, indexes will be created in a consistent order.

User Case Description

Issue: We observed that, when creating a database based on the same gORM schema multiple times, indexes could appear in different orders, hurting determinism for use-cases like schema comparison.

Issue: We observed that, when creating a database based on the same
gORM schema multiple times, indexes could appear in different orders,
hurting determinism for use-cases like schema comparison.

In order to fix this, it's simple to switch the ParseIndexes function
to return a list of indices rather than a map, so the callers will
iterate in deterministic order.
@bamo
Copy link
Author

bamo commented Sep 27, 2024

If the maintainers are happy with this, I think it makes sense to do this for ParseUniqueConstraints and ParseCheckConstraints as well.

I'm happy to do those as follow-on PRs as well. Just let me know.

@bamo
Copy link
Author

bamo commented Sep 27, 2024

The test failure seems like a flake or something (container failed to start). Looks like I don't have permissions to re-run it.

@bamo
Copy link
Author

bamo commented Oct 1, 2024

@jinzhu What do you think about this?

@aashishkapur
Copy link

I have the same issue - would be good to merge this fix.

@jinzhu
Copy link
Member

jinzhu commented Oct 9, 2024

Hi @bamo

Can you rebase the master branch, it might fix the tests.

@bamo
Copy link
Author

bamo commented Oct 9, 2024

@jinzhu done!

@bamo
Copy link
Author

bamo commented Oct 18, 2024

Hey @jinzhu, I don't mean to bother you here, just wondering if there's anything else you'd like me to do for this or if you think we can land this soon. Thanks!

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.

3 participants