Skip to content

[DOCS] Remove _term and _time agg order keys#78209

Merged
jrodewig merged 1 commit intoelastic:masterfrom
jrodewig:docs__remove-_time_term-agg-order
Sep 22, 2021
Merged

[DOCS] Remove _term and _time agg order keys#78209
jrodewig merged 1 commit intoelastic:masterfrom
jrodewig:docs__remove-_time_term-agg-order

Conversation

@jrodewig
Copy link
Copy Markdown
Contributor

@jrodewig jrodewig commented Sep 22, 2021 •
edited
Loading

Adds an 8.0 breaking change for the removal of the _term and _time
agg order keys.

Relates to #39450

Preview

https://elasticsearch_78209.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking_80_aggregations_changes

@jrodewig
[DOCS] Remove _term and _time agg order keys
d5c0d25 
Adds an 8.0 breaking change for the removal of the `_term` and `_time`
agg `order` keys.

Relates to #39450
@jrodewig jrodewig marked this pull request as ready for review September 22, 2021 19:33
@jrodewig jrodewig added the :Analytics/Aggregations Aggregations label Sep 22, 2021
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 22, 2021
@jrodewig jrodewig requested a review from nik9000 September 22, 2021 19:34
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 22, 2021

Pinging @elastic/es-analytics-geo (Team:Analytics)

@not-napoleon
Copy link
Copy Markdown
Member

not-napoleon commented Sep 22, 2021

Should we (do we?) support this in 7.x REST compatibility mode? IIRC that didn't exist when we dropped these sort keys

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Sep 22, 2021

Should we (do we?) support this in 7.x REST compatibility mode? IIRC that didn't exist when we dropped these sort keys

I think we probably should, yeah. I think the docs change is still right even so.

@jrodewig
Copy link
Copy Markdown
Contributor Author

jrodewig commented Sep 22, 2021

Should we (do we?) support this in 7.x REST compatibility mode? IIRC that didn't exist when we dropped these sort keys

Thanks for calling this out @not-napoleon. It looks like we do support them with version compatibility headers. However, I don't think we need to call that out in this breaking change. IMO that's sorta the same as using a previous version of the API.

elasticsearch/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java

Line 579 in 979f297

if (parser.getRestApiVersion() == RestApiVersion.V_7 && ("_term".equals(orderKey) || "_time".equals(orderKey))) {

not-napoleon
not-napoleon approved these changes Sep 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

Yes, sorry, docs change is fine. I think we just need to also do REST compatibility.

@jrodewig jrodewig merged commit 15baf40 into elastic:master Sep 22, 2021
@jrodewig jrodewig deleted the docs__remove-_time_term-agg-order branch September 22, 2021 19:54
@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Sep 22, 2021

It looks like we do support them with version compatibility headers.

Nice! I'd assumed we dumped them. I'm so happy we still have that. Now I can GC _term and _time from my brain again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

@not-napoleon not-napoleon not-napoleon approved these changes

Assignees

No one assigned

Labels

:Analytics/Aggregations Aggregations >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jrodewig @elasticmachine @not-napoleon @nik9000 @jakelandis @elasticsearchmachine