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

Better error message when buckets_path refers to multi-bucket agg #30152

Closed

Conversation

polyfractal
Copy link
Contributor

@polyfractal polyfractal commented Apr 25, 2018

Instead of throwing a technical error when the user specifies a pipeline agg path that includes a multi-bucket, we can check during the property resolution and throw a friendlier exception

The old exception:

buckets_path must reference either a number value or a single value numeric metric aggregation, got: java.lang.Object[]

Compared to the new exceptions:

[histo2] is a [date_histogram], but only number value or a single value numeric metric aggregation are allowed.

[the_percentiles] is a [tdigest_percentiles] which contains multiple values, but only number value or a single value numeric metric aggregation are allowed. Please specify which value to return.

I'm not super pleased with how this was implemented, adding a boolean to getProperty(). But it seemed the least invasive way. In particular because we have to deal with both multi-buckets and numeric aggs that have multiple values.

Note: this uses the same changes to AggregatorTestCase as #29641 so we can test pipelines easier. Not relevant anymore

Closes #25273

Instead of throwing a technical error (`found Object[]`), we can check
to see if any of the referenced aggs are a multi-bucket agg and throw
a friendlier exception.

Closes elastic#25273
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@colings86
Copy link
Contributor

@polyfractal I assume this is still something we want to get merged? Should we update the branch and find a reviewer?

@polyfractal
Copy link
Contributor Author

Urgh yes, forgot about this. Will get it updated and find a victim reviewer :)

@polyfractal
Copy link
Contributor Author

Hmm, I'm looking at this again and I don't really like how it was implemented. I'll carve out some time to re-examine the issue and see if it can be fixed a different way that doesn't need to change the method signatures. Just feels like a lot of complication for a nicer error message.

@polyfractal
Copy link
Contributor Author

Closing for now. I've decided I don't like the implementation, and don't think there's a need to keep this PR pending/stalled while (or if) I think about a better way.

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

Successfully merging this pull request may close these issues.

4 participants