From 068ca67dbaa44164af5e816ed49a7c1b0764d95c Mon Sep 17 00:00:00 2001 From: xvrl Date: Mon, 21 Jan 2013 14:21:01 -0800 Subject: [PATCH 1/7] fix cache not preserving timezone information --- .../query/timeseries/TimeseriesQueryQueryToolChest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java index 99bf679c006..0e5d96b385c 100644 --- a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java +++ b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java @@ -191,7 +191,8 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest retVal = Lists.newArrayListWithCapacity(1 + aggs.size()); - retVal.add(input.getTimestamp().getMillis()); + // make sure to preserve timezone information when caching results + retVal.add(input.getTimestamp()); for (AggregatorFactory agg : aggs) { retVal.add(results.getMetric(agg.getName())); } @@ -215,7 +216,7 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest aggsIter = aggs.iterator(); Iterator resultIter = results.iterator(); - DateTime timestamp = new DateTime(resultIter.next()); + DateTime timestamp = (DateTime)resultIter.next(); while (aggsIter.hasNext() && resultIter.hasNext()) { final AggregatorFactory factory = aggsIter.next(); retVal.put(factory.getName(), factory.deserialize(resultIter.next())); From f05c050c53d4c7d61d7097d8f225c6028d79fd23 Mon Sep 17 00:00:00 2001 From: xvrl Date: Mon, 21 Jan 2013 15:49:39 -0800 Subject: [PATCH 2/7] add test for timezone --- .../timeseries/TimeseriesQueryRunnerTest.java | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java b/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java index b6479983634..550e918eb27 100644 --- a/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java +++ b/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java @@ -40,6 +40,7 @@ import com.metamx.druid.query.segment.MultipleIntervalSegmentSpec; import com.metamx.druid.result.Result; import com.metamx.druid.result.TimeseriesResultValue; import org.joda.time.DateTime; +import org.joda.time.DateTimeZone; import org.joda.time.Interval; import org.joda.time.Period; import org.junit.Assert; @@ -257,6 +258,55 @@ public class TimeseriesQueryRunnerTest TestHelper.assertExpectedResults(expectedResults, results); } + @Test + public void testTimeseriesWithTimeZone() + { + TimeseriesQuery query = Druids.newTimeseriesQueryBuilder() + .dataSource(QueryRunnerTestHelper.dataSource) + .intervals(QueryRunnerTestHelper.firstToThird) + .aggregators( + Arrays.asList( + QueryRunnerTestHelper.rowsCount, + new LongSumAggregatorFactory( + "idx", + "index" + ) + ) + ) + .granularity(new PeriodGranularity(new Period("P1D"), null, DateTimeZone.forID("America/Los_Angeles"))) + .build(); + + List> expectedResults = Arrays.asList( + new Result( + new DateTime("2011-03-31", DateTimeZone.forID("America/Los_Angeles")), + new TimeseriesResultValue( + ImmutableMap.of("rows", 13L, "idx", 6619L) + ) + ), + new Result( + new DateTime("2011-04-01T", DateTimeZone.forID("America/Los_Angeles")), + new TimeseriesResultValue( + ImmutableMap.of("rows", 13L, "idx", 5827L) + ) + ), + new Result( + new DateTime("2011-04-02", DateTimeZone.forID("America/Los_Angeles")), + new TimeseriesResultValue( + ImmutableMap.of("rows", 0L, "idx", 0L) + ) + ) + ); + + Iterable> results = Sequences.toList( + runner.run(query), + Lists.>newArrayList() + ); + + TestHelper.assertExpectedResults(expectedResults, results); + + + } + @Test public void testTimeseriesWithVaryingGran() From 8f38b775ae1aa7f6d044f7760e6618304b6f5788 Mon Sep 17 00:00:00 2001 From: xvrl Date: Mon, 21 Jan 2013 16:31:32 -0800 Subject: [PATCH 3/7] fix expected object type --- .../query/timeseries/TimeseriesQueryQueryToolChest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java index 0e5d96b385c..108d82e0266 100644 --- a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java +++ b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java @@ -49,6 +49,7 @@ import org.joda.time.DateTime; import org.joda.time.Interval; import org.joda.time.Minutes; import org.joda.time.Period; +import org.joda.time.format.ISODateTimeFormat; import javax.annotation.Nullable; import java.nio.ByteBuffer; @@ -216,7 +217,10 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest aggsIter = aggs.iterator(); Iterator resultIter = results.iterator(); - DateTime timestamp = (DateTime)resultIter.next(); + DateTime timestamp = ISODateTimeFormat.dateTimeParser() + .withOffsetParsed() + .parseDateTime(resultIter.next().toString()); + while (aggsIter.hasNext() && resultIter.hasNext()) { final AggregatorFactory factory = aggsIter.next(); retVal.put(factory.getName(), factory.deserialize(resultIter.next())); From d7ea8e9afcc3be41ef826da7743051248ecdf7de Mon Sep 17 00:00:00 2001 From: xvrl Date: Mon, 21 Jan 2013 17:01:41 -0800 Subject: [PATCH 4/7] compare result timestamp based on millis + utcoffset --- client/src/main/java/com/metamx/druid/result/Result.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/main/java/com/metamx/druid/result/Result.java b/client/src/main/java/com/metamx/druid/result/Result.java index 9fc5aebf347..9b055cd4800 100644 --- a/client/src/main/java/com/metamx/druid/result/Result.java +++ b/client/src/main/java/com/metamx/druid/result/Result.java @@ -71,7 +71,7 @@ public class Result implements Comparable> Result result = (Result) o; - if (timestamp != null ? !timestamp.equals(result.timestamp) : result.timestamp != null) { + if (timestamp != null ? !(timestamp.isEqual(result.timestamp) && timestamp.getZone().getOffset(timestamp) == result.timestamp.getZone().getOffset(result.timestamp)) : result.timestamp != null) { return false; } if (value != null ? !value.equals(result.value) : result.value != null) { From 35058786d9eb96a6a443c9182e4845c1bc2b08bd Mon Sep 17 00:00:00 2001 From: xvrl Date: Tue, 22 Jan 2013 19:31:31 -0800 Subject: [PATCH 5/7] match query interval to granularity for this test --- .../query/timeseries/TimeseriesQueryRunnerTest.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java b/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java index 550e918eb27..3be91c3686f 100644 --- a/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java +++ b/server/src/test/java/com/metamx/druid/query/timeseries/TimeseriesQueryRunnerTest.java @@ -263,7 +263,7 @@ public class TimeseriesQueryRunnerTest { TimeseriesQuery query = Druids.newTimeseriesQueryBuilder() .dataSource(QueryRunnerTestHelper.dataSource) - .intervals(QueryRunnerTestHelper.firstToThird) + .intervals("2011-03-31T00:00:00-07:00/2011-04-02T00:00:00-07:00") .aggregators( Arrays.asList( QueryRunnerTestHelper.rowsCount, @@ -288,12 +288,6 @@ public class TimeseriesQueryRunnerTest new TimeseriesResultValue( ImmutableMap.of("rows", 13L, "idx", 5827L) ) - ), - new Result( - new DateTime("2011-04-02", DateTimeZone.forID("America/Los_Angeles")), - new TimeseriesResultValue( - ImmutableMap.of("rows", 0L, "idx", 0L) - ) ) ); @@ -303,11 +297,8 @@ public class TimeseriesQueryRunnerTest ); TestHelper.assertExpectedResults(expectedResults, results); - - } - @Test public void testTimeseriesWithVaryingGran() { From 55ae4c87dde33e4dc5127c41da945c63987e1d8c Mon Sep 17 00:00:00 2001 From: xvrl Date: Tue, 22 Jan 2013 10:59:05 -0800 Subject: [PATCH 6/7] timezone support in groupby query --- .../group/GroupByQueryQueryToolChest.java | 15 +++++- .../com/metamx/druid/input/MapBasedRow.java | 46 +++++++++++++++---- .../druid/query/group/GroupByQueryEngine.java | 3 +- .../GroupByTimeseriesQueryRunnerTest.java | 2 +- 4 files changed, 54 insertions(+), 12 deletions(-) diff --git a/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java b/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java index abe5610732d..913ef5e57ce 100644 --- a/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java +++ b/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java @@ -32,6 +32,7 @@ import com.metamx.druid.Query; import com.metamx.druid.aggregation.AggregatorFactory; import com.metamx.druid.index.v1.IncrementalIndex; import com.metamx.druid.initialization.Initialization; +import com.metamx.druid.input.MapBasedRow; import com.metamx.druid.input.Row; import com.metamx.druid.input.Rows; import com.metamx.druid.query.CacheStrategy; @@ -119,7 +120,19 @@ public class GroupByQueryQueryToolChest implements QueryToolChest() + { + @Override + public Row apply(@Nullable Row input) + { + final MapBasedRow row = (MapBasedRow) input; + return new MapBasedRow(query.getGranularity().toDateTime(row.getTimestampFromEpoch()), row.getEvent()); + } + } + ); } }; } diff --git a/common/src/main/java/com/metamx/druid/input/MapBasedRow.java b/common/src/main/java/com/metamx/druid/input/MapBasedRow.java index 03c56e2f66d..d823762eddd 100644 --- a/common/src/main/java/com/metamx/druid/input/MapBasedRow.java +++ b/common/src/main/java/com/metamx/druid/input/MapBasedRow.java @@ -37,7 +37,7 @@ import java.util.Map; */ public class MapBasedRow implements Row { - private final long timestamp; + private final DateTime timestamp; private final Map event; @JsonCreator @@ -46,22 +46,21 @@ public class MapBasedRow implements Row @JsonProperty("event") Map event ) { - this(timestamp.getMillis(), event); + this.timestamp = timestamp; + this.event = event; } public MapBasedRow( long timestamp, Map event - ) - { - this.timestamp = timestamp; - this.event = event; + ) { + this(new DateTime(timestamp), event); } @Override public long getTimestampFromEpoch() { - return timestamp; + return timestamp.getMillis(); } @Override @@ -120,7 +119,7 @@ public class MapBasedRow implements Row @JsonProperty public DateTime getTimestamp() { - return new DateTime(timestamp); + return timestamp; } @JsonProperty @@ -133,9 +132,38 @@ public class MapBasedRow implements Row public String toString() { return "MapBasedRow{" + - "timestamp=" + new DateTime(timestamp) + + "timestamp=" + timestamp + ", event=" + event + '}'; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + MapBasedRow that = (MapBasedRow) o; + + if (!event.equals(that.event)) { + return false; + } + if (!timestamp.equals(that.timestamp)) { + return false; + } + + return true; + } + + @Override + public int hashCode() + { + int result = timestamp.hashCode(); + result = 31 * result + event.hashCode(); + return result; + } } diff --git a/server/src/main/java/com/metamx/druid/query/group/GroupByQueryEngine.java b/server/src/main/java/com/metamx/druid/query/group/GroupByQueryEngine.java index 788e1adb02a..cb9a1468b80 100644 --- a/server/src/main/java/com/metamx/druid/query/group/GroupByQueryEngine.java +++ b/server/src/main/java/com/metamx/druid/query/group/GroupByQueryEngine.java @@ -43,6 +43,7 @@ import com.metamx.druid.index.v1.processing.DimensionSelector; import com.metamx.druid.input.MapBasedRow; import com.metamx.druid.input.Row; import com.metamx.druid.query.dimension.DimensionSpec; +import org.joda.time.DateTime; import org.joda.time.Interval; import javax.annotation.Nullable; @@ -347,7 +348,7 @@ public class GroupByQueryEngine .transform( new Function, Row>() { - private final long timestamp = cursor.getTime().getMillis(); + private final DateTime timestamp = cursor.getTime(); private final int[] increments = positionMaintainer.getIncrements(); @Override diff --git a/server/src/test/java/com/metamx/druid/query/group/GroupByTimeseriesQueryRunnerTest.java b/server/src/test/java/com/metamx/druid/query/group/GroupByTimeseriesQueryRunnerTest.java index 0860715963b..8a1c7637367 100644 --- a/server/src/test/java/com/metamx/druid/query/group/GroupByTimeseriesQueryRunnerTest.java +++ b/server/src/test/java/com/metamx/druid/query/group/GroupByTimeseriesQueryRunnerTest.java @@ -112,7 +112,7 @@ public class GroupByTimeseriesQueryRunnerTest extends TimeseriesQueryRunnerTest MapBasedRow row = (MapBasedRow) input; return new Result( - new DateTime(input.getTimestampFromEpoch()), new TimeseriesResultValue(row.getEvent()) + row.getTimestamp(), new TimeseriesResultValue(row.getEvent()) ); } } From ee7337fbb9a88e297f7f585f4632089be6f7da85 Mon Sep 17 00:00:00 2001 From: Eric Tschetter Date: Thu, 24 Jan 2013 18:25:21 -0600 Subject: [PATCH 7/7] 1) Adjust the Timeseries caching fixes to still store the long, but do the timezone adjustment on the way out. 2) Store a reference to the granularity object instead of getting it every time --- .../druid/query/group/GroupByQueryQueryToolChest.java | 7 +++++-- .../timeseries/TimeseriesQueryQueryToolChest.java | 10 +++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java b/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java index 913ef5e57ce..42f1940218c 100644 --- a/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java +++ b/client/src/main/java/com/metamx/druid/query/group/GroupByQueryQueryToolChest.java @@ -29,6 +29,7 @@ import com.metamx.common.guava.ConcatSequence; import com.metamx.common.guava.Sequence; import com.metamx.common.guava.Sequences; import com.metamx.druid.Query; +import com.metamx.druid.QueryGranularity; import com.metamx.druid.aggregation.AggregatorFactory; import com.metamx.druid.index.v1.IncrementalIndex; import com.metamx.druid.initialization.Initialization; @@ -125,11 +126,13 @@ public class GroupByQueryQueryToolChest implements QueryToolChest() { + private final QueryGranularity granularity = query.getGranularity(); + @Override - public Row apply(@Nullable Row input) + public Row apply(Row input) { final MapBasedRow row = (MapBasedRow) input; - return new MapBasedRow(query.getGranularity().toDateTime(row.getTimestampFromEpoch()), row.getEvent()); + return new MapBasedRow(granularity.toDateTime(row.getTimestampFromEpoch()), row.getEvent()); } } ); diff --git a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java index 108d82e0266..df619f340f0 100644 --- a/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java +++ b/client/src/main/java/com/metamx/druid/query/timeseries/TimeseriesQueryQueryToolChest.java @@ -28,6 +28,7 @@ import com.metamx.common.guava.MergeSequence; import com.metamx.common.guava.Sequence; import com.metamx.common.guava.nary.BinaryFn; import com.metamx.druid.Query; +import com.metamx.druid.QueryGranularity; import com.metamx.druid.ResultGranularTimestampComparator; import com.metamx.druid.TimeseriesBinaryFn; import com.metamx.druid.aggregation.AggregatorFactory; @@ -192,8 +193,7 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest retVal = Lists.newArrayListWithCapacity(1 + aggs.size()); - // make sure to preserve timezone information when caching results - retVal.add(input.getTimestamp()); + retVal.add(input.getTimestamp().getMillis()); for (AggregatorFactory agg : aggs) { retVal.add(results.getMetric(agg.getName())); } @@ -208,6 +208,8 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest>() { + private final QueryGranularity granularity = query.getGranularity(); + @Override public Result apply(@Nullable Object input) { @@ -217,9 +219,7 @@ public class TimeseriesQueryQueryToolChest implements QueryToolChest aggsIter = aggs.iterator(); Iterator resultIter = results.iterator(); - DateTime timestamp = ISODateTimeFormat.dateTimeParser() - .withOffsetParsed() - .parseDateTime(resultIter.next().toString()); + DateTime timestamp = granularity.toDateTime(((Number) resultIter.next()).longValue()); while (aggsIter.hasNext() && resultIter.hasNext()) { final AggregatorFactory factory = aggsIter.next();