Skip to content

Use Sort object in "delete by query" #5143

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 1 commit into from
Aug 19, 2025

Conversation

ramikg
Copy link
Contributor

@ramikg ramikg commented Aug 17, 2025

The delete by query API's sort property supports the Sort object:

"sort": {
  "timestamp": {
    "order": "desc",
    "unmapped_type": "date"
  }
}

But the property's type does not reflect this fact.

This PR changes the type of sort from string[] to the more general Sort object (as used by the search API):

But the body is missing the sort parameter, this PR adds it as Sort object.

@l-trotta
Copy link
Contributor

Hello @ramikg, thank you for you interest in contributing to the spec!
That sort field in DeleteByQueryRequest is a query param and cannot be an object, if you check SearchRequest you can see that it has the same field.
One thing we have to do for sure is making them the same, so sort in DeleteByQueryRequest should also be a string | string[]. But I'm not finding any evidence that DeleteByQueryRequest has a body field sort, did you manage to reproduce it?

@l-trotta l-trotta self-assigned this Aug 19, 2025
@ramikg
Copy link
Contributor Author

ramikg commented Aug 19, 2025

Oops, I've missed that the sort property I've modified is under query_parameters.
I've restored the query parameter to its original form.

I tried using the sort property (exactly as provided above) in the body and verified that Elasticsearch actually parses it (e.g. wrong order or unmapped_type values resulted in an error).
I've checked this in Elasticsearch versions 9.1.2 & 8.19.2.

@ramikg ramikg force-pushed the fix-sort-type-in-deletebyquery branch from 57c6eb1 to 6d06941 Compare August 19, 2025 10:51
@l-trotta
Copy link
Contributor

@ramikg thanks for checking! I've done a bit of digging and this seems to be part of a larger issue, where delete by query actually supports all parameters supported by search request. let's merge this for now, then we'll see what to do about the rest :D

@l-trotta l-trotta merged commit 380cde2 into elastic:main Aug 19, 2025
16 of 18 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 19, 2025
github-actions bot pushed a commit that referenced this pull request Aug 19, 2025
github-actions bot pushed a commit that referenced this pull request Aug 19, 2025
github-actions bot pushed a commit that referenced this pull request Aug 19, 2025
@ramikg
Copy link
Contributor Author

ramikg commented Aug 19, 2025

Thank you!

By the way, this seems to be the case for "update by query" as well (and perhaps for additional APIs).

l-trotta pushed a commit that referenced this pull request Aug 19, 2025
(cherry picked from commit 380cde2)

Co-authored-by: Rami <[email protected]>
l-trotta pushed a commit that referenced this pull request Aug 19, 2025
(cherry picked from commit 380cde2)

Co-authored-by: Rami <[email protected]>
l-trotta pushed a commit that referenced this pull request Aug 19, 2025
(cherry picked from commit 380cde2)

Co-authored-by: Rami <[email protected]>
l-trotta pushed a commit that referenced this pull request Aug 19, 2025
(cherry picked from commit 380cde2)

Co-authored-by: Rami <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
@l-trotta
Copy link
Contributor

We've investigated this a bit more and found out that the sort body parameter actually does nothing in this context, and moreover the sort query parameter isn't actually supported. I'm reverting this and deprecating the existing query param, sorry for the confusion!

l-trotta added a commit that referenced this pull request Aug 21, 2025
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
This reverts commit 380cde2.

(cherry picked from commit 00fe735)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
This reverts commit 380cde2.

(cherry picked from commit 00fe735)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
This reverts commit 380cde2.

(cherry picked from commit 00fe735)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
This reverts commit 380cde2.

(cherry picked from commit 00fe735)
l-trotta added a commit that referenced this pull request Aug 21, 2025
…5173)

This reverts commit 380cde2.

(cherry picked from commit 00fe735)

Co-authored-by: Laura Trotta <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
…5174)

This reverts commit 380cde2.

(cherry picked from commit 00fe735)

Co-authored-by: Laura Trotta <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
…5175)

This reverts commit 380cde2.

(cherry picked from commit 00fe735)

Co-authored-by: Laura Trotta <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
…5176)

This reverts commit 380cde2.

(cherry picked from commit 00fe735)

Co-authored-by: Laura Trotta <[email protected]>
@ramikg
Copy link
Contributor Author

ramikg commented Aug 21, 2025

We've investigated this a bit more and found out that the sort body parameter actually does nothing in this context, and moreover the sort query parameter isn't actually supported. I'm reverting this and deprecating the existing query param, sorry for the confusion!

Could you please elaborate?

I may be missing something, but in my tests it seems that the sort body parameter is effective. For example:

POST test/_bulk
{ "index": {} }
{ "amount": 1 }
{ "index": {} }
{ "amount": 2 }
{ "index": {} }
{ "amount": 3 }
{ "index": {} }
{ "amount": 4 }
{ "index": {} }
{ "amount": 5 }

POST test/_delete_by_query
{
  "query": {
    "match_all": {}
  },
  "sort": {
    "amount": {
      "order": "asc",
      "unmapped_type": "long"
    }
  },
  "max_docs": 2
}

After running the commands above, the remaining amounts were {3, 4, 5}, and then when I deleted again with "order": "desc" then I was left with just 3.
(I've also tried several other experiments.)

@l-trotta
Copy link
Contributor

@ramikg ah that's how! sorry I was testing with size, not considering max_docs. reverting again ^^"

l-trotta added a commit that referenced this pull request Aug 21, 2025
l-trotta pushed a commit that referenced this pull request Aug 21, 2025
l-trotta added a commit that referenced this pull request Aug 21, 2025
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
Co-authored-by: Rami <[email protected]>
(cherry picked from commit 18cb83c)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
Co-authored-by: Rami <[email protected]>
(cherry picked from commit 18cb83c)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
Co-authored-by: Rami <[email protected]>
(cherry picked from commit 18cb83c)
github-actions bot pushed a commit that referenced this pull request Aug 21, 2025
Co-authored-by: Rami <[email protected]>
(cherry picked from commit 18cb83c)
l-trotta added a commit that referenced this pull request Aug 21, 2025
(cherry picked from commit 18cb83c)

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Rami <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
(cherry picked from commit 18cb83c)

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Rami <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
(cherry picked from commit 18cb83c)

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Rami <[email protected]>
l-trotta added a commit that referenced this pull request Aug 21, 2025
(cherry picked from commit 18cb83c)

Co-authored-by: Laura Trotta <[email protected]>
Co-authored-by: Rami <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants