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

Bugfix sort order query string in elastic search api. #5490

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

fulmicoton
Copy link
Contributor

No description provided.

@fulmicoton fulmicoton force-pushed the sort-order-query-string-broken branch 3 times, most recently from b9a030f to ce8fcaf Compare October 15, 2024 05:08
The sort order was parsed using the generated from_str_name method.
The spec of this method probably changed at one point, and we ended up
requiring sort order to be uppercase.

This PR changes the behavior to follow what we do when we parse a json
string, and adds unit tests.

Closes #5491
@fulmicoton fulmicoton force-pushed the sort-order-query-string-broken branch from ce8fcaf to d95318c Compare October 15, 2024 05:10
Copy link

On SSD:

Average search latency is 1.0x that of the reference (lower is better).
Ref run id: 3842, ref commit: 4c1e228
Link

On GCS:

Average search latency is 1.05x that of the reference (lower is better).
Ref run id: 3843, ref commit: 4c1e228
Link

Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

do we want to keep supporting upper case? I don't see harm in doing so

@trinity-1686a
Copy link
Contributor

fix #5491

@fulmicoton fulmicoton merged commit e08eb9f into main Oct 16, 2024
5 checks passed
@fulmicoton fulmicoton deleted the sort-order-query-string-broken branch October 16, 2024 10:33
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