From 61b599d1f5eeaa39a8bc0bf123bda8ca535fb814 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 14 Mar 2018 14:53:29 -0700 Subject: [PATCH 1/2] SegmentMetadataQuery: Fix default interval handling. PR #4131 introduced a new copy builder for segmentMetadata that did not retain the value of usingDefaultInterval. This led to it being dropped and the default-interval handling not working as expected. Instead of using the default 1 week history when intervals are not provided, the segmentMetadata query would query _all_ segments, incurring an unexpected performance hit. This patch fixes the bug and adds a test for the copy builder. --- .../src/main/java/io/druid/query/Druids.java | 11 +++++- ...egmentMetadataQueryQueryToolChestTest.java | 37 +++++++++++++++++++ .../metadata/SegmentMetadataQueryTest.java | 3 ++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/io/druid/query/Druids.java b/processing/src/main/java/io/druid/query/Druids.java index 1576c01a1c79..e235a99d0220 100644 --- a/processing/src/main/java/io/druid/query/Druids.java +++ b/processing/src/main/java/io/druid/query/Druids.java @@ -591,6 +591,7 @@ public static class SegmentMetadataQueryBuilder private EnumSet analysisTypes; private Boolean merge; private Boolean lenientAggregatorMerge; + private Boolean usingDefaultInterval; private Map context; public SegmentMetadataQueryBuilder() @@ -601,6 +602,7 @@ public SegmentMetadataQueryBuilder() analysisTypes = null; merge = null; lenientAggregatorMerge = null; + usingDefaultInterval = null; context = null; } @@ -613,7 +615,7 @@ public SegmentMetadataQuery build() merge, context, analysisTypes, - false, + usingDefaultInterval, lenientAggregatorMerge ); } @@ -627,6 +629,7 @@ public static SegmentMetadataQueryBuilder copy(SegmentMetadataQuery query) .analysisTypes(query.getAnalysisTypes()) .merge(query.isMerge()) .lenientAggregatorMerge(query.isLenientAggregatorMerge()) + .usingDefaultInterval(query.isUsingDefaultInterval()) .context(query.getContext()); } @@ -696,6 +699,12 @@ public SegmentMetadataQueryBuilder lenientAggregatorMerge(boolean lenientAggrega return this; } + public SegmentMetadataQueryBuilder usingDefaultInterval(boolean usingDefaultInterval) + { + this.usingDefaultInterval = usingDefaultInterval; + return this; + } + public SegmentMetadataQueryBuilder context(Map c) { context = c; diff --git a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java index 6a0e3c42a42b..d7c0910e668f 100644 --- a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java +++ b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java @@ -27,6 +27,7 @@ import io.druid.jackson.DefaultObjectMapper; import io.druid.java.util.common.Intervals; import io.druid.query.CacheStrategy; +import io.druid.query.Druids; import io.druid.query.TableDataSource; import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.DoubleMaxAggregatorFactory; @@ -38,10 +39,15 @@ import io.druid.query.metadata.metadata.SegmentMetadataQuery; import io.druid.query.spec.LegacySegmentSpec; import io.druid.segment.column.ValueType; +import io.druid.timeline.LogicalSegment; +import org.joda.time.Interval; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Test; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; public class SegmentMetadataQueryQueryToolChestTest { @@ -271,6 +277,37 @@ public void testMergeAggregatorsConflict() ); } + @Test + public void testFilterSegments() + { + final SegmentMetadataQueryConfig config = new SegmentMetadataQueryConfig(); + final SegmentMetadataQueryQueryToolChest toolChest = new SegmentMetadataQueryQueryToolChest(config); + + final List filteredSegments = toolChest.filterSegments( + Druids.newSegmentMetadataQueryBuilder().dataSource("foo").merge(true).build(), + ImmutableList + .of( + "2000-01-01/P1D", + "2000-01-04/P1D", + "2000-01-09/P1D", + "2000-01-09/P1D" + ) + .stream() + .map(interval -> (LogicalSegment) () -> new Interval(interval)) + .collect(Collectors.toList()) + ); + + Assert.assertEquals(Period.weeks(1), config.getDefaultHistory()); + Assert.assertEquals( + ImmutableList.of( + new Interval("2000-01-04/P1D"), + new Interval("2000-01-09/P1D"), + new Interval("2000-01-09/P1D") + ), + filteredSegments.stream().map(LogicalSegment::getInterval).collect(Collectors.toList()) + ); + } + @SuppressWarnings("ArgumentParameterSwap") @Test public void testMergeRollup() diff --git a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryTest.java b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryTest.java index 6e688c7968bf..16eaedd9b272 100644 --- a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryTest.java +++ b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryTest.java @@ -895,6 +895,9 @@ public void testSerdeWithDefaultInterval() throws Exception // test serialize and deserialize Assert.assertEquals(query, MAPPER.readValue(MAPPER.writeValueAsString(query), Query.class)); + + // test copy + Assert.assertEquals(query, Druids.SegmentMetadataQueryBuilder.copy((SegmentMetadataQuery) query).build()); } @Test From 6d5011830c399cf276f9162803862bfabb8c7769 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Wed, 14 Mar 2018 16:09:15 -0700 Subject: [PATCH 2/2] Intervals --- .../metadata/SegmentMetadataQueryQueryToolChestTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java index d7c0910e668f..359e39f6af2b 100644 --- a/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java +++ b/processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChestTest.java @@ -40,7 +40,6 @@ import io.druid.query.spec.LegacySegmentSpec; import io.druid.segment.column.ValueType; import io.druid.timeline.LogicalSegment; -import org.joda.time.Interval; import org.joda.time.Period; import org.junit.Assert; import org.junit.Test; @@ -293,16 +292,16 @@ public void testFilterSegments() "2000-01-09/P1D" ) .stream() - .map(interval -> (LogicalSegment) () -> new Interval(interval)) + .map(interval -> (LogicalSegment) () -> Intervals.of(interval)) .collect(Collectors.toList()) ); Assert.assertEquals(Period.weeks(1), config.getDefaultHistory()); Assert.assertEquals( ImmutableList.of( - new Interval("2000-01-04/P1D"), - new Interval("2000-01-09/P1D"), - new Interval("2000-01-09/P1D") + Intervals.of("2000-01-04/P1D"), + Intervals.of("2000-01-09/P1D"), + Intervals.of("2000-01-09/P1D") ), filteredSegments.stream().map(LogicalSegment::getInterval).collect(Collectors.toList()) );