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

.NET SDK CI is failing since v1.7.1 #4525

Closed
curquiza opened this issue Mar 21, 2024 · 8 comments · Fixed by #4539
Closed

.NET SDK CI is failing since v1.7.1 #4525

curquiza opened this issue Mar 21, 2024 · 8 comments · Fixed by #4539
Labels
bug Something isn't working as expected v1.7.4 PRs/issues solved in v1.7.4
Milestone

Comments

@curquiza
Copy link
Member

Since we release v1.7.1, the .NET SDK CI is failing.

https://github.com/meilisearch/meilisearch/actions/runs/8378570359/job/22943427563

But did not fail with v1.7.0

https://github.com/meilisearch/meilisearch/actions/runs/8378435323/job/22942984013

I checked, it's not a random failure.

One of the failing tests is: https://github.com/meilisearch/meilisearch-dotnet/blob/b9b1ecbc62985b2d079bb5c0637fccd5d1c02539/tests/Meilisearch.Tests/SearchTests.cs#L462

@curquiza curquiza added the bug Something isn't working as expected label Mar 21, 2024
@curquiza
Copy link
Member Author

@Kerollmops since you made the change in v1.7.1

@sanders41
Copy link
Contributor

This seems like a case sensitivity issue. All keys in the json file are pascal case, while the test uses camel case.

I tested this theory in Python. Matching the dotnet test returns document 10 first.

import json

from meilisearch_python_sdk import Client


def main() -> int:
    with open("data.json") as f:  # data.json is the json from the dotnet test
        data = json.load(f)

    client = Client("http://127.0.0.1:7700", "masterKey")
    index = client.create_index("movies", primary_key="Id")
    task = index.update_searchable_attributes(["name", "info.comment"])
    client.wait_for_task(task.task_uid)
    task = index.update_sortable_attributes(["info.reviewNb"])
    client.wait_for_task(task.task_uid)
    task = index.add_documents(data)
    client.wait_for_task(task.task_uid)
    movies = index.search("", sort=["info.reviewNb:desc"])
    print(movies)

    return 0


if __name__ == "__main__":
    raise SystemExit(main())

Changing the case to match the json file returns 11 first (the expected value)

import json

from meilisearch_python_sdk import Client


def main() -> int:
    with open("data.json") as f:
        data = json.load(f)

    client = Client("http://127.0.0.1:7700", "masterKey")
    index = client.create_index("movies", primary_key="Id")
    task = index.update_searchable_attributes(["name", "Info.Comment"])
    client.wait_for_task(task.task_uid)
    task = index.update_sortable_attributes(["Info.ReviewNb"])
    client.wait_for_task(task.task_uid)
    task = index.add_documents(data)
    client.wait_for_task(task.task_uid)
    movies = index.search("", sort=["Info.ReviewNb:desc"])
    print(movies)

    return 0


if __name__ == "__main__":
    raise SystemExit(main())

@curquiza
Copy link
Member Author

Thank you @sanders41

I'm not sure I understand WHY this changed between v1.7.0 and v1.7.1. I want us to be sure we are aware of this change on the engine side. .NET SDK did not change anything, only the engine changed. So I want to understand why we now return "Gladiator" instead of "Interstellar" in this test.

Capture d’écran 2024-03-22 à 17 04 59

So I'm not sure I understand why it's related to camelcase issue.

@sanders41
Copy link
Contributor

Yes, I agree. I was just pointing out that the root cause seems to be the values used to not be case sensitive and they are now. So that seems to be the place to start looking for the change.

@sanders41
Copy link
Contributor

Interestingly I checked out the v1.7.0, then all the way back to 4b644f6 and it was always consistent. "Gladiator" was always returned for me first unless I matched the case in the json.

@dureuill
Copy link
Contributor

Hello,

yes, Meilisearch is case-sensitive for fields, so unless the C# code is changing the casing before sending the requests over the wire, the field that is set as sortable does not actually exist in the DB.

We're then sorting by a non-existing field, which is the same as a placeholder search. The order of documents in a placeholder search is not guaranteed to be consistent between Meilisearch versions.

@dureuill
Copy link
Contributor

dureuill commented Mar 26, 2024

I investigated further, and there's definitely a Meilisearch v1.7.1 related bug here.

