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.

Sign up for GitHub

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 support for multiple aggregations #2682

Merged
merged 4 commits into from
Mar 29, 2022
Merged

Add support for multiple aggregations #2682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
235fd74
Add support for multiple aggregations
felipeelia Mar 27, 2022
3ed4750
Merge branch 'develop' into feature/multiple-aggs
felipeelia Mar 28, 2022
f3df048
Improve check for numeric array keys
felipeelia Mar 29, 2022
26356af
Merge branch 'develop' into feature/multiple-aggs
felipeelia Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add support for multiple aggregations
  • Loading branch information
@felipeelia
felipeelia committed Mar 27, 2022
commit 235fd7438a11cea97a5f9fde87cac5e2de422f03
56 changes: 39 additions & 17 deletions includes/classes/Indexable/Post/Post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1800,24 +1800,15 @@ public function format_args( $args, $wp_query ) {
/**
* Aggregations
*/
if ( isset( $args['aggs'] ) && ! empty( $args['aggs']['aggs'] ) ) {
$agg_obj = $args['aggs'];

// Add a name to the aggregation if it was passed through
if ( ! empty( $agg_obj['name'] ) ) {
$agg_name = $agg_obj['name'];
} else {
$agg_name = 'aggregation_name';
}

// Add/use the filter if warranted
if ( isset( $agg_obj['use-filter'] ) && false !== $agg_obj['use-filter'] && $use_filters ) {

// If a filter is being used, use it on the aggregation as well to receive relevant information to the query
$formatted_args['aggs'][ $agg_name ]['filter'] = $filter;
$formatted_args['aggs'][ $agg_name ]['aggs'] = $agg_obj['aggs'];
if ( isset( $args['aggs'] ) ) {
// An array of aggregations.
if ( isset( $args['aggs'][0] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to check if this is an array with is_array() ? Right now I don't think it would do any harm, but i'm thinking if there's ever a case where we offer an aggregations filter for example, and people can manipulate the array indexes. If there's not $args['aggs'][0] but there's $args['aggs'][1] this code will not run.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made the changes we discussed in slack @oscarssanchez . Do you mind giving yet another look at it? Thanks!

foreach ( $args['aggs'] as $agg ) {
$formatted_args = $this->apply_aggregations( $formatted_args, $agg, $use_filters, $filter );
}
} else {
$formatted_args['aggs'][ $agg_name ] = $agg_obj['aggs'];
// Single aggregation.
$formatted_args = $this->apply_aggregations( $formatted_args, $args['aggs'], $use_filters, $filter );
}
}

Expand Down Expand Up @@ -2224,4 +2215,35 @@ protected function determine_mapping_version_based_on_existing( $mapping, $index

return 'unknown';
}

/**
* Given ES args, add aggregations to it.
*
* @since 4.1.0
* @param array $formatted_args Formatted Elasticsearch query.
* @param array $agg Aggregation data.
* @param boolean $use_filters Whether filters should be used or not.
* @param array $filter Filters defined so far.
* @return array Formatted Elasticsearch query with the aggregation added.
*/
protected function apply_aggregations( $formatted_args, $agg, $use_filters, $filter ) {
if ( empty( $agg['aggs'] ) ) {
return $formatted_args;
}

// Add a name to the aggregation if it was passed through
$agg_name = ( ! empty( $agg['name'] ) ) ? $agg['name'] : 'aggregation_name';

// Add/use the filter if warranted
if ( isset( $agg['use-filter'] ) && false !== $agg['use-filter'] && $use_filters ) {

// If a filter is being used, use it on the aggregation as well to receive relevant information to the query
$formatted_args['aggs'][ $agg_name ]['filter'] = $filter;
$formatted_args['aggs'][ $agg_name ]['aggs'] = $agg['aggs'];
} else {
$formatted_args['aggs'][ $agg_name ] = $agg['aggs'];
}

return $formatted_args;
}
}
32 changes: 32 additions & 0 deletions tests/php/indexables/TestPost.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5939,6 +5939,38 @@ public function testFormatArgsAggs() {
);

$this->assertSame( 'terms.post_type', $args['aggs']['aggregation_name']['terms']['field'] );

// Multiple aggs.
$args = $post->format_args(
[
// Triggers $use_filter to be true.
'post_status' => 'publish',

'aggs' => [
[
'name' => 'taxonomies',
'use-filter' => true,
'aggs' => [
'terms' => [
'field' => 'terms.category.slug',
],
],
],
[
'aggs' => [
'terms' => [
'field' => 'terms.post_type',
],
],
]
],
],
new \WP_Query()
);

$this->assertSame( 'publish', $args['aggs']['taxonomies']['filter']['bool']['must'][1]['term']['post_status'] );
$this->assertSame( 'terms.category.slug', $args['aggs']['taxonomies']['aggs']['terms']['field'] );
$this->assertSame( 'terms.post_type', $args['aggs']['aggregation_name']['terms']['field'] );
}

/**
Expand Down