Skip to content

Throw exception if legacy interval cannot be parsed in DateIntervalWrapper #41972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Merged
polyfractal merged 1 commit into from
May 9, 2019
Merged

Throw exception if legacy interval cannot be parsed in DateIntervalWrapper #41972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ...main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateIntervalWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,15 @@ public Rounding createRounding(ZoneId timeZone) {
} else {
// We're not sure what the interval was originally (legacy) so use old behavior of assuming
// calendar first, then fixed. Required because fixed/cal overlap in places ("1h")
DateTimeUnit intervalAsUnit = tryIntervalAsCalendarUnit();
if (intervalAsUnit != null) {
tzRoundingBuilder = Rounding.builder(tryIntervalAsCalendarUnit());
DateTimeUnit calInterval = tryIntervalAsCalendarUnit();
TimeValue fixedInterval = tryIntervalAsFixedUnit();
if (calInterval != null) {
tzRoundingBuilder = Rounding.builder(calInterval);
} else if (fixedInterval != null) {
tzRoundingBuilder = Rounding.builder(fixedInterval);
} else {
tzRoundingBuilder = Rounding.builder(tryIntervalAsFixedUnit());
// If we get here we have exhausted our options and are not able to parse this interval
throw new IllegalArgumentException("Unable to parse interval [" + dateHistogramInterval + "]");
}
}
if (timeZone != null) {
Expand Down
10 changes: 10 additions & 0 deletions .../org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,16 @@ public void testLegacyThenNew() throws IOException {
assertWarnings("[interval] on [date_histogram] is deprecated, use [fixed_interval] or [calendar_interval] in the future.");
}

public void testIllegalInterval() throws IOException {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> testSearchCase(new MatchAllDocsQuery(),
Collections.emptyList(),
aggregation -> aggregation.dateHistogramInterval(new DateHistogramInterval("foobar")).field(DATE_FIELD),
histogram -> {}
));
assertThat(e.getMessage(), equalTo("Unable to parse interval [foobar]"));
assertWarnings("[interval] on [date_histogram] is deprecated, use [fixed_interval] or [calendar_interval] in the future.");
}

private void testSearchCase(Query query, List<String> dataset,
Consumer<DateHistogramAggregationBuilder> configure,
Consumer<InternalDateHistogram> verify) throws IOException {
Expand Down
8 changes: 2 additions & 6 deletions x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/rollup_search.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,9 +1246,7 @@ setup:

---
"Search error against live index":
- skip:
version: "all"
reason: "AwaitsFix: https://github.com/elastic/elasticsearch/issues/41970"

- do:
catch: bad_request
rollup.rollup_search:
Expand All @@ -1264,9 +1262,7 @@ setup:

---
"Search error against rollup and live index":
- skip:
version: "all"
reason: "AwaitsFix: https://github.com/elastic/elasticsearch/issues/41970"

- do:
catch: bad_request
rollup.rollup_search:
Expand Down