Skip to content

Conversation

@iverase
Copy link
Contributor

@iverase iverase commented Sep 17, 2020

Make sure the the new HLL++ is different to the original one by not merging the original and just add a few values to the new one.

closes #62540

@iverase iverase added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.0.0 v7.10.0 labels Sep 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 17, 2020
@iverase
Copy link
Contributor Author

iverase commented Sep 17, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-windows

2 similar comments
@iverase
Copy link
Contributor Author

iverase commented Sep 17, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-windows

@iverase
Copy link
Contributor Author

iverase commented Sep 17, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-windows

Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

Left a comment about a maybe-future tweak, but LGTM as it stands right now :)

int extraValues = between(10, 100);
for (int i = 0; i < extraValues; i++) {
for (int i = 0; i < 10; i++) {
newState.collect(0, BitMixer.mix64(randomIntBetween(500, 10000)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't looked closely at the test, but is there a way to ensure we add new values that weren't previously added? Maybe by picking from a different range of random values or something? Or keeping a set?

Maybe it's a non-issue and random enough to not matter... might be a problem for a different day :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does use a different range of values, Still for precision 4 in the previous approach you could get the same sketch at the end :( I think this way is very very unlikely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha. Sounds good!

@iverase iverase merged commit 9e5da6e into elastic:master Sep 17, 2020
@iverase iverase deleted the fixCardinalityTest branch September 17, 2020 14:22
iverase added a commit that referenced this pull request Sep 17, 2020
) (#62554)

Make sure the the new HLL++ is different to the original one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v7.10.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InternalCardinalityTests#testEqualsAndHashcode failure

4 participants