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(NODE-3174): Preserve sort key order for numeric string keys #2788

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented Apr 23, 2021

Description

Updated internal representation of sort to use a map instead of a plain object in order to preserve correct ordering for multi-key sort queries; also enabled the use of a map type to specify input sort options

@dariakp dariakp marked this pull request as ready for review April 23, 2021 21:41
@dariakp dariakp requested review from emadum and durran April 23, 2021 22:14
@dariakp dariakp force-pushed the NODE-3174/preserve-sort-key-order branch from 79c4b10 to ed9ee63 Compare April 26, 2021 22:09
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

LGTM. Are the todos to be addressed in another PR or ticket?

@dariakp
Copy link
Contributor Author

dariakp commented Apr 27, 2021

LGTM. Are the todos to be addressed in another PR or ticket?

It really depends on how the investigation for the unified test framework/extended json in our internal DRIVERS-1701 ticket plays out. Unless the outcome is favorable, this won't be able to be addressed at all, but I wanted to keep the test marked regardless so that it's top of mind whenever we get around to cleaning them up.

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Nice work! Can you create a Jira ticket for the future work described in the TODO comments, and update the comments with a reference to the ticket? These kind of comments have a tendency to never get followed up on if they aren't associated with a ticket. (Edit: perhaps the DRIVERS ticket mentioned above would suffice?)

@dariakp dariakp requested a review from emadum April 27, 2021 18:20
@dariakp dariakp merged commit 440de41 into 4.0 Apr 28, 2021
@dariakp dariakp deleted the NODE-3174/preserve-sort-key-order branch April 28, 2021 14:17
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