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

fix(facetOrdering): make sure ordered facets is a dense array #879

Merged
merged 2 commits into from Oct 15, 2021

Conversation

@Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Oct 12, 2021

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:

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}]
Otherwise the "undefined" values (are ordered, but not in the results) take up place and it messes up the limits.
Haroenv added a commit to algolia/instantsearch.js that referenced this issue Oct 12, 2021
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');
Copy link
Member

@francoischalifour francoischalifour Oct 13, 2021

Choose a reason for hiding this comment

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

Where are the undefined values in the dataset?

Copy link
Contributor Author

@Haroenv Haroenv Oct 13, 2021

Choose a reason for hiding this comment

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

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:

orderedFacets[reverseOrder[name]] = item;

Copy link
Member

@francoischalifour francoischalifour Oct 13, 2021

Choose a reason for hiding this comment

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

Ah OK I misunderstood the test because of the missing test name and the PR post.

test/spec/SearchResults/getFacetValues-facetOrdering.js Outdated Show resolved Hide resolved
@Haroenv Haroenv requested review from francoischalifour, and tkrugg (assigned from algolia/frontend-experiences-web) and removed request for Oct 13, 2021
@@ -779,3 +779,22 @@ describe('hierarchical facet', function() {
expect(facetValues).toEqual(expected);
});
});

test('customer issue', function() {
var rawResults = require('./getFacetValues/sparse.json');
Copy link
Member

@francoischalifour francoischalifour Oct 13, 2021

Choose a reason for hiding this comment

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

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) {
Copy link
Member

@francoischalifour francoischalifour Oct 13, 2021

Choose a reason for hiding this comment

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

The data manipulation seems complicated. We can rather not push the facet in the array when we detect that it's undefined? (~line 725)

Copy link
Contributor Author

@Haroenv Haroenv Oct 13, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@francoischalifour francoischalifour Oct 15, 2021

Choose a reason for hiding this comment

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

Right, let's keep it that way.

@Haroenv Haroenv merged commit 990f8bc into develop Oct 15, 2021
5 checks passed
@Haroenv Haroenv deleted the fix/dynamic-sparse branch Oct 15, 2021
Haroenv added a commit that referenced this issue Oct 15, 2021
 * fix(facetOrdering): make sure ordered facets is a dense array (#879) 990f8bc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants