Skip to content

Conversation

@benbenwilde
Copy link

@benbenwilde benbenwilde commented Sep 26, 2019

Fixes an issue where a datetime extended_stats aggregation was getting deserialized as a regular stats aggregation.

Running extended_stats on a datetime field (vs a numeric field) returns a different response with extra "as_string" fields.

e.g.

   "extended_stats#1": {
            "count": 3,
            "min": 1569521764937,
            "max": 1569526264937,
            "avg": 1569524464937,
            "sum": 4708573394811,
            "min_as_string": "2019-09-26T18:16:04.937Z",
            "max_as_string": "2019-09-26T19:31:04.937Z",
            "avg_as_string": "2019-09-26T19:01:04.937Z",
            "sum_as_string": "2119-03-18T09:03:14.811Z",
            "sum_of_squares": 7.390221138118668e+24,
            "variance": 3779929134421.3335,
            "std_deviation": 1944203.9847766317,
            "std_deviation_bounds": {
                "upper": 1569528353344.9695,
                "lower": 1569520576529.0305
            },
            "sum_of_squares_as_string": "292278994-08-17T07:12:55.807Z",
            "variance_as_string": "2089-10-12T04:18:54.421Z",
            "std_deviation_as_string": "1970-01-01T00:32:24.203Z",
            "std_deviation_bounds_as_string": {
                "upper": "2019-09-26T20:05:53.344Z",
                "lower": "2019-09-26T17:56:16.529Z"
            }
        }

The aggregate json converter was intended to grab the first set of fields, skip the "as_string" metrics, and then, if any more fields remained, to grab the rest of the fields for extended metrics.

However, the existing code was not going back to the json reader when checking to see if the current field was an "as_string" field. Therefore it thought that all of the remaining fields were "as_string" fields, even though that was not the case.

Fixes an issue where datetime extended_stats aggregation was getting deserialized as a regular stats aggregation
Note that this fix might not impact anything in practice, since when this code is hit, the rest of the fields are "as_string" fields anyways (in the response examples I'm aware of).
@benbenwilde
Copy link
Author

benbenwilde commented Sep 27, 2019

Also, this bug causes any additional sequential aggregations to fail to be parsed, because it actually reads to the first } and then drops out. So once it gets out it thinks it's back at the aggregation dictionary level, but really it's at sum_of_squares_as_string. This produces a few empty results in the deserialized aggregation dictionary, and causes other aggregations (and possibly other things from the result) to be lost if they sequentially follow the extended stats aggregation.

@benbenwilde
Copy link
Author

This also causes a client exception while reading the response if the date time extended stats field is on a nested field.

@russcam russcam self-assigned this Sep 30, 2019
russcam added a commit that referenced this pull request Sep 30, 2019
Supersedes: #4103 

Fixes an issue where datetime extended_stats aggregation was getting deserialized as a regular stats aggregation

Note that this fix might not impact anything in practice, since when this code is hit, the rest of the fields are "as_string" fields anyways (in the response examples I'm aware of).

Add a unit test to assert that the change to AggregateJsonConverter for ExtendedStats aggregation can deserialize the provided response.

Co-authored-by: benbenwilde <[email protected]>
@russcam
Copy link
Contributor

russcam commented Sep 30, 2019

Closing this as I brought your changes into #4109 and added a unit test.

Thanks for opening this PR, @benbenwilde 👍

@russcam russcam closed this Sep 30, 2019
russcam added a commit that referenced this pull request Sep 30, 2019
Add a unit test to assert that ExtendedStats aggregation can deserialize the provided response in #4103

(cherry picked from commit 1e45ee8)
russcam added a commit that referenced this pull request Sep 30, 2019
Add a unit test to assert that ExtendedStats aggregation can deserialize the provided response in #4103

(cherry picked from commit 1e45ee8)
russcam added a commit that referenced this pull request Oct 1, 2019
Add a unit test to assert that ExtendedStats aggregation can deserialize the provided response in #4103

(cherry picked from commit 1e45ee8)
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