Remove deprecated date histo interval #75000
Conversation
not-napoleon
added
>breaking
:Analytics/Aggregations
| } else { | ||
| return new DateHistogramInterval(p.text()); | ||
| } | ||
| }, Histogram.INTERVAL_FIELD, ObjectParser.ValueType.LONG); |
There was a problem hiding this comment.
to support compatibility you can set .forRestApiVersion(equalTo(RestApiVersion.V_7) on Histogram.INTERVAL_FIELD
this will only parse the field for v7 requests
There was a problem hiding this comment.
yup, I'm working on that now. It needs to be slightly more clever, because I don't want to keep the field itself in 8.0. Instead we can make a reasonable guess as to how to turn the 7.x version into one of the fields we're replacing it with in 8.0. I'll ping you when I have a draft ready
There was a problem hiding this comment.
@pgomulka Thanks for your help with this! I think it's all set now for version compatibility now.
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| } else { | ||
| wrapper.dateHistogramInterval((DateHistogramInterval) interval); | ||
| try { |
There was a problem hiding this comment.
I wonder if we need try catch logic. Maybe we could extract https://github.com/elastic/elasticsearch/pull/75000/files#diff-dd9ebdfdc17eaf7e26fe7cb69a5695245895ff84a1b268b75da2937628f7d8e7R165 check and choose if calendarInterval or fixedInterval should be called?
There was a problem hiding this comment.
That link isn't working, but I'm pretty sure I know what you mean. I replaced the try/catch with an if/else.
| assertThat(e.toString(), containsString("[interval] must be 1 or greater for aggregation [date_histogram]")); | ||
| } | ||
| } | ||
| // TODO: add some tests for negative fixed and calendar intervals |
There was a problem hiding this comment.
Will those tests be part of this PR or you plan to add them in another PR?
There was a problem hiding this comment.
Yeah, I was going to add them in later. I don't think it's terribly urgent, and the test I removed only checked it on the legacy interval, so our coverage isn't worse for not adding them now. And it's much less likely to be a problem because fixed & calendar intervals don't just accept bare longs, so a negative will almost definitely fail to parse. Truthfully, I'm not even 100% sure it's worth doing, but I figured a small note was low friction.
| default: | ||
| throw new IllegalStateException("Unknown interval type."); | ||
| if (type.isValid() == false || intervalType.isValid() == false) { | ||
| throw new IllegalStateException("Unknown interval type."); |
There was a problem hiding this comment.
Nit: Should this be an IllegalArgumentException instead of a IllegalStateException.
|
@elasticmachine update branch |
Date histogram interval parameter was deprecated in 7.2, in favor of the more specific fixed_interval and calendar_interval parameters. The old logic used some poorly understood guessing to decide if it should operate in fixed or calendar mode. The new logic requires a specific choice by the user, which is more explicit. In 7.x REST compatibility mode, we will parse the interval as calendar if possible, and otherwise interpret it as fixed.
This will remove the
intervalparameter to date histogram aggregations and composites, deprecated in 7.2. The more precisefixed_intervalandcalendar_intervaloptions should be used instead.