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

Simplify primary key inference #752

Merged
merged 5 commits into from
Jan 2, 2023
Merged

Simplify primary key inference #752

merged 5 commits into from
Jan 2, 2023

Conversation

dureuill
Copy link
Contributor

@dureuill dureuill commented Dec 19, 2022

Pull Request

Related issue

Related to meilisearch/meilisearch#3233

What does this PR do?

User PoV

  • Change primary key inference to only consider a value as a candidate when it ends with "id", rather than when it simply contains "id".
  • Change primary key inference to always fail when there are multiple candidates.
  • Replace UserError::MissingPrimaryKey with UserError::NoPrimaryKeyCandidateFound and UserError::MultiplePrimaryKeyCandidatesFound

Implementation-wise

  • Remove uses of UserError::MissingPrimaryKey not pertaining to inference. This introduces a possible panicking path.

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!

@dureuill dureuill added indexing Related to the documents/settings indexing algorithms. DB breaking The related changes break the DB API breaking The related changes break the milli API labels Dec 19, 2022
@dureuill dureuill marked this pull request as draft December 19, 2022 16:54
@dureuill dureuill force-pushed the simpler_primary_key_inference branch from 8a7bf09 to 36fd2de Compare December 20, 2022 10:59
@dureuill dureuill marked this pull request as ready for review December 20, 2022 10:59
irevoire
irevoire previously approved these changes Dec 20, 2022
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Very nice; I left a little comment, but if you think it's ok like that, we can merge 👍

milli/src/update/index_documents/enrich.rs Outdated Show resolved Hide resolved
milli/src/error.rs Outdated Show resolved Hide resolved
@dureuill dureuill force-pushed the simpler_primary_key_inference branch 2 times, most recently from 1cc56fd to bfcb75b Compare December 21, 2022 09:24
@dureuill
Copy link
Contributor Author

I added a log looking like this:

[2022-12-21T09:19:42Z INFO  milli::update::index_documents::enrich] Primary key was not specified in index. Inferred to 'id'

when inference took place

@dureuill dureuill changed the title Simpler primary key inference Simplify primary key inference Dec 21, 2022
Displays log message in the form:
```
[2022-12-21T09:19:42Z INFO  milli::update::index_documents::enrich] Primary key was not specified in index. Inferred to 'id'
```
@dureuill dureuill force-pushed the simpler_primary_key_inference branch from bfcb75b to 4b166be Compare December 21, 2022 14:13
@irevoire irevoire self-requested a review January 2, 2023 10:22
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 2, 2023

Build succeeded!

And happy new year! 🎉

@bors bors bot merged commit 31155dc into main Jan 2, 2023
@bors bors bot deleted the simpler_primary_key_inference branch January 2, 2023 12:12
bors bot added a commit to meilisearch/meilisearch that referenced this pull request Jan 4, 2023
3269: Simplify primary key inference r=dureuill a=dureuill

# Pull Request

## Related issue
Related to #3233

## What does this PR do?
- Integrates meilisearch/milli#752 in meilisearch
- Remove `Serialize` and `Deserialize` from `error::Code` as it is unused.
- No longer filter on `milli` logs when `--log-level` is "info".
  - `milli` only has the newly-added inference log at the `info` level (from greping `info` in the codebase)
  - the default value for `--log-level` is "INFO" and not "info" since `v0.30` so the filter is not active by default.
- updates milli to v0.38.0

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: Louis Dureuil <louis@meilisearch.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API breaking The related changes break the milli API DB breaking The related changes break the DB indexing Related to the documents/settings indexing algorithms.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants