Skip to content

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

Closed
polyfractal wants to merge 7 commits intoelastic:masterfrom
polyfractal:better_error_pipeline_path
Closed

Better error message when buckets_path refers to multi-bucket agg #30152
polyfractal wants to merge 7 commits intoelastic:masterfrom
polyfractal:better_error_pipeline_path

Conversation

@polyfractal
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

Pinging @elastic/es-search-aggs

@colings86
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@polyfractal
Copy link
Copy Markdown
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
Copy Markdown
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