Skip to content

Search: Fix aggregation ordering in search results#8100

Merged
oskosk merged 1 commit intomasterfrom
8096/search-fix-aggregation-ordering
Nov 23, 2017
Merged

Search: Fix aggregation ordering in search results#8100
oskosk merged 1 commit intomasterfrom
8096/search-fix-aggregation-ordering

Conversation

@nickdaugherty
Copy link
Copy Markdown
Contributor

Elasticsearch doesn’t guarantee the order of returned Aggregations will
match the input order, which leads to unexpected behavior when
rendering the Aggregations in the Widget.

Adds code to re-order the Aggregation results to match the ordering as
passed to Jetpack_Search::set_filters().

Fixes #8096

Elasticsearch doesn’t guarantee the order of returned Aggregations will
match the input order, which leads to unexpected behavior when
rendering the Aggregations in the Widget.

Adds code to re-order the Aggregation results to match the ordering as
passed to `Jetpack_Search::set_filters()`.

Fixes #8096
@nickdaugherty nickdaugherty requested a review from a team as a code owner November 2, 2017 21:03
@nickdaugherty nickdaugherty added the [Feature] Search For all things related to Search label Nov 2, 2017
@nickdaugherty nickdaugherty changed the title Fix aggregation ordering in search results Search: Fix aggregation ordering in search results Nov 2, 2017
@nickdaugherty nickdaugherty added the [Status] Needs Review This PR is ready for review. label Nov 2, 2017
@gibrown
Copy link
Copy Markdown
Member

gibrown commented Nov 2, 2017

Can't we have the code doing the display just use the aggregation name rather than relying on the order?

@nickdaugherty
Copy link
Copy Markdown
Contributor Author

The internals of the Search code combines the originally specified Aggregations into the Aggregation results array, so that there is a single array that contains both the settings as specified in Jetpack_Search::set_filters(), and the resulting 'buckets' returned by ES. This all happens in Jetpack_Search::get_filters(), which is what the Widget ends up calling before running a foreach() on them.

It would be possible to re-sort them at the Widget level, but it's better having them in the 'expected' order (as defined by site developer) everywhere, so it's consistent regardless of how the module is used - in both the Widget, but also any custom code that builds upon Jetpack_Search::get_filters() directly.

@nickdaugherty
Copy link
Copy Markdown
Contributor Author

Also I should give a demo of the problem. Say you do this:

Jetpack_Search::instance()->set_filters(array(
		'Month' => array(
			'type'     => 'date_histogram',
			'field'    => 'post_date',
			'interval' => 'month',
			'count'    => 10,
		),
		'Year' => array(
			'type'     => 'date_histogram',
			'field'    => 'post_date',
			'interval' => 'year',
			'count'    => 10,
		),
		'Categories' => array(
			'type'     => 'taxonomy',
			'taxonomy' => 'category',
			'count'    => 10,
		),
	) );

The Aggregations that get returned in the ES result aren't necessarily in the same order, so you could end up with an array like this:

array(
    'Year' => array( ... ),
    'Categories' => array( ... ),
    'Month' => array( ... ),
)

Then when the Widget renders, the ordering is 'off' and it's not possible to control the display order.

Copy link
Copy Markdown
Member

@gibrown gibrown left a comment

Choose a reason for hiding this comment

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

Ahh, I understand now. Thanks. LGTM

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 6, 2017
@oskosk oskosk added this to the 5.6 milestone Nov 23, 2017
@oskosk oskosk merged commit 5e8ef49 into master Nov 23, 2017
@oskosk oskosk deleted the 8096/search-fix-aggregation-ordering branch November 23, 2017 19:04
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 23, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Search For all things related to Search Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants