Open colings86 opened 8 years ago
I think the terms aggregation validates the order during the search phase. I get a 500
response with a bad terms agg order vs a 503
from a bad (date) histogram order. Example.
It looks like validation code does exist in AggregationPath.java. The code gets a bit hairy but I think the reason this is called during the search phrase is that all the Aggregator
s need to be created before validation which happens in the AggregationPhase
. Would it be possible to create the Aggregator
s during the parsing phase and then perform validation?
If that's too painful it seems it wouldn't be too hard to call the same validation logic the terms agg uses during the search phrase for the (date) histogram.
Also the terms
agg supports multiple criteria for the order
, but the (date) histogram
agg does not: "order" : [ { "rock>playback_stats.avg" : "desc" }, { "_count" : "desc" } ]
. I don't see a reason why the histogram agg order
would have different behavior than the terms
agg. The class org.elasticsearch.search.aggregations.bucket.terms.InternalOrder.CompoundOrder
contains the code to support multiple criteria for the terms
agg order
.
@qwerty4030 the parsing happens on the coordinating node (the node that receives the request from the client) but Aggregator
s are created on the shard so we can't create the Aggregator
s during the parsing phase and perform the validation with them. I think we will need to create code to do the validation using the AggregatorBuilder
objects instead since this is what we have available on the coordinating node at parse time.
As for the discrepancy between order in the terms aggregation and the histogram aggregations, it would be nice to have them consistent. Maybe we could have both use the same Order
classes rather than having separate implementations but I fear that this will be fairly tricky.
I can see this happening in two parts:
histogram
aggs to use the same order
code/logic as the terms
agg.order
to be validated at parse time (use AggregatorBuilder
instead of Aggregator
).1 doesn't seem too bad since the code already exists. 2 is more involved: I saw alot of instanceof
s, casting, and tricky assumptions in the current order
validation. Ideally the order
validation can be cleaned up to be more interface friendly.
@colings86 does this issue need further discussion?
Well now that step 1 was done in #22343 we can give part 2 a shot. @colings86 @javanna Do you guys see any obvious roadblocks to validate terms
and histogram
aggs that are ordered by sub-aggregation, e.g. multi sub-agg levels, metrics, scripts, etc... by just looking at the AggregatorBuilder
instance? Thanks
@qwerty4030 I don't see any roadblocks. I think this should be possible since the pipeline aggregations do something a little similar when validating the buckets_path
, however I'm not going to go so far as saying that its going to be easy. Maybe we should start with trying to add the validation on AggregatorBuilder's without modifying the existing logic with the Aggregator's (so essentially do both) and then once we know the validation using the AggregatorBuilder's is robust we can remove any unnecessary logic later int he execution?
Sounds good. Thanks for the tip about the pipeline aggregations. I found some relevant code here, but I'm not sure how robust this validation is.
@elastic/es-search-aggs
Pinging @elastic/es-analytics-geo (Team:Analytics)
Because the exception is thrown in the reduce phase it is a SearchPhaseExecutionException which maps to a 503 Unavailable status code when in fact it should return a 400 Bad Request status code and ideal the exception should be a SearchParseException
Changing this to a bug - to address that we are returning the correct error code.
Pinging @elastic/es-analytical-engine (Team:Analytics)
At the moment we don't validate that the order parameter for the histogram, date_histogram and terms aggregations is valid at parse time. Instead we rely on an exception to be thrown when we try to sort the buckets at reduce time. Apart from the not being a nice user experience as we should fail quickly on bad requests this also causes a few other issues highlighted in #14771 and #19851:
SearchPhaseExecutionException
which maps to a503 Unavailable
status code when in fact it should return a400 Bad Request
status code and ideal the exception should be aSearchParseException