Skip to content
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

Remove timezone validation on rollup range queries #40647

Merged
merged 2 commits into from
Apr 2, 2019
Merged

Remove timezone validation on rollup range queries #40647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c6d9a2
Remove timezone validation on rollup range queries
polyfractal Mar 29, 2019
ac933b3
Merge remote-tracking branch 'origin/master' into rollup_remove_range…
polyfractal Apr 2, 2019
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
38 changes: 6 additions & 32 deletions ...llup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,14 @@
import org.elasticsearch.xpack.core.rollup.RollupField;
import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
import org.elasticsearch.xpack.core.rollup.action.RollupSearchAction;
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
import org.elasticsearch.xpack.rollup.Rollup;
import org.elasticsearch.xpack.rollup.RollupJobIdentifierUtils;
import org.elasticsearch.xpack.rollup.RollupRequestTranslator;
import org.elasticsearch.xpack.rollup.RollupResponseTranslator;
import org.joda.time.DateTimeZone;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -286,11 +283,8 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set<RollupJobCaps> jobCap
} else if (builder.getWriteableName().equals(RangeQueryBuilder.NAME)) {
RangeQueryBuilder range = (RangeQueryBuilder) builder;
String fieldName = range.fieldName();
// Many range queries don't include the timezone because the default is UTC, but the query
// builder will return null so we need to set it here
String timeZone = range.timeZone() == null ? DateTimeZone.UTC.toString() : range.timeZone();

String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName, timeZone);
String rewrittenFieldName = rewriteFieldName(jobCaps, RangeQueryBuilder.NAME, fieldName);
RangeQueryBuilder rewritten = new RangeQueryBuilder(rewrittenFieldName)
.from(range.from())
.to(range.to())
Expand All @@ -306,12 +300,12 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set<RollupJobCaps> jobCap
} else if (builder.getWriteableName().equals(TermQueryBuilder.NAME)) {
TermQueryBuilder term = (TermQueryBuilder) builder;
String fieldName = term.fieldName();
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null);
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName);
return new TermQueryBuilder(rewrittenFieldName, term.value());
} else if (builder.getWriteableName().equals(TermsQueryBuilder.NAME)) {
TermsQueryBuilder terms = (TermsQueryBuilder) builder;
String fieldName = terms.fieldName();
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName, null);
String rewrittenFieldName = rewriteFieldName(jobCaps, TermQueryBuilder.NAME, fieldName);
return new TermsQueryBuilder(rewrittenFieldName, terms.values());
} else if (builder.getWriteableName().equals(MatchAllQueryBuilder.NAME)) {
// no-op
Expand All @@ -321,11 +315,7 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set<RollupJobCaps> jobCap
}
}

private static String rewriteFieldName(Set<RollupJobCaps> jobCaps,
String builderName,
String fieldName,
String timeZone) {
List<String> incompatibleTimeZones = timeZone == null ? Collections.emptyList() : new ArrayList<>();
private static String rewriteFieldName(Set<RollupJobCaps> jobCaps, String builderName, String fieldName) {
List<String> rewrittenFieldNames = jobCaps.stream()
// We only care about job caps that have the query's target field
.filter(caps -> caps.getFieldCaps().keySet().contains(fieldName))
Expand All @@ -335,17 +325,7 @@ private static String rewriteFieldName(Set<RollupJobCaps> jobCaps,
// For now, we only allow filtering on grouping fields
.filter(agg -> {
String type = (String)agg.get(RollupField.AGG);

// If the cap is for a date_histo, and the query is a range, the timezones need to match
if (type.equals(DateHistogramAggregationBuilder.NAME) && timeZone != null) {
boolean matchingTZ = ((String)agg.get(DateHistogramGroupConfig.TIME_ZONE))
.equalsIgnoreCase(timeZone);
if (matchingTZ == false) {
incompatibleTimeZones.add((String)agg.get(DateHistogramGroupConfig.TIME_ZONE));
}
return matchingTZ;
}
// Otherwise just make sure it's one of the three groups
// make sure it's one of the three groups
return type.equals(TermsAggregationBuilder.NAME)
|| type.equals(DateHistogramAggregationBuilder.NAME)
|| type.equals(HistogramAggregationBuilder.NAME);
Expand All @@ -363,14 +343,8 @@ private static String rewriteFieldName(Set<RollupJobCaps> jobCaps,
.distinct()
.collect(ArrayList::new, List::addAll, List::addAll);
if (rewrittenFieldNames.isEmpty()) {
if (incompatibleTimeZones.isEmpty()) {
throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
+ "] query is not available in selected rollup indices, cannot query.");
} else {
throw new IllegalArgumentException("Field [" + fieldName + "] in [" + builderName
+ "] query was found in rollup indices, but requested timezone is not compatible. Options include: "
+ incompatibleTimeZones);
}
} else if (rewrittenFieldNames.size() > 1) {
throw new IllegalArgumentException("Ambiguous field name resolution when mapping to rolled fields. Field name [" +
fieldName + "] was mapped to: [" + Strings.collectionToDelimitedString(rewrittenFieldNames, ",") + "].");
Expand Down
9 changes: 4 additions & 5 deletions .../plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,15 @@ public void testRangeNullTimeZone() {
assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp"));
}

public void testRangeWrongTZ() {
public void testRangeDifferentTZ() {
final GroupConfig groupConfig = new GroupConfig(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h"), null, "UTC"));
final RollupJobConfig config = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, groupConfig, emptyList(), null);
RollupJobCaps cap = new RollupJobCaps(config);
Set<RollupJobCaps> caps = new HashSet<>();
caps.add(cap);
Exception e = expectThrows(IllegalArgumentException.class,
() -> TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps));
assertThat(e.getMessage(), equalTo("Field [foo] in [range] query was found in rollup indices, but requested timezone is not " +
"compatible. Options include: [UTC]"));
QueryBuilder rewritten = TransportRollupSearchAction.rewriteQuery(new RangeQueryBuilder("foo").gt(1).timeZone("CET"), caps);
assertThat(rewritten, instanceOf(RangeQueryBuilder.class));
assertThat(((RangeQueryBuilder)rewritten).fieldName(), equalTo("foo.date_histogram.timestamp"));
}

public void testTermQuery() {
Expand Down