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

Change string sort order #6406

Closed
wants to merge 8 commits into from
Closed

Change string sort order #6406

wants to merge 8 commits into from

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Mar 22, 2023

This implements option 3 from #6118

  • Strings and Binaries are no longer considered equal even if they have the same payload.
  • String sort order uses unsigned character values instead of our utf8_compare. This is a user facing breaking change that has been requested by Incorrect special character sorting #2573
  • Since the string order has changed, we need to bump the file format because Set<StringData> and Set<Mixed> (with strings) and all string indexes have a new structure.

The first commit in this PR is a cherry-pick of #5662 because otherwise all this will conflict in a difficult way.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@ironage ironage self-assigned this Mar 22, 2023
@cla-bot cla-bot bot added the cla: yes label Mar 22, 2023
@jedelbo
Copy link
Contributor

jedelbo commented Mar 22, 2023

@ironage perhaps you can rebase this once we get master merged into next-major. The "delete some obsolete cruft" is now in master.

@jedelbo
Copy link
Contributor

jedelbo commented Mar 22, 2023

I will make the merge into next-major once we have a new release.

@ironage ironage marked this pull request as draft March 22, 2023 17:38
@ironage
Copy link
Contributor Author

ironage commented Mar 22, 2023

I will leave this as a draft until it is rebased to include #5662. Additionally, we will need server changes to support the new RQL syntax before releasing this. That is tracked in https://jira.mongodb.org/browse/BAAS-20265.

@michael-wb
Copy link
Contributor

michael-wb commented Mar 22, 2023

@ironage - #5662 was causing build failures and was reverted. I restored the tg/delete-cruft branch, but a new pull request and fixes will be needed to merge those changes again.

tgoyne and others added 4 commits April 12, 2023 16:35
Partially based on 5f2dda1 Delete some obsolete cruft

set_string_compare_method() and everyhing related to it has never actually been
used by any SDK, and is not really the correct solution to the problem anyway.
test migration
@ironage ironage marked this pull request as ready for review April 24, 2023 09:20
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ironage can this somehow be divided in two: One PR handling the changes to the query system and one handling the changes to the index? The one handling the index should also include upgrade logic and test cases for that. When you write the upgrade tests, you can create the input files and test case on the branch je/upgrade-from-23-tests.

@ironage
Copy link
Contributor Author

ironage commented May 24, 2023

I will split this up. @jedelbo I cannot find the branch je/upgrade-from-23-tests did you push it?

@ironage ironage marked this pull request as draft May 24, 2023 17:46
This was referenced May 24, 2023
@ironage
Copy link
Contributor Author

ironage commented Oct 19, 2023

These changes have been broken down and replaced and merged by 3 parts: #6668 #6669 #6670

@ironage ironage closed this Oct 19, 2023
@ironage ironage deleted the js/sorting branch October 19, 2023 16:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants