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

Remove deprecated sort mode from geo_sort #119789

Closed
wants to merge 6 commits into from

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jan 8, 2025

This removes the deprecated sort_mode key from _geo_distance sorting. It's been deprecated since 5.0.

This removes the deprecated `sort_mode` key from `_geo_distance`
sorting. It's been deprecated since 5.0.
@nik9000 nik9000 added >breaking :Analytics/Geo Indexing, search aggregations of geo points and shapes v9.0.0 labels Jan 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 8, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @nik9000, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

Copy link
Contributor

@craigtaverner craigtaverner left a comment

Choose a reason for hiding this comment

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

Changelog needs updating

@@ -0,0 +1,12 @@
pr: 119789
summary: Remove deprecated sort mode from `geo_sort`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
summary: Remove deprecated sort mode from `geo_sort`
summary: Remove deprecated `sort_mode` from sort by `_geo_distance`

type: breaking
issues: []
breaking:
title: Remove deprecated sort mode from `geo_sort`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: Remove deprecated sort mode from `geo_sort`
title: Remove deprecated `sort_mode` from sort by `_geo_distance`

breaking:
title: Remove deprecated sort mode from `geo_sort`
area: Geo
details: Please describe the details of this change for the release notes. You can
Copy link
Contributor

Choose a reason for hiding this comment

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

This section appears to be a template, and should be replaced.

Copy link
Contributor

@craigtaverner craigtaverner left a comment

Choose a reason for hiding this comment

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

I updated the changlog, but while area: Geo was allowed, breaking.area: Geo was not, so I changed that to breaking.area: Search.

@craigtaverner craigtaverner requested a review from a team as a code owner January 9, 2025 10:29
@iverase
Copy link
Contributor

iverase commented Jan 9, 2025

I wonder if we still need to support Rest API compatibility here, so if the user adds to the call compatible-with=8 should still work?

@nik9000
Copy link
Member Author

nik9000 commented Jan 9, 2025

Having talked with a few folks we're not going to remove this. It's just not worth disrupting the tiny percentage of users that might use this. I'm going to close this and add a comment saying such so folks don't try this again.

@nik9000 nik9000 removed the v9.0.0 label Jan 9, 2025
@nik9000 nik9000 closed this Jan 9, 2025
@craigtaverner
Copy link
Contributor

Interesting, if we cannot remove anything as low impact as this (no behavioural change, only syntax change) deprecated as far back as 5.0, is it really possible to remove anything at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >breaking Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants