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.

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

Adds tests for cardinality and filter aggregations #23826

Merged
merged 2 commits into from
Apr 3, 2017
Merged

Adds tests for cardinality and filter aggregations #23826

merged 2 commits into from
Apr 3, 2017

Conversation

colings86
Copy link
Contributor

Relates to #22278

@colings86 colings86 added review >test Issues or PRs that are addressing/adding tests v6.0.0-alpha1 labels Mar 30, 2017
@colings86 colings86 self-assigned this Mar 30, 2017
@colings86 colings86 requested a review from jpountz March 30, 2017 15:36
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left some minor comments.

}
} else {
for (long i = 0; i < runLens.size(); i++) {
values.add(runLens.get(i));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should be s/i/(bucket << p)+i/

public boolean equals(long bucket, HyperLogLogPlusPlus other) {
return Objects.equals(p, other.p) &&
Objects.equals(algorithm.get(bucket), other.algorithm.get(bucket))
&&
Copy link
Contributor

Choose a reason for hiding this comment

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

weird indentation?

}
OpenBitSet other = (OpenBitSet) obj;
return Objects.equals(impl, other.impl);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that this is not needed anymore?

for (int i = 0; i < numDocs; i++) {
if (frequently()) {
// make sure we have more than one segment to test the merge
indexWriter.commit();
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do indexWriter.getReader().close(), which will create a segment too but not do a fsync

import java.util.Map;

public class InternalCardinalityTests extends InternalAggregationTestCase<InternalCardinality> {
private static List<HyperLogLogPlusPlus> algos;
Copy link
Contributor

Choose a reason for hiding this comment

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

this variable never seems to be read?

Copy link
Contributor Author

@colings86 colings86 Mar 31, 2017

Choose a reason for hiding this comment

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

This is needed so we keep a reference to the HyperLogLogPlusPlus implementations we create in createTestInstance() so we can close them and release the appropriate BigArray pages after the test is complete (see the cleanup() method).

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM

@colings86 colings86 merged commit 058869e into elastic:master Apr 3, 2017
colings86 added a commit that referenced this pull request Apr 3, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2017
* master: (137 commits)
  CONSOLEify the "using scripts" documentation
  Adds tests for cardinality and filter aggregations
  Add Backoff policy to azure repository
  Revert "Adds tests for cardinality and filter aggregations (elastic#23826)"
  Adds tests for cardinality and filter aggregations (elastic#23826)
  mute testDifferentRolesMaintainPathOnRestart
  Replace custom sort field with SortedSetSortField and SortedNumericSortField when possible (elastic#23827)
  Prevent nodes from joining if newer indices exist in the cluster (elastic#23843)
  Synchronized CollapseTopFieldDocs with lucenes relatives (elastic#23854)
  CONSOLEify analysis docs
  Adapted search_shards rest test to work with Perl
  To examine an exception in rest tests, the exception should be caught, not ignored
  Fixed bad YAML in rest tests
  Fix BootstrapForTesting blowup
  Ban Boolean#getBoolean
  Fix language in some docs
  CONSOLEify lang-analyzer docs
  Stricter parsing of remote node attribute
  Fix cross-cluster remote node gateway attributes
  FieldCapabilitiesRequest should implements Replaceable since it accepts index patterns
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 4, 2017
* master: (146 commits)
  Introduce single-node discovery
  Await termination after shutting down executors
  Fix initialization issue in ElasticsearchException
  Fix bulk queue size in thread pool docs
  [DOCS] Remove line about eager loading global ordinals
  GceDiscoverTests - remove intitial_state_timeout
  SpecificMasterNodesIT shouldn't use autoMinMasterNodes
  testRestorePersistentSettings doesn't to mess with discovery settings
  testDifferentRolesMaintainPathOnRestart shouldn't use auto managing of min master nodes
  CONSOLEify the "using scripts" documentation
  Adds tests for cardinality and filter aggregations
  Add Backoff policy to azure repository
  Revert "Adds tests for cardinality and filter aggregations (elastic#23826)"
  Adds tests for cardinality and filter aggregations (elastic#23826)
  mute testDifferentRolesMaintainPathOnRestart
  Replace custom sort field with SortedSetSortField and SortedNumericSortField when possible (elastic#23827)
  Prevent nodes from joining if newer indices exist in the cluster (elastic#23843)
  Synchronized CollapseTopFieldDocs with lucenes relatives (elastic#23854)
  CONSOLEify analysis docs
  Adapted search_shards rest test to work with Perl
  ...
javanna added a commit to javanna/elasticsearch that referenced this pull request May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test Issues or PRs that are addressing/adding tests v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants