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

Removes toString->parse roundtrip for missing value in terms agg #67954

Merged
merged 3 commits into from
Jan 26, 2021

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jan 25, 2021

Removes unnecessary and confusing roundtrip that missing values are undertaking
in the terms agg if they are already specified as numbers. It also improves a
somewhat confusing error message when the missing value cannot be parsed
according to the specified format.

Closes #67197

Removes unnecessary and confusing roundtrip that missing values are undertaking
in the terms agg if they are already specified as numbers. It also improves a
somewhat confusing error message when the missing value cannot be parsed
according to the specified format.

Closes elastic#67197
@imotov imotov requested a review from not-napoleon January 25, 2021 23:15
@imotov imotov marked this pull request as ready for review January 25, 2021 23:15
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 25, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Makes me happy.

@@ -454,7 +454,7 @@ public double parseDouble(String value, boolean roundUp, LongSupplier now) {
try {
n = format.parse(value);
} catch (ParseException e) {
throw new RuntimeException(e);
throw new RuntimeException("Cannot parse the value [" + value + "] using the pattern [" + pattern + "]");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add , e just so we don't lose it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch!

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

@imotov imotov merged commit 03d4ba5 into elastic:master Jan 26, 2021
imotov added a commit that referenced this pull request Jan 26, 2021
) (#68014)

Removes unnecessary and confusing roundtrip that missing values are undertaking
in the terms agg if they are already specified as numbers. It also improves a
somewhat confusing error message when the missing value cannot be parsed
according to the specified format.

Closes #67197
@imotov imotov deleted the issue-67197-unparseable-number branch February 4, 2021 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Unparseable number" exception when missing is used with format in terms agg
5 participants