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

Clean up top_metric's internal API #71738

Merged
merged 6 commits into from
Apr 19, 2021
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 15, 2021

This changes top_metrics's implementation of getProperty and value
to be a bit more consisntent which shold make it easier to integrate
into transform. We expect value to return NaN when the value isn't a
number or is null but was throwing exceptions in both that cases.
getProperty was throwing exceptions on null and non-numeric values as
well. Now it returns null or BytesRef.

This changes `top_metrics`'s implementation of `getProperty` and `value`
to be a bit more consisntent which shold make it easier to integrate
into transform. We expect `value` to return `NaN` when the value isn't a
number or is null but was throwing exceptions in both that cases.
`getProperty` was throwing exceptions on null and non-numeric values as
well. Now it returns `null` or `BytesRef`.
}
return metric.numberValue();
return metric.getValue().getKey();
Copy link
Member Author

Choose a reason for hiding this comment

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

This'll return a BytesRef if you fetch from a keyword or ip or bytes field. Would it be more useful to you if I formatted it? Or do you want the raw bytes?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my local version I have a valueAsString method, I don't think I need this interface.

I will ping you on my PR once I have it ready.

Copy link
Contributor

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

@nik9000 nik9000 marked this pull request as ready for review April 15, 2021 19:08
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 15, 2021
@elasticmachine
Copy link
Collaborator

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

@nik9000 nik9000 merged commit 4db5284 into elastic:master Apr 19, 2021
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 19, 2021
This changes `top_metrics`'s implementation of `getProperty` and `value`
to be a bit more consisntent which shold make it easier to integrate
into transform. We expect `value` to return `NaN` when the value isn't a
number or is null but was throwing exceptions in both that cases.
`getProperty` was throwing exceptions on null and non-numeric values as
well. Now it returns `null` or `BytesRef`.
@nik9000 nik9000 added v7.14.0 and removed v7.13.0 labels Apr 19, 2021
nik9000 added a commit that referenced this pull request Apr 20, 2021
This changes `top_metrics`'s implementation of `getProperty` and `value`
to be a bit more consisntent which shold make it easier to integrate
into transform. We expect `value` to return `NaN` when the value isn't a
number or is null but was throwing exceptions in both that cases.
`getProperty` was throwing exceptions on null and non-numeric values as
well. Now it returns `null` or `BytesRef`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants