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

Jump to bottom

handle empty sketches #7526

Merged
merged 4 commits into from
Apr 25, 2019
Merged

handle empty sketches #7526

merged 4 commits into from
Apr 25, 2019

Conversation

AlexanderSaydakov
Copy link
Contributor

this is to address #7486

@gianm
Copy link
Contributor

gianm commented Apr 23, 2019

Could you add a test for this please? (It seems like the kind of thing that's likely to get missed if the code is ever refactored, so a test is good to protect against that.)

@AlexanderSaydakov
Copy link
Contributor Author

Could you point me to the best example of a test for such a purpose? Should it be a micro-test (minimalistic unit test) or some sort of an integration test similar to what we do now in DoublesSketchAggregatorTest where we construct a json query and run it using the AggregationTestHelper?

@gianm
Copy link
Contributor

gianm commented Apr 23, 2019

@AlexanderSaydakov I am not intimately familiar with the code, but I'd guess that a minimalistic unit test would be best. In general I prefer unit tests when feasible.

gianm
gianm approved these changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM after CI - thanks @AlexanderSaydakov!

@AlexanderSaydakov
Copy link
Contributor Author

I don't understand what happened with the inspection:
Java HotSpot(TM) 64-Bit Server VM warning: Insufficient space for shared memory file:
[22:26:11] 30896

@jon-wei
Copy link
Contributor

jon-wei commented Apr 25, 2019

TeamCity inspection report doesn't show any new errors in the files affected by this patch, merging

@jon-wei jon-wei merged commit 9d8f934 into apache:master Apr 25, 2019
@clintropolis clintropolis mentioned this pull request Apr 25, 2019
Merged
clintropolis pushed a commit to clintropolis/druid that referenced this pull request Apr 25, 2019
@AlexanderSaydakov @clintropolis
handle empty sketches (apache#7526)
0408c6c 
* handle empty sketches

* return array of NaN in case of empty sketch

* noinspection ForLoopReplaceableByForEach in tests

* style fixes
@clintropolis clintropolis mentioned this pull request Apr 25, 2019
Closed
clintropolis added a commit that referenced this pull request Apr 25, 2019
@clintropolis
handle empty sketches (#7526) (#7552)
d4dde9c 
* handle empty sketches

* return array of NaN in case of empty sketch

* noinspection ForLoopReplaceableByForEach in tests

* style fixes
@jon-wei jon-wei mentioned this pull request Apr 26, 2019
Closed
@jihoonson jihoonson added this to the 0.14.1 milestone Apr 26, 2019
gianm pushed a commit to implydata/druid-public that referenced this pull request May 10, 2019
@clintropolis @gianm
handle empty sketches (apache#7526) (apache#7552)
bdd972d 
* handle empty sketches

* return array of NaN in case of empty sketch

* noinspection ForLoopReplaceableByForEach in tests

* style fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianm gianm gianm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AlexanderSaydakov @gianm @jon-wei @jihoonson