fix(facetOrdering): make sure ordered facets is a dense array #879
Conversation
Otherwise the "undefined" values (are ordered, but not in the results) take up place and it messes up the limits.
This caused the number of items in limit to be lower than the possible returned results, as sparse elements in an array still "take up space" algolia/algoliasearch-helper-js#879
| @@ -779,3 +779,22 @@ describe('hierarchical facet', function() { | |||
| expect(facetValues).toEqual(expected); | |||
| }); | |||
| }); | |||
|
|
|||
| test('customer issue', function() { | |||
| var rawResults = require('./getFacetValues/sparse.json'); | |||
There was a problem hiding this comment.
Where are the undefined values in the dataset?
There was a problem hiding this comment.
The undefined happens because there are pinned values that aren't in the result set. A simpler example:
const ordering = ['a', 'b', 'c', 'd']
const values = { a: 5, c: 4, d: 10}
const oldOutput = [{a: 5}, undefined, {c: 4}, {d: 10}]
const newOutput = [{a: 5}, {c: 4}, {d: 10}]The reason empty elements get added is because we don't know what order the items will come in, so you can't just push, you need to set the items in a specific order. Densifying the array at the end seems to me to be the simple solution to avoid empty arrays.
relevant code:
There was a problem hiding this comment.
Ah OK I misunderstood the test because of the missing test name and the PR post.
Haroenv
requested review from
francoischalifour,
a team and
tkrugg
and removed request for
a team
| @@ -779,3 +779,22 @@ describe('hierarchical facet', function() { | |||
| expect(facetValues).toEqual(expected); | |||
| }); | |||
| }); | |||
|
|
|||
| test('customer issue', function() { | |||
| var rawResults = require('./getFacetValues/sparse.json'); | |||
There was a problem hiding this comment.
Ah OK I misunderstood the test because of the missing test name and the PR post.
| @@ -728,6 +728,10 @@ function sortViaFacetOrdering(facetValues, facetOrdering) { | |||
| } | |||
| }); | |||
|
|
|||
| orderedFacets = orderedFacets.filter(function(facet) { | |||
There was a problem hiding this comment.
The data manipulation seems complicated. We can rather not push the facet in the array when we detect that it's undefined? (~line 725)
There was a problem hiding this comment.
the thing is that it's not pushed, but set in a specific position. Imagine if you after release need to set something on position 0 and position 5, you'll get [a, , , , , b]. You can't push, as the order of the items that get inserted is the one of the order, not the order of the facet values itself.
Alternative I can think of is searching the facet value for each value in order, but that's On^m instead of Om*n
There was a problem hiding this comment.
Right, let's keep it that way.
…a/algoliasearch-helper-js#879) * fix(facetOrdering): make sure ordered facets is a dense array Otherwise the "undefined" values (are ordered, but not in the results) take up place and it messes up the limits. * update test name
* fix(facetOrdering): make sure ordered facets is a dense array (algolia/algoliasearch-helper-js#879) algolia/algoliasearch-helper-js@fc1b997
If there are pinned values that don't occur in the facet values, they should not take up space in the result of
getFacetValues. In pseudo code: