添加链接
link管理
链接快照平台
  • 输入网页链接,自动生成快照
  • 标签化管理网页链接

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 DateHistogramAggregation - changing Interval to FixedInterval produces an invalid request #4839 DateHistogramAggregation - changing Interval to FixedInterval produces an invalid request #4839 awelburn opened this issue Jul 1, 2020 · 1 comment · Fixed by #4856

NEST/Elasticsearch.Net version : 7.8.1
Elasticsearch version : 7.8.0

Description of the problem including expected versus actual behavior :
Converting from 6.x to 7, using NEST.7xUpgradeAssistant (really helpful btw!)
When changing code from
.Interval(DateInterval.Day)
the comment on the Obsolete attribute implies that the equivalent call should be
.FixedInterval(DateInterval.Day)
but this seems to produce an invalid request? (showing only the aggs fragment):
"aggs": {"doccountbyday": {"date_histogram": {"field": "published","fixed_interval": "day"}}}
The Reason is
failed to parse setting [date_histogram.fixedInterval] with value [day] as a time value: unit is missing or unrecognized

I've figured out that the new code should be
.FixedInterval(new Time(1, TimeUnit.Day))
which does produce a valid request:
{"aggs":{"doccountbyday":{"date_histogram":{"field":"published","fixed_interval":"1d"}}}

Expected behavior
Either remove FixedInterval(DateInterval interval) - it's not a breaking change if it's already broken?!
or perform the conversion from DateInterval to Time within the method?

Thanks for opening @awelburn .

This is an unfortunate bug- FixedInterval should accept only a Time , since a unit needs to be supplied.

Either remove FixedInterval(DateInterval interval) - it's not a breaking change if it's already broken?!
or perform the conversion from DateInterval to Time within the method?

You make a good point about when is a breaking change acceptable :) My personal preference is to fix this in master by breaking, and implement a formatter in 7.x to resolve at serialization; my reasoning here is that it's a small fix to workaround in 7.x, provides a "just works" path for folks upgrading, and doesn't break the signature IDateHistogramAggregation for everyone. FixedInterval doesn't support all of the units of DateInterval such as Month , Quarter and Year . For these, I don't think we have any choice with these but to throw an exception.

What are your thoughts, @Mpdreamz ?

1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 Co-authored-by: Russ Cam <[email protected]> 1. For FixedInterval to accept only Time 2. For CalendarInterval to accept DateInterval or DateMathTime This aligns with DateHistogramCompositeAggregationSource. Fixes #4839 Co-authored-by: Russ Cam <[email protected]>