Skip to content

support multiple intervals in dataSource inputSpec#1988

Merged
gianm merged 3 commits intoapache:masterfrom
himanshug:multi-interval-batch-delta
Dec 4, 2015
Merged

support multiple intervals in dataSource inputSpec#1988
gianm merged 3 commits intoapache:masterfrom
himanshug:multi-interval-batch-delta

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

so that users can read data from multiple intervals from input dataSource

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

list of strings

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use full words too!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@himanshug himanshug force-pushed the multi-interval-batch-delta branch 2 times, most recently from 37a0499 to 60bcd10 Compare November 22, 2015 20:15
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Nov 24, 2015

👍

@himanshug himanshug added this to the 0.9.0 milestone Dec 2, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could potentially return the same segments multiple times, I think. Is that bad?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm thinking of the case where two of the intervals in the list both partially overlap the same segment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or even if two intervals in the list overlap each other.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps add a test for that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch.
but, it wouldn't matter because list of intervals is always list of "disjoint" intervals ensured by calling JodaUtils.condenseIntervals(..). In case of 2 intervals overlapping same segment, this would have given same segment twice, which again wouldn't matter because caller uses "windowed" segments appropriately.
however, that looks weird from api perspective so updated the code to remove duplicates and also updated the test case to verify same.

@himanshug himanshug force-pushed the multi-interval-batch-delta branch from 60bcd10 to 49f6cd7 Compare December 3, 2015 04:58
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Dec 3, 2015

@himanshug looks like a legitimate ci failure. Could you fix that and squash the commits? 👍 from me after that, everything else looks good

[ERROR] /home/travis/build/druid-io/druid/indexing-service/src/test/java/io/druid/indexing/test/TestIndexerMetadataStorageCoordinator.java:[35,8] io.druid.indexing.test.TestIndexerMetadataStorageCoordinator is not abstract and does not override abstract method getUsedSegmentsForIntervals(java.lang.String,java.util.List<org.joda.time.Interval>) in io.druid.indexing.overlord.IndexerMetadataStorageCoordinator

@himanshug himanshug force-pushed the multi-interval-batch-delta branch from 49f6cd7 to 61aaa09 Compare December 4, 2015 03:29
@himanshug
Copy link
Copy Markdown
Contributor Author

@gianm ah that happened due to rebase not updating a new class. fixed

gianm added a commit that referenced this pull request Dec 4, 2015
support multiple intervals in dataSource inputSpec
@gianm gianm merged commit 20544d4 into apache:master Dec 4, 2015
@himanshug himanshug deleted the multi-interval-batch-delta branch December 5, 2015 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants