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

v0.27 facetsDistribution behavior changed on empty response #2352

Closed
bidoubiwa opened this issue Apr 26, 2022 · 16 comments · Fixed by #2366
Closed

v0.27 facetsDistribution behavior changed on empty response #2352

bidoubiwa opened this issue Apr 26, 2022 · 16 comments · Fixed by #2366
Assignees
Labels
bug Something isn't working as expected milli Related to the milli workspace v0.27.0 PRs/issues solved in v0.27.0
Milestone

Comments

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Apr 26, 2022

Describe the bug
In v0.26 when a search was made with the facetsDistribution set to genres for example, even when the returned hits contained no hits contained genres the facets distribution value was:

    facetsDistribution: { genres: {} },

with the v0.27 it becomes:

    facetsDistribution: {},

Works in:

  • v0.26.1

but not in:

  • V0.27.0rc0
  • V0.27.0rc1
  • V0.27.0rc2

To Reproduce
Steps to reproduce the behavior:

  1. add genres in the filterableAttributes of an empty index.
  2. add one document lets say: { "id": 1, "title": "hello" }
  3. make a search with facetDistribution set to ["genres"]
  4. this is the return facetsDistribution: {}

Expected behavior
Same behavior as before

Meilisearch version: [e.g. v0.20.0]
v0.27.0rc2

@curquiza curquiza added this to the v0.27.0 milestone Apr 26, 2022
@curquiza curquiza added the bug Something isn't working as expected label Apr 26, 2022
@curquiza
Copy link
Member

curquiza commented Apr 26, 2022

@gmourier is this behavior in the specs? A thing like this "all the required attributes in facetsDistribution (request body) should be put in the search response body in facetDistribution"
This behavior is important for front-end developers if I'm correct, can you confirm @bidoubiwa?

Also can we add tests about this behavior? 🔥

@bidoubiwa
Copy link
Contributor Author

I can confirm! It is used natively by instantsearch to generate the facet UI. Without it, the component does not render

@irevoire
Copy link
Member

irevoire commented Apr 26, 2022

{
  "id": "the best doggo",
  "doggo": {
    "name": "bob",
    "race": "bernese mountain",
    "picture": "https://thewoofclub.ch/wp-content/uploads/2020/09/bernese-puppy.png"
  }
}

If I ask the field distribution for doggo. What should happen?

{
  "doggo": {},
  "doggo.name": { "bob": 1 },
  "doggo.race": { "bernese mountain": 1 },
  "doggo.picture": { "https://thewoofclub.ch/wp-content/uploads/2020/09/bernese-puppy.png": 1 },
}

Or

{
  "doggo.name": { "bob": 1 },
  "doggo.race": { "bernese mountain": 1 },
  "doggo.picture": { "https://thewoofclub.ch/wp-content/uploads/2020/09/bernese-puppy.png": 1 },
}

Or

{
  "doggo": {},
}

Please not this solution 🙏

?

@gmourier @bidoubiwa

EDIT: MY EXAMPLES WERE BROKEN

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

For me, the example 1 is the best thing to do in this case because you could have a non-nested doggo field in another document. This is consistent with today's behavior and does not block the use of a user who has not specifically specified a nested field.

This will happen in case a user has set doggo as a filterable and not just a specific field like doggo.name.

It is typically for this type of behavior that .* would have been useful because the user would be specifying that they want everything back inside doggo to be distributed as facets and the response could eliminate the lonely doggo key distribution

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 27, 2022

I m not sure I understand the context. Was doggo added as a filterableAttribute? How does that work if I want to filter on it?
filter = Doggo = "foe" what happens in this case ?. If I want to have the facets distribution of doggo.name do I have to add it to facetsDistribution: ["doggo.name"] or does facetsDistribution: ["doggo"] makes all child filterable? If yes to which extend ? The nested of the nested of the nested if filterable?

I'm a bit lost.

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

Was doggo added as a filterableAttribute?

Yep otherwise it would have thrown an error at search.

filter = Doggo = "foe" what happens in this case ?

In this case, it will return documents having a simple field doggo (that contains no nested) having the value foe.

If I want to have the facets distribution of doggo.name do I have to add it to facetsDistribution: ["doggo.name"] or does facetsDistribution: ["doggo"] makes all child filterable?

This is the last one, "filterableAttributes": ["doggo"] permits to ask for a distribution on doggo.name if the field exists in doggo as well than asking for a distribution for all nested fields by using "facetsDistribution": ["doggo"].

In any case, the engine just tries to adapt as best as possible to the fact that a schema does not exist. If you specify doggo.name instead of doggo as a filterableAttributes, the engine will throw an error if filter and facetsDistribution contains doggo instead of doggo.name. This is the current behavior for a field that has not been set as a filterableAttributes.

We can expect that a majority of users will not mix documents having different representations for a field.

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

Testing with v0.26.1 I have the current v0.27RC behavior with postman.

Capture d’écran 2022-04-27 à 16 37 13

Documents do not contain a genres field thus the requested facet is not displayed in the response. Am I being crazy right now? Did you tested it doing a pure API call?

@bidoubiwa
Copy link
Contributor Author

This is problematic as instant-search uses facetsDistribution to create components and excepts at all time a key: { ... }

Another inconsistency is that the following implies that name is a facet value. But a key is not a facet value.

{
  "doggo": {},
  "doggo.name": 1,
  "doggo.race": 1,
  "doggo.picture": 1,
}

that output makes the most sense for me:

doggo: {}
doggo.name: {},
doggo.race: {},
doggo.picture: {}

and if you have results:

doggo: {}
doggo.name: { "medor" : 1 },
doggo.race: {},
doggo.picture: {}

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 27, 2022

You can reproduce it the following way:

  • create a repo
  • npm init -y
  • npm install meilisearch
  • touch index.js
  • open index.js copy the following:
const { MeiliSearch } = require('meilisearch')

;(async () => {
  const client = new MeiliSearch({
    host: 'http://127.0.0.1:7700',
    apiKey: 'masterKey',
  })

  const index = client.index('movies')

  const dataset = [
      { id: 1, title: 'Carol' },
      { id: 2, title: 'Wonder Woman', genres: ['Action', 'Adventure'] },
      { id: 3, title: 'Life of Pi', genres: ['Adventure', 'Drama'] },
      { id: 4, title: 'Mad Max: Fury Road', genres: ['Adventure', 'Science Fiction'] },
      { id: 5, title: 'Moana', genres: ['Fantasy', 'Action']},
      { id: 6, title: 'Philadelphia', genres: ['Drama'] },
  ]

  await index.updateFilterableAttributes([
    'genres'
  ])

  let { uid } = await index.addDocuments(dataset)

  await client.waitForTask(uid)

  const filteredSearch = await index.search('Carol', {
    facetsDistribution: ["genres"]
  })
  console.log({ filteredSearch, hit: filteredSearch.hits[0] })

})()
  • run node index.js

Try with v0.26 and v0.27

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

In this example, your documents contains a genres attribute. Okay, I will investigate! I tested it without having a document containing genres.

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 27, 2022

I made a search on Carol on which there is no genres.

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

I managed to reproduce it @irevoire.

Here is the behavior of v0.26.0:

  • If no documents in the db have a genres field, asking for genres as a facet won't add it in the response. (This behavior is still here in v0.27)
  • If some documents in the db have a genres field but the returned search results won't have a genres field, genres is returned as an empty object in facetsDistribution. (This statement is falsy in v0.27)

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

If we have to talk about the behavior of nested fields with facetsDistribution, please make another issue if the current behavior is not good after the bug has been fixed for the sake of clarity. We wasted a lot of time responding and trying to figure out the bug which was not related to the nested field support feature initially. It seems that we introduced a design question in the bug ticket which was already miss understood! Thanks 😇

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Apr 27, 2022

I'm not sure it is wasted as it raised an important other issue! But I will create an a part issue for this :)

EDIT: I did not see tamo's edit. Indeed the behavior is the one expected! Good for me

@gmourier
Copy link
Member

gmourier commented Apr 27, 2022

I apologize, my previous message can sound a bit harsh. The word "wasted" sounds bigger than what I wanted to express. We finally managed to understand each other, I just understood the fact that the format of the nested example was wrong. Thank you for reporting this bug and giving instructions so we can reproduce it @bidoubiwa! 🙌

bors bot added a commit to meilisearch/milli that referenced this issue Apr 28, 2022
518: Return facets even when there is no value associated to it r=Kerollmops a=Kerollmops

This PR is related to meilisearch/meilisearch#2352 and should fix the issue when Meilisearch is up-to-date with this PR.

Co-authored-by: Kerollmops <clement@meilisearch.com>
@curquiza curquiza added the milli Related to the milli workspace label May 4, 2022
@curquiza curquiza linked a pull request May 4, 2022 that will close this issue
bors bot added a commit that referenced this issue May 4, 2022
2366: Bump milli to v0.26.4 r=curquiza a=curquiza

Fixes the milli related issues
- #2352
- #2358
- #2338

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
bors bot added a commit that referenced this issue May 4, 2022
2366: Bump milli to v0.26.4 r=curquiza a=curquiza

Fixes the milli related issues
- #2352
- #2358
- #2338

Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
Co-authored-by: Kerollmops <clement@meilisearch.com>
@curquiza
Copy link
Member

curquiza commented May 4, 2022

Closed by #2366

@curquiza curquiza closed this as completed May 4, 2022
@curquiza curquiza added the v0.27.0 PRs/issues solved in v0.27.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected milli Related to the milli workspace v0.27.0 PRs/issues solved in v0.27.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants