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 missing fields in range aggregation response for date fields #82732

Merged

Conversation

salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Jan 18, 2022

This is a fix for #82688

When running a range aggregation on a date field both fromAsStr and
toAsStr need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in to, from, originalTo
and originalFrom which in turn result in:

  • incorrect key values (wildcard character used for infinite values);
  • missing fields in the response (to, from, to_as_string, from_as_string)
    due to checks for infinite values in the serialisation logic;

Here we make sure we use the correct from/to values by eventually using
either numeric ranges or parsing string ranges.

When running a range aggregation on a date field both `fromAsStr` and
`toAsStr` need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in `to`, `from`, `originalTo`
and `originalFrom` which in turn result in:
* incorrect key values (wildcard character used for infinite values);
* missing fields in the response (to, from, to_as_string, from_as_string)
  due to checks for initite values in the serialization logic;
@salvatore-campagna
Copy link
Contributor Author

This needs to be back-ported to 7.17 (feature freeze tomorrow) and 8.0.

@salvatore-campagna salvatore-campagna added v7.17.0 v8.0.0 :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jan 18, 2022
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM

@salvatore-campagna salvatore-campagna added the auto-backport Automatically create backport pull requests when merged label Jan 18, 2022
Copy link
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.

LGTM. Thanks for the quick turn around.

@salvatore-campagna salvatore-campagna merged commit 1186e90 into elastic:master Jan 18, 2022
salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Jan 18, 2022
…astic#82732)

When running a range aggregation on a date field both `fromAsStr` and
`toAsStr` need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in `to`, `from`, `originalTo`
and `originalFrom` which in turn result in:
* incorrect key values (wildcard character used for infinite values);
* missing fields in the response (to, from, to_as_string, from_as_string)
  due to checks for initite values in the serialization logic;
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.0
7.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 82732

salvatore-campagna added a commit to salvatore-campagna/elasticsearch that referenced this pull request Jan 18, 2022
…astic#82732)

When running a range aggregation on a date field both `fromAsStr` and
`toAsStr` need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in `to`, `from`, `originalTo`
and `originalFrom` which in turn result in:
* incorrect key values (wildcard character used for infinite values);
* missing fields in the response (to, from, to_as_string, from_as_string)
  due to checks for initite values in the serialization logic;

(cherry picked from commit 1186e90)

# Conflicts:
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/40_range.yml
@salvatore-campagna
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
7.17

Questions ?

Please refer to the Backport tool documentation

salvatore-campagna added a commit that referenced this pull request Jan 18, 2022
…lds (#82732) (#82757)

When running a range aggregation on a date field both `fromAsStr` and
`toAsStr` need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in `to`, `from`, `originalTo`
and `originalFrom` which in turn result in:
* incorrect key values (wildcard character used for infinite values);
* missing fields in the response (to, from, to_as_string, from_as_string)
  due to checks for initite values in the serialization logic;

(cherry picked from commit 1186e90)
salvatore-campagna added a commit that referenced this pull request Jan 18, 2022
…ds (#82732) (#82755)

When running a range aggregation on a date field both `fromAsStr` and
`toAsStr` need to be parsed to extract the correct range boundaries.
Not doing so results in +/- infinite values in `to`, `from`, `originalTo`
and `originalFrom` which in turn result in:
* incorrect key values (wildcard character used for infinite values);
* missing fields in the response (to, from, to_as_string, from_as_string)
  due to checks for initite values in the serialization logic;

* Update yaml test skip version after back-port from 8.1.0
@albertzaharovits albertzaharovits changed the title fix: missing fields in range aggregation response for date fields Fix missing fields in range aggregation response for date fields Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants