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

Search: Fix aggregation ordering in search results #8100

Merged
merged 1 commit into from
Nov 23, 2017

Conversation

nickdaugherty
Copy link
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 To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 2, 2017
@gibrown
Copy link
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
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
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
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 To request a review from fellow Jetpack developers. Label will be renamed soon. 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