DateHistogramAggregation - changing Interval to FixedInterval produces an invalid request
· 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]>