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

Conversation

@colings86
Copy link
Member

commented Mar 30, 2017

Relates to #22278

@colings86 colings86 self-assigned this Mar 30, 2017

@colings86 colings86 requested a review from jpountz Mar 30, 2017

@martijnvg martijnvg referenced this pull request Mar 30, 2017
63 of 63 tasks complete
@jpountz
Copy link
Contributor

left a comment

I left some minor comments.

...main/java/org/elasticsearch/search/aggregations/metrics/cardinality/HyperLogLogPlusPlus.java Outdated
}
} else {
for (long i = 0; i < runLens.size(); i++) {
values.add(runLens.get(i));

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2017

Contributor

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

...main/java/org/elasticsearch/search/aggregations/metrics/cardinality/HyperLogLogPlusPlus.java Outdated
public boolean equals(long bucket, HyperLogLogPlusPlus other) {
return Objects.equals(p, other.p) &&
Objects.equals(algorithm.get(bucket), other.algorithm.get(bucket))
&&

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2017

Contributor

weird indentation?

...main/java/org/elasticsearch/search/aggregations/metrics/cardinality/HyperLogLogPlusPlus.java Outdated
}
OpenBitSet other = (OpenBitSet) obj;
return Objects.equals(impl, other.impl);
}

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2017

Contributor

It seems to me that this is not needed anymore?

core/src/test/java/org/elasticsearch/search/aggregations/bucket/FilterAggregatorTests.java Outdated
for (int i = 0; i < numDocs; i++) {
if (frequently()) {
// make sure we have more than one segment to test the merge
indexWriter.commit();

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2017

Contributor

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;

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2017

Contributor

this variable never seems to be read?

This comment has been minimized.

Copy link
@colings86

colings86 Mar 31, 2017

Author Member

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).

colings86 added 2 commits Mar 29, 2017

@colings86 colings86 force-pushed the colings86:tests/aggUnitTests branch to 3a44442 Mar 31, 2017

@jpountz
jpountz approved these changes Apr 3, 2017
Copy link
Contributor

left a comment

LGTM

@colings86 colings86 merged commit 058869e into elastic:master Apr 3, 2017

1 of 2 checks passed

elasticsearch-ci Build finished.
Details
CLA Commit author has signed the CLA
Details
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
Merge branch 'master' into single-node-discovery
* 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
Merge branch 'master' into single-node-bootstrap-checks
* 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
Projects
None yet
2 participants
You can’t perform that action at this time.