Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add all_obkv_to_json function #707

Merged
merged 2 commits into from
Nov 28, 2022
Merged

Add all_obkv_to_json function #707

merged 2 commits into from
Nov 28, 2022

Conversation

GregoryConrad
Copy link
Contributor

@GregoryConrad GregoryConrad commented Nov 24, 2022

What does this PR do?

When embedding milli in an application (other than Meilisearch), it often makes sense to not use the displayed_attributes functionality and instead just use milli as a full document store. Thus, this PR adds a function, all_obkv_to_json, to supplement the already exposed milli::obkv_to_json so that those embedding milli do not need to deal with displayed_attributes if they don't need to.

This PR also introduces a slight breaking change: obkv_to_json now accepts a reference to obkv::KvReaderU16 instead of taking ownership of it. As far as I can tell, this seems like a change for the better (obkv_to_json only acts upon obkv rather than consuming it), but I can change it back if you so desire. (reverted in 935a724)

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you. This looks good to me. It will also help us debug things :)

milli/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you very much!
bors merge

@Kerollmops Kerollmops added the no breaking The related changes are not breaking (DB nor API) label Nov 28, 2022
@Kerollmops
Copy link
Member

Hey @GregoryConrad, Would it be possible to quickly fix the Clippy CI in a dedicated commit, please?

@bors
Copy link
Contributor

bors bot commented Nov 28, 2022

Build succeeded:

@bors bors bot merged commit f698e6c into meilisearch:main Nov 28, 2022
@GregoryConrad
Copy link
Contributor Author

Hey @GregoryConrad, Would it be possible to quickly fix the Clippy CI in a dedicated commit, please?

Hi @Kerollmops, cargo clippy runs fine on my machine, and it looks ok in CI based on that bors message? Am I overlooking something?

@Kerollmops
Copy link
Member

Hum... strange, it looks like the issue was in the filter-parser crate. Are you sure you use the latest version of Rust? i.e. rustup update.

@GregoryConrad
Copy link
Contributor Author

@Kerollmops I still can't replicate:

Screenshot 2022-11-28 at 11 08 31 AM

Would you mind copy-pasting the clippy output you have from filter-parser so I can fix it? Perhaps it is on an outdated commit (before I reverted the pass-by-reference changes I made)?

@Kerollmops
Copy link
Member

Hey @GregoryConrad, sorry for the late reply. I just tried to trigger the Clippy error again and could not output it again 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants