From cdb39cf5dc50e97f53c04fdaf269f718c0134fe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 29 Jul 2015 18:51:36 +0200 Subject: [PATCH] Aggregations: Fix setting timezone on default DateTime formatter This PR prevents setting timezone on ValueFormatter.DateTime. Instead the timezone information needed when printing buckets key-as-string information is provided at constrution time of the ValueFormatter, making sure we don't overwrite any constants. This, however, made it necessary to be able to access the timezone information when resolving the format in ValueSourseParser, so the `time_zone` parameter is now parsed alongside the `format` parameter in ValueSourceParser rather than in DateHistogramParser. Closes #12531 --- .../common/rounding/TimeZoneRounding.java | 3 ++ .../bucket/histogram/DateHistogramParser.java | 17 ++----- .../support/ValuesSourceParser.java | 49 +++++++++++++++---- .../support/format/ValueFormat.java | 11 +++-- .../support/format/ValueFormatter.java | 22 +++++---- 5 files changed, 64 insertions(+), 38 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java b/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java index 107324949db..1e6bbb6f339 100644 --- a/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java +++ b/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java @@ -64,6 +64,9 @@ public abstract class TimeZoneRounding extends Rounding { } public Builder timeZone(DateTimeZone timeZone) { + if (timeZone == null) { + throw new IllegalArgumentException("Setting null as timezone is not supported"); + } this.timeZone = timeZone; return this; } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java index 2381dc6e302..ae2ab8a5124 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java @@ -33,10 +33,7 @@ import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.aggregations.support.ValuesSourceParser; -import org.elasticsearch.search.aggregations.support.format.ValueFormatter.DateTime; import org.elasticsearch.search.internal.SearchContext; -import org.joda.time.DateTimeZone; - import java.io.IOException; /** @@ -45,7 +42,6 @@ import java.io.IOException; public class DateHistogramParser implements Aggregator.Parser { static final ParseField EXTENDED_BOUNDS = new ParseField("extended_bounds"); - static final ParseField TIME_ZONE = new ParseField("time_zone"); static final ParseField OFFSET = new ParseField("offset"); static final ParseField INTERVAL = new ParseField("interval"); @@ -83,6 +79,7 @@ public class DateHistogramParser implements Aggregator.Parser { ValuesSourceParser vsParser = ValuesSourceParser.numeric(aggregationName, InternalDateHistogram.TYPE, context) .targetValueType(ValueType.DATE) .formattable(true) + .timezoneAware(true) .build(); boolean keyed = false; @@ -90,7 +87,6 @@ public class DateHistogramParser implements Aggregator.Parser { ExtendedBounds extendedBounds = null; InternalOrder order = (InternalOrder) Histogram.Order.KEY_ASC; String interval = null; - DateTimeZone timeZone = DateTimeZone.UTC; long offset = 0; XContentParser.Token token; @@ -101,9 +97,7 @@ public class DateHistogramParser implements Aggregator.Parser { } else if (vsParser.token(currentFieldName, token, parser)) { continue; } else if (token == XContentParser.Token.VALUE_STRING) { - if (context.parseFieldMatcher().match(currentFieldName, TIME_ZONE)) { - timeZone = DateTimeZone.forID(parser.text()); - } else if (context.parseFieldMatcher().match(currentFieldName, OFFSET)) { + if (context.parseFieldMatcher().match(currentFieldName, OFFSET)) { offset = parseOffset(parser.text()); } else if (context.parseFieldMatcher().match(currentFieldName, INTERVAL)) { interval = parser.text(); @@ -121,8 +115,6 @@ public class DateHistogramParser implements Aggregator.Parser { } else if (token == XContentParser.Token.VALUE_NUMBER) { if ("min_doc_count".equals(currentFieldName) || "minDocCount".equals(currentFieldName)) { minDocCount = parser.longValue(); - } else if ("time_zone".equals(currentFieldName) || "timeZone".equals(currentFieldName)) { - timeZone = DateTimeZone.forOffsetHours(parser.intValue()); } else { throw new SearchParseException(context, "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].", parser.getTokenLocation()); @@ -193,13 +185,10 @@ public class DateHistogramParser implements Aggregator.Parser { } Rounding rounding = tzRoundingBuilder - .timeZone(timeZone) + .timeZone(vsParser.input().timezone()) .offset(offset).build(); ValuesSourceConfig config = vsParser.config(); - if (config.formatter()!=null) { - ((DateTime) config.formatter()).setTimeZone(timeZone); - } return new HistogramAggregator.Factory(aggregationName, config, rounding, order, keyed, minDocCount, extendedBounds, new InternalDateHistogram.Factory()); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParser.java index cb70e896baa..4e3c022f94f 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParser.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceParser.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.support; import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; @@ -39,6 +40,7 @@ import org.elasticsearch.search.SearchParseException; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.support.format.ValueFormat; import org.elasticsearch.search.internal.SearchContext; +import org.joda.time.DateTimeZone; import java.io.IOException; import java.util.Map; @@ -50,6 +52,8 @@ import static com.google.common.collect.Maps.newHashMap; */ public class ValuesSourceParser { + static final ParseField TIME_ZONE = new ParseField("time_zone"); + public static Builder any(String aggName, InternalAggregation.Type aggType, SearchContext context) { return new Builder<>(aggName, aggType, context, ValuesSource.class); } @@ -66,14 +70,19 @@ public class ValuesSourceParser { return new Builder<>(aggName, aggType, context, ValuesSource.GeoPoint.class).targetValueType(ValueType.GEOPOINT).scriptable(false); } - private static class Input { - String field = null; - Script script = null; + public static class Input { + private String field = null; + private Script script = null; @Deprecated - Map params = null; // TODO Remove in 3.0 - ValueType valueType = null; - String format = null; - Object missing = null; + private Map params = null; // TODO Remove in 3.0 + private ValueType valueType = null; + private String format = null; + private Object missing = null; + private DateTimeZone timezone = DateTimeZone.UTC; + + public DateTimeZone timezone() { + return this.timezone; + } } private final String aggName; @@ -83,6 +92,7 @@ public class ValuesSourceParser { private boolean scriptable = true; private boolean formattable = false; + private boolean timezoneAware = false; private ValueType targetValueType = null; private ScriptParameterParser scriptParameterParser = new ScriptParameterParser(); @@ -105,6 +115,8 @@ public class ValuesSourceParser { input.field = parser.text(); } else if (formattable && "format".equals(currentFieldName)) { input.format = parser.text(); + } else if (timezoneAware && context.parseFieldMatcher().match(currentFieldName, TIME_ZONE)) { + input.timezone = DateTimeZone.forID(parser.text()); } else if (scriptable) { if ("value_type".equals(currentFieldName) || "valueType".equals(currentFieldName)) { input.valueType = ValueType.resolveForScript(parser.text()); @@ -123,6 +135,14 @@ public class ValuesSourceParser { } return true; } + if (token == XContentParser.Token.VALUE_NUMBER) { + if (timezoneAware && context.parseFieldMatcher().match(currentFieldName, TIME_ZONE)) { + input.timezone = DateTimeZone.forOffsetHours(parser.intValue()); + } else { + return false; + } + return true; + } if (scriptable && token == XContentParser.Token.START_OBJECT) { if (context.parseFieldMatcher().match(currentFieldName, ScriptField.SCRIPT)) { input.script = Script.parse(parser, context.parseFieldMatcher()); @@ -203,7 +223,7 @@ public class ValuesSourceParser { config.fieldContext = new FieldContext(input.field, indexFieldData, fieldType); config.missing = input.missing; config.script = createScript(); - config.format = resolveFormat(input.format, fieldType); + config.format = resolveFormat(input.format, input.timezone, fieldType); return config; } @@ -222,9 +242,9 @@ public class ValuesSourceParser { return valueFormat; } - private static ValueFormat resolveFormat(@Nullable String format, MappedFieldType fieldType) { + private static ValueFormat resolveFormat(@Nullable String format, @Nullable DateTimeZone timezone, MappedFieldType fieldType) { if (fieldType instanceof DateFieldMapper.DateFieldType) { - return format != null ? ValueFormat.DateTime.format(format) : ValueFormat.DateTime.mapper((DateFieldMapper.DateFieldType) fieldType); + return format != null ? ValueFormat.DateTime.format(format, timezone) : ValueFormat.DateTime.mapper((DateFieldMapper.DateFieldType) fieldType, timezone); } if (fieldType instanceof IpFieldMapper.IpFieldType) { return ValueFormat.IPv4; @@ -238,6 +258,10 @@ public class ValuesSourceParser { return ValueFormat.RAW; } + public Input input() { + return this.input; + } + public static class Builder { private final ValuesSourceParser parser; @@ -256,6 +280,11 @@ public class ValuesSourceParser { return this; } + public Builder timezoneAware(boolean timezoneAware) { + parser.timezoneAware = timezoneAware; + return this; + } + public Builder targetValueType(ValueType valueType) { parser.targetValueType = valueType; return this; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormat.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormat.java index c309be0a9c7..33cf9cc5099 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormat.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormat.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.support.format; import org.elasticsearch.index.mapper.core.DateFieldMapper; +import org.joda.time.DateTimeZone; /** * @@ -67,12 +68,12 @@ public class ValueFormat { public static final DateTime DEFAULT = new DateTime(DateFieldMapper.Defaults.DATE_TIME_FORMATTER.format(), ValueFormatter.DateTime.DEFAULT, ValueParser.DateMath.DEFAULT); - public static DateTime format(String format) { - return new DateTime(format, new ValueFormatter.DateTime(format), new ValueParser.DateMath(format)); + public static DateTime format(String format, DateTimeZone timezone) { + return new DateTime(format, new ValueFormatter.DateTime(format, timezone), new ValueParser.DateMath(format)); } - public static DateTime mapper(DateFieldMapper.DateFieldType fieldType) { - return new DateTime(fieldType.dateTimeFormatter().format(), ValueFormatter.DateTime.mapper(fieldType), ValueParser.DateMath.mapper(fieldType)); + public static DateTime mapper(DateFieldMapper.DateFieldType fieldType, DateTimeZone timezone) { + return new DateTime(fieldType.dateTimeFormatter().format(), ValueFormatter.DateTime.mapper(fieldType, timezone), ValueParser.DateMath.mapper(fieldType)); } public DateTime(String pattern, ValueFormatter formatter, ValueParser parser) { @@ -81,7 +82,7 @@ public class ValueFormat { @Override public DateTime create(String pattern) { - return format(pattern); + return format(pattern, DateTimeZone.UTC); } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormatter.java b/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormatter.java index 0427210afc1..8316e917bd9 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormatter.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/support/format/ValueFormatter.java @@ -33,7 +33,6 @@ import java.text.DecimalFormat; import java.text.DecimalFormatSymbols; import java.text.NumberFormat; import java.util.Locale; -import java.util.TimeZone; /** * A strategy for formatting time represented as millis long value to string @@ -61,7 +60,6 @@ public interface ValueFormatter extends Streamable { String format(long value); /** - * The * @param value double The double value to format. * @return The formatted value as string */ @@ -104,8 +102,8 @@ public interface ValueFormatter extends Streamable { public static final ValueFormatter DEFAULT = new ValueFormatter.DateTime(DateFieldMapper.Defaults.DATE_TIME_FORMATTER); private DateTimeZone timeZone = DateTimeZone.UTC; - public static DateTime mapper(DateFieldMapper.DateFieldType fieldType) { - return new DateTime(fieldType.dateTimeFormatter()); + public static DateTime mapper(DateFieldMapper.DateFieldType fieldType, DateTimeZone timezone) { + return new DateTime(fieldType.dateTimeFormatter(), timezone); } static final byte ID = 2; @@ -122,15 +120,21 @@ public interface ValueFormatter extends Streamable { this.formatter = formatter; } + public DateTime(String format, DateTimeZone timezone) { + this.formatter = Joda.forPattern(format); + this.timeZone = timezone != null ? timezone : DateTimeZone.UTC; + } + + public DateTime(FormatDateTimeFormatter formatter, DateTimeZone timezone) { + this.formatter = formatter; + this.timeZone = timezone != null ? timezone : DateTimeZone.UTC; + } + @Override public String format(long time) { return formatter.printer().withZone(timeZone).print(time); } - public void setTimeZone(DateTimeZone timeZone) { - this.timeZone = timeZone; - } - @Override public String format(double value) { return format((long) value); @@ -264,7 +268,7 @@ public interface ValueFormatter extends Streamable { } } - + static class BooleanFormatter implements ValueFormatter { static final byte ID = 10;