1. The test is correct considering how the .net SDK works

The .net SDK changes the case of fields in some circumstances, so the lowercase fields in the tests are actually consistent with the uppercase fields from the JSON dataset.

Behavior of the .net SDK
  1. The SDK deserializes the JSON file as a MovieWithInfo class, such that the uppercased field match the corresponding upper case property
  2. The SDK then serializes again the MovieWithInfo class in the call to Index.AddDocumentsAsync
  3. In doing so, it transforms the keys so that their first letter is lowercase. We even have an issue open about this behavior
  4. In the end, the keys stored in the Meilisearch instance are lower-cased

2. Meilisearch v1.7.1 does not trigger a reindexing operation when it should with nested fields

  1. Adding the documents with the upper case: curl http://localhost:7700/indexes/test4/documents -H 'Content-Type: application/json' -d @"/Users/dureuill/dev/meilisearch-dotnet/tests/Meilisearch.Tests/Datasets/movies_with_info.json"
  2. Checking the documents:
❯ curl http://localhost:7700/indexes/test4/documents
{"results":[{"Id":"10","Name":"Gladiator","Info":{"Comment":"a movie about old times","ReviewNb":700}},{"Id":"11","Name":"Interstellar","Info":{"Comment":"the best movie","ReviewNb":1000}},{"Id":"12","Name":"Star Wars","Info":{"Comment":"a lot of wars in the stars","ReviewNb":900}},{"Id":"13","Name":"Harry Potter","Info":{"Comment":"a movie about a wizard boy","ReviewNb":900}},{"Id":"14","Name":"Iron Man","Info":{"Comment":"a movie about a rich man","ReviewNb":800}},{"Id":"15","Name":"Spider-Man","Info":{"Comment":"the spider bit the boy","ReviewNb":900}},{"Id":"16","Name":"Amélie Poulain","Info":{"Comment":"talks about hapiness","ReviewNb":800}}],"offset":0,"limit":20,"total":7}%
  1. Making the documents sortable by the Info.ReviewNb:
❯ echo '{"sortableAttributes": ["Info.ReviewNb"] }' | mieli settings -i test4
  1. Attempting to sort the documents by review nb doesn't work:
❯ curl http://localhost:7700/indexes/test4/search -H 'Content-Type: application/json' --data-binary '{"sort": ["Info.ReviewNb:desc"]}'

{"hits":[{"Id":"10","Name":"Gladiator","Info":{"Comment":"a movie about old times","ReviewNb":700}},{"Id":"11","Name":"Interstellar","Info":{"Comment":"the best movie","ReviewNb":1000}},{"Id":"12","Name":"Star Wars","Info":{"Comment":"a lot of wars in the stars","ReviewNb":900}},{"Id":"13","Name":"Harry Potter","Info":{"Comment":"a movie about a wizard boy","ReviewNb":900}},{"Id":"14","Name":"Iron Man","Info":{"Comment":"a movie about a rich man","ReviewNb":800}},{"Id":"15","Name":"Spider-Man","Info":{"Comment":"the spider bit the boy","ReviewNb":900}},{"Id":"16","Name":"Amélie Poulain","Info":{"Comment":"talks about hapiness","ReviewNb":800}}],"query":"","processingTimeMs":0,"limit":20,"offset":0,"estimatedTotalHits":7}%

@dureuill dureuill added this to the v1.7.4 milestone Mar 27, 2024
meili-bors bot added a commit that referenced this issue Mar 27, 2024
4539: Don't optimize reindexing when fields contain dots r=Kerollmops a=dureuill

# Pull Request

## Related issue
Fixes #4525

## What does this PR do?
- Don't try to optimize the amount of reindexing operation when nested fields are used anywhere in:
    - the field distribution (e.g. a key actually contains a `.`)
    - the old faceted fields
    - the new faceted fields

This is because the facet distribution is not reporting on existing nested fields.



Co-authored-by: Louis Dureuil <louis@meilisearch.com>
@curquiza curquiza linked a pull request Mar 28, 2024 that will close this issue
@curquiza
Copy link
Member Author

Closed by #4539

@meili-bot meili-bot added the v1.7.4 PRs/issues solved in v1.7.4 label Apr 2, 2024
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 v1.7.4 PRs/issues solved in v1.7.4
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants