-
Notifications
You must be signed in to change notification settings - Fork 733
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
Changes from 7 commits
714756d
3e58dae
f68022a
3be8893
9cec59b
512a2f1
dd8dbc1
848bd8c
984ac60
2f51705
00ac03a
a4038f6
5821d84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
{ | ||
parent::__construct($name); | ||
|
||
if (null !== $bucketsPath) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the if condition is now always |
||
$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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} |
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; | ||
} | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolassing ^^
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)