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

add Extended Stats Bucket #1756

Merged
merged 13 commits into from
Mar 5, 2020
54 changes: 54 additions & 0 deletions src/Aggregation/ExtendedStatsBucket.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php

namespace Elastica\Aggregation;

use Elastica\Exception\InvalidException;

/**
* Implements a Extended Stats Bucket Aggregation.
*
* @see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-pipeline-extended-stats-bucket-aggregation.html
*/
class ExtendedStatsBucket extends AbstractAggregation
{
public function __construct(string $name, ?string $bucketsPath = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed it now: why is the bucketPath not required in the constructor, but later required when building the request( toArray())?

As it is required, I'd make it required (not empty) and remove the setBucketPath() method. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

@thePanz Perhaps the same argument as in the other PR might apply. No strong opinion which on is better but we should become consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i followed the model of the StatsBucket aggregation :/

But both of you are right. We can make the $bucketsPath argument mandatory and remove the setter in the 2 classes :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nicolassing : I'd leave the setters (and do the "not empty" check there, throwing an exception)
and make the argument required in the constructor

I'll cleanup the other aggregations @ruflin , but in another PR. I'd like not to have devs creating invalid objects to start with :) but this is a topic for a discussion, not here tho :)

Copy link
Owner

Choose a reason for hiding this comment

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

SGTM, lets try it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But first: let's have the $bucketsPath argument as required :)

{
parent::__construct($name);

if (null !== $bucketsPath) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the if condition is now always true

$this->setBucketsPath($bucketsPath);
}
}

public function setBucketsPath(string $bucketsPath): self
{
return $this->setParam('buckets_path', $bucketsPath);
}

public function setGapPolicy(string $gapPolicy): self
{
return $this->setParam('gap_policy', $gapPolicy);
}

public function setFormat(string $format): self
{
return $this->setParam('format', $format);
}

public function setSigma(int $sigma): self
{
return $this->setParam('sigma', $sigma);
}

/**
* @throws InvalidException If buckets path or script is not set
*/
public function toArray(): array
{
if (!$this->hasParam('buckets_path')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes ! I was correcting it ^^

throw new InvalidException('Buckets path is required');
}

return parent::toArray();
}
}
96 changes: 96 additions & 0 deletions tests/Aggregation/ExtendedStatsBucketTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<?php

namespace Elastica\Test\Aggregation;

use Elastica\Aggregation\ExtendedStatsBucket;
use Elastica\Aggregation\Histogram;
use Elastica\Aggregation\Max;
use Elastica\Document;
use Elastica\Exception\InvalidException;
use Elastica\Index;
use Elastica\Query;

/**
* @internal
*/
class ExtendedStatsBucketTest extends BaseAggregationTest
{
/**
* @group functional
*/
public function testExtendedStatBucketAggregation(): void
{
$bucketScriptAggregation = new ExtendedStatsBucket('result', 'age_groups>max_weight');

$histogramAggregation = new Histogram('age_groups', 'age', 10);

$histogramAggregation->addAggregation((new Max('max_weight'))->setField('weight'));

$query = Query::create([])
->addAggregation($histogramAggregation)
->addAggregation($bucketScriptAggregation)
;

$results = $this->_getIndexForTest()->search($query)->getAggregation('result');

$this->assertEquals(3, $results['count']);
$this->assertEquals(50, $results['min']);
$this->assertEquals(70, $results['max']);
$this->assertEquals(60, $results['avg']);
$this->assertEquals(180, $results['sum']);
$this->assertEquals(11000, $results['sum_of_squares']);
$this->assertEquals(66.66666666666667, $results['variance']);
$this->assertEquals(8.16496580927726, $results['std_deviation']);
$this->assertEquals(['upper' => 76.32993161855453, 'lower' => 43.670068381445475], $results['std_deviation_bounds']);
}

/**
* @group unit
*/
public function testConstructThroughSetters(): void
{
$serialDiffAgg = new ExtendedStatsBucket('bucket_part');

$serialDiffAgg
->setBucketsPath('age_groups>max_weight')
->setFormat('test_format')
->setGapPolicy(10)
;

$expected = [
'extended_stats_bucket' => [
'buckets_path' => 'age_groups>max_weight',
'format' => 'test_format',
'gap_policy' => 10,
],
];

$this->assertEquals($expected, $serialDiffAgg->toArray());
}

/**
* @group unit
*/
public function testToArrayInvalidBucketsPath(): void
{
$this->expectException(InvalidException::class);

$serialDiffAgg = new ExtendedStatsBucket('bucket_part');
$serialDiffAgg->toArray();
}

private function _getIndexForTest(): Index
{
$index = $this->_createIndex();

$index->addDocuments([
Document::create(['weight' => 60, 'height' => 180, 'age' => 25]),
Document::create(['weight' => 70, 'height' => 156, 'age' => 32]),
Document::create(['weight' => 50, 'height' => 155, 'age' => 45]),
]);

$index->refresh();

return $index;
}
}