Skip to content

SegmentMetadataQuery support for returning aggregators.#2295

Merged
fjy merged 1 commit intoapache:masterfrom
gianm:segment-metadata-query-query-metadata
Jan 22, 2016
Merged

SegmentMetadataQuery support for returning aggregators.#2295
fjy merged 1 commit intoapache:masterfrom
gianm:segment-metadata-query-query-metadata

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Jan 20, 2016

No description provided.

fjy
fjy reviewed Jan 20, 2016
View reviewed changes
Comment thread processing/src/main/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java
Copy link
Copy Markdown
Contributor

@fjy fjy Jan 20, 2016

Choose a reason for hiding this comment

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

is it never possible to have any overlap here?

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jan 20, 2016

Choose a reason for hiding this comment

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

Different rows in different segments count as two rows, so there can't be overlap

Copy link
Copy Markdown
Contributor

@fjy fjy Jan 20, 2016

Choose a reason for hiding this comment

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

nvm, this is just copied from deleted code

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jan 20, 2016

👍

@gianm gianm force-pushed the segment-metadata-query-query-metadata branch from c9bc98d to 776981f Compare January 20, 2016 01:25
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Jan 20, 2016

hmm, one thing I did in this patch is make null + something = something, rather than null. The idea was that for users that have "merge": true to see a summary, and a mix of some segments with metadata and some without (because maybe they just upgraded to 0.9.0) it's better to show something rather than nothing.

Metadata.merge on the other hand treats null + something = null. maybe that difference is confusing and I should change it. Or maybe it should be an option.

any thoughts anyone?

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Jan 20, 2016

OK I changed it

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Jan 20, 2016

actually I'm gonna add a flag to make both modes possible. will reopen.

@gianm gianm closed this Jan 20, 2016
@gianm gianm reopened this Jan 20, 2016
@gianm gianm force-pushed the segment-metadata-query-query-metadata branch 2 times, most recently from c8309fb to b09848f Compare January 20, 2016 19:04
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Jan 20, 2016

reopened with lenientAggregatorMerge flag

@gianm gianm force-pushed the segment-metadata-query-query-metadata branch from b09848f to 8f2bcb9 Compare January 20, 2016 19:16
@fjy fjy added this to the 0.9.0 milestone Jan 20, 2016
fjy
fjy reviewed Jan 20, 2016
View reviewed changes
Comment thread processing/src/main/java/io/druid/query/metadata/SegmentMetadataQueryQueryToolChest.java
Copy link
Copy Markdown
Contributor

@fjy fjy Jan 20, 2016

Choose a reason for hiding this comment

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

why not just make this an empty list? easier to read the following code

Copy link
Copy Markdown
Contributor

@fjy fjy Jan 20, 2016

Choose a reason for hiding this comment

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

oh, you might rely on newIntervals to be a null for a check later on I guess

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jan 20, 2016

Still 👍

himanshug
himanshug reviewed Jan 21, 2016
View reviewed changes
Comment thread processing/src/main/java/io/druid/query/metadata/metadata/SegmentAnalysis.java
Copy link
Copy Markdown
Contributor

@himanshug himanshug Jan 21, 2016

Choose a reason for hiding this comment

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

not all aggregators might implement equals() correctly, we should probably put a comment here saying this is best-effort only.

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jan 21, 2016

Choose a reason for hiding this comment

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

added comments

Copy link
Copy Markdown
Contributor Author

@gianm gianm Jan 21, 2016

Choose a reason for hiding this comment

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

to equals & hashCode

@himanshug
Copy link
Copy Markdown
Contributor

himanshug commented Jan 21, 2016

👍 besides #2295 (comment)

lenient merging is ok for the UI usecase (also its configurable so doesn't really matter for other use cases)

@gianm gianm force-pushed the segment-metadata-query-query-metadata branch from 8f2bcb9 to f2c7cdb Compare January 21, 2016 22:15
@gianm gianm force-pushed the segment-metadata-query-query-metadata branch from f2c7cdb to d416279 Compare January 22, 2016 01:27
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Jan 22, 2016

rebased against master

fjy added a commit that referenced this pull request Jan 22, 2016
@fjy
Merge pull request #2295 from gianm/segment-metadata-query-query-meta…
f03b654 
…data

SegmentMetadataQuery support for returning aggregators.
@fjy fjy merged commit f03b654 into apache:master Jan 22, 2016
@himanshug himanshug mentioned this pull request Jan 22, 2016
Merged
@fjy fjy added the Improvement label Feb 5, 2016
@gianm gianm deleted the segment-metadata-query-query-metadata branch February 23, 2016 01:06
@gianm gianm mentioned this pull request Dec 24, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Improvement

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@gianm @fjy @himanshug