Skip to content

Improper getter value is fixed.#6930

Merged
clintropolis merged 2 commits intoapache:masterfrom
kamaci:feature/aggregator-getter
Feb 7, 2019
Merged

Improper getter value is fixed.#6930
clintropolis merged 2 commits intoapache:masterfrom
kamaci:feature/aggregator-getter

Conversation

@kamaci
Copy link
Copy Markdown
Member

@kamaci kamaci commented Jan 28, 2019

No description provided.

clintropolis
clintropolis reviewed Jan 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

Nice catch 👍

Is it worth adding a "serde" test?

@kamaci
Copy link
Copy Markdown
Member Author

kamaci commented Jan 29, 2019

Do you mean adding a method to ApproximateHistogramPostAggregatorTest which extends BucketsPostAggregator?

@leventov
Copy link
Copy Markdown
Member

leventov commented Jan 29, 2019

Didn't you find this bug using some sort of inspection or static analysis?

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jan 29, 2019

Do you mean adding a method to ApproximateHistogramPostAggregatorTest which extends BucketsPostAggregator?

I'd suggest adding a BucketsPostAggregatorTest that has a testSerde() method similar to ConstantPostAggregatorTest.

@kamaci
Copy link
Copy Markdown
Member Author

kamaci commented Jan 29, 2019

@gianm We can merge this PR if it is OK and I can create another PR which includes all tests for BucketsPostAggregatorTest?

@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Jan 29, 2019

I think the serde unit test should be included here because it fixes a bug.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jan 29, 2019

I agree with @jihoonson, it is best to add a unit test in the same patch that fixes a bug.

@kamaci
Copy link
Copy Markdown
Member Author

kamaci commented Jan 29, 2019

👍

@jon-wei jon-wei added this to the 0.14.0 milestone Feb 1, 2019
@kamaci
Copy link
Copy Markdown
Member Author

kamaci commented Feb 6, 2019

Do you mean adding a method to ApproximateHistogramPostAggregatorTest which extends BucketsPostAggregator?

I'd suggest adding a BucketsPostAggregatorTest that has a testSerde() method similar to ConstantPostAggregatorTest.

I've updated my PR.

clintropolis
clintropolis approved these changes Feb 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the test! 👍 after CI

@kamaci
Copy link
Copy Markdown
Member Author

kamaci commented Feb 7, 2019

CI checks are OK 👍

@clintropolis clintropolis merged commit 3097562 into apache:master Feb 7, 2019
clintropolis pushed a commit to clintropolis/druid that referenced this pull request Feb 7, 2019
@kamaci @clintropolis
Improper getter value is fixed. (apache#6930)
ed77fcb 
* Improper getter value is fixed.

* Test class is added.
@clintropolis clintropolis mentioned this pull request Feb 7, 2019
Merged
fjy pushed a commit that referenced this pull request Feb 8, 2019
@clintropolis @fjy
Improper getter value is fixed. (#6930) (#7033)
da03057 
* Improper getter value is fixed.

* Test class is added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clintropolis clintropolis clintropolis approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

0.14.0

Development

Successfully merging this pull request may close these issues.

6 participants

@kamaci @leventov @gianm @jihoonson @clintropolis @jon-wei