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

Add integration test for doc mapper update #5266

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Conversation

rdettai
Copy link
Contributor

@rdettai rdettai commented Jul 26, 2024

Description

Add some high level validation that searches still work after doc mapper update.

It would be interesting to do the same with aggregations.

Part of #5084

How was this PR tested?

NA (these are tests)

Copy link
Contributor Author

@rdettai rdettai left a comment

Choose a reason for hiding this comment

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

Any juicy scenario suggestion is welcome

@trinity-1686a
Copy link
Contributor

trinity-1686a commented Jul 26, 2024

ideas of test i've run manually with various level of success, which represent some edge case i thought of:

  • u64 -> string, verify the field is still present after update (initially broken, fixed by cast updated fields #5076)
  • with dynamic mode: create index without field1, ingest data with field1, create that field1, ingest some more. Check we can search for both before and after.
  • with strict mode: create index without a field1, ingest docs without field1, create field1, ingest data with this field. Check we don't get error from old splits because of searching on unknown field
  • index string with raw tokenizer no position; ingest document containing - (dash) in value, such as roller-coaster. update tokenizer to default, with positions. ingest more documents. make a phrase query for that doc (this is expected to work)
  • index string with default tokenizer no position; ingest document with - (dash) in value. update to enable positions. ingest more documents. make a phrase query for that doc. this one should work in the future, but is expected to fail currently

@rdettai rdettai force-pushed the doc_mapper_update_tests branch 2 times, most recently from 9d37903 to dee8f36 Compare July 29, 2024 07:57
@rdettai rdettai mentioned this pull request Jul 29, 2024
9 tasks
@rdettai
Copy link
Contributor Author

rdettai commented Jul 29, 2024

@trinity-1686a I added all your test scnarii

Following your suggestion to test "u64 -> string, verify the field is still present after update", I changed the assert from just the number of hits to the whole hit content. I got 2 surprising results:

  • test_update_doc_mapping_json_to_object: I update a json type to an explicit object type, and the fields are dropped in the resulting hit.
  • test_update_doc_mapping_strict_to_dynamic: I switch from an explicit (strict) schema to a dynamic one, and the fields are dropped in the resulting hit.

@trinity-1686a
Copy link
Contributor

trinity-1686a commented Jul 29, 2024

that's interesting, can you add these test as disabled, and i'll try to make them pass

edit: gh wasn't showing me the whole file, the tests are already there. Still you'll need to disable them before we can merge

@rdettai
Copy link
Contributor Author

rdettai commented Jul 29, 2024

done

Copy link

github-actions bot commented Jul 29, 2024

On SSD:

Average search latency is 0.998x that of the reference (lower is better).
Ref run id: 2598, ref commit: 5b5978e
Link

On GCS:

Average search latency is 0.998x that of the reference (lower is better).
Ref run id: 2599, ref commit: 5b5978e
Link

@rdettai rdettai enabled auto-merge (squash) July 30, 2024 08:26
@rdettai rdettai merged commit e862964 into main Jul 30, 2024
5 checks passed
@rdettai rdettai deleted the doc_mapper_update_tests branch July 30, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants