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

Option to configure default analysis types in SegmentMetadataQuery #4259

Merged
merged 23 commits into from
May 26, 2017
Merged

Option to configure default analysis types in SegmentMetadataQuery #4259

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ddb6565
Option to configure default analysis types
gkc2104 May 4, 2017
4103a6c
Updated Docs and renamed
gkc2104 May 9, 2017
9891b09
Added serde tests and Null handling
gkc2104 May 9, 2017
81e7464
Fixed Documentation
gkc2104 May 9, 2017
6c9cde2
Updated implementation
gkc2104 May 11, 2017
a756d4f
Merge branch 'master' of https://github.com/druid-io/druid into defau…
gkc2104 May 11, 2017
dfacab1
Updated implementation
gkc2104 May 11, 2017
98db5a7
Updated implementation
gkc2104 May 11, 2017
7313175
Added usingDefaultIntervals in Builder
gkc2104 May 11, 2017
6612a4a
Updated implementation
gkc2104 May 11, 2017
639661f
Updated implementation and added failing test
gkc2104 May 11, 2017
cb98ac3
filterSegments implementation updated
gkc2104 May 12, 2017
5529f6a
Merge branch 'master' of https://github.com/druid-io/druid into defau…
gkc2104 May 12, 2017
b544694
Updated imlementation
gkc2104 May 16, 2017
f68927e
Padding
leventov May 16, 2017
aee4da9
Merge remote-tracking branch 'upstream/master' into defaultAnalysisUp…
leventov May 16, 2017
6e1a11c
Add missing Override
leventov May 16, 2017
bd2324f
Updated implementation
gkc2104 May 24, 2017
1871c92
Hmm
gkc2104 May 24, 2017
6433796
Fixed a naming bug
gkc2104 May 24, 2017
7096d9a
Fixed bug
gkc2104 May 24, 2017
8aecbde
Merge branch 'defaultAnalysisUpdate' of https://github.com/metamx/dru…
gkc2104 May 24, 2017
b7f586c
Removed comment
gkc2104 May 24, 2017
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
27 changes: 21 additions & 6 deletions processing/src/main/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ public SegmentMetadataQueryQueryToolChest(SegmentMetadataQueryConfig config)
}

@Inject
public SegmentMetadataQueryQueryToolChest(SegmentMetadataQueryConfig config, GenericQueryMetricsFactory queryMetricsFactory)
public SegmentMetadataQueryQueryToolChest(
SegmentMetadataQueryConfig config,
GenericQueryMetricsFactory queryMetricsFactory
)
{
this.config = config;
this.queryMetricsFactory = queryMetricsFactory;
Expand Down Expand Up @@ -233,18 +236,30 @@ public SegmentAnalysis apply(@Nullable SegmentAnalysis input)
@Override
public <T extends LogicalSegment> List<T> filterSegments(SegmentMetadataQuery query, List<T> segments)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of the changes in this method? They don't seem related to the rest of the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are unrelated to the changes proposed for this patch, I'll open up another PR for this.
The current implementation does not take into consideration the time range while filtering, I added an improvement to only include segments that are within the intervals mentioned by the query.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation does not take into consideration the time range while filtering, I added an improvement to only include segments that are within the intervals mentioned by the query.

filterSegments doesn't need to worry about that, since the list of segments it gets has already been pruned down to whatever matches the intervals filter. In fact many implementations just return segments; with no changes -- check out timeseries for an example. filterSegments only has to potentially filter it down even further, if it wants (see timeBoundary for another example of that). I suppose the javadocs for filterSegments could be more clear on this.

Please also see #4259 (review) for rationale about the current behavior of defaultInterval.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that this method was called to prune the segments to include only the required ones from a list of all segments, didn't know it was already pruned. Now that I look at the examples you suggested I understand why it is required even if we already prune it.

Reg: defaultInterval - I had some failing tests that were because of defaultInterval, I don't remember the exact test case, but reverting to the old behavior did not break any tests (probably the change of not having null configs helped I guess), so we can keep the implementation on master

I excluded these changes from this PR.

{
if (!query.isUsingDefaultInterval()) {
return segments;
}

if (segments.size() <= 1) {
return segments;
}

final T max = segments.get(segments.size() - 1);

DateTime targetEnd = max.getInterval().getEnd();
final Interval targetInterval = new Interval(config.getDefaultHistory(), targetEnd);
List<Interval> intervals = query.getIntervals();

DateTime queryStartTime = JodaUtils.ETERNITY.getEnd();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other parts of the Druid codebase new DateTime(JodaUtils.MAX_INSTANT) is used

DateTime queryEndTIme = JodaUtils.ETERNITY.getStart();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryEndTime


for (Interval interval : intervals) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider

queryStartTime = intervals.stream().map(Interval::getEnd)
    .max(Ordering.natural()).orElseThrow(IllegalStateException::new);

queryEndTIme = queryEndTIme.isAfter(interval.getEnd()) ? queryEndTIme : interval.getEnd();
queryStartTime = queryStartTime.isBefore(interval.getStart()) ? queryStartTime : interval.getStart();
}

Interval targetInterval;
if (!query.isUsingDefaultInterval()) {
targetInterval = new Interval(queryStartTime, queryEndTIme);
} else {
DateTime finalEndTime = queryEndTIme.isBefore(targetEnd) ? queryEndTIme : targetEnd;
targetInterval = new Interval(config.getDefaultHistory(), finalEndTime);
}

return Lists.newArrayList(
Iterables.filter(
Expand Down
20 changes: 10 additions & 10 deletions processing/src/main/java/io/druid/query/metadata/metadata/SegmentMetadataQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,8 @@ public SegmentMetadataQuery(
if (querySegmentSpec == null) {
this.usingDefaultInterval = true;
} else {
if (querySegmentSpec.getIntervals().size() == 1 && querySegmentSpec.getIntervals()
.get(0)
.equals(DEFAULT_INTERVAL)) {
this.usingDefaultInterval = true;
} else {
this.usingDefaultInterval = false;
}
this.usingDefaultInterval = (querySegmentSpec.getIntervals().size() == 1 &&
querySegmentSpec.getIntervals().get(0).equals(DEFAULT_INTERVAL));
}
this.toInclude = toInclude == null ? new AllColumnIncluderator() : toInclude;
this.merge = merge == null ? false : merge;
Expand Down Expand Up @@ -262,9 +257,14 @@ public SegmentMetadataQuery withFinalizedAnalysisTypes(SegmentMetadataQueryConfi
return this;
}
return Druids.SegmentMetadataQueryBuilder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a new query object if analysisTypes are already non-null, just return this

.copy(this)
.analysisTypes(config.getDefaultAnalysisTypes())
.build();
.copy(this)
.analysisTypes(config.getDefaultAnalysisTypes())
.build();
}

public List<Interval> getIntervals()
{
return this.getQuerySegmentSpec().getIntervals();
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions processing/src/test/java/io/druid/query/metadata/SegmentMetadataQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,9 +1104,8 @@ public Interval getInterval()
Assert.assertEquals(expectedSegments3.get(i).getInterval(), filteredSegments2.get(i).getInterval());
}

Assert.assertFalse(testQuery2.isUsingDefaultInterval());
Assert.assertTrue(testQuery.isUsingDefaultInterval());

Assert.assertFalse(testQuery2.isUsingDefaultInterval());

}

Expand Down