ronantakizawa/codereview-bench huggingface dataset:
Repository: meilisearch/meilisearch
File: crates/meilisearch/tests/documents/get_documents.rs (Rust)

Code before:
use actix_web::http::header::ACCEPT_ENCODING;
use actix_web::test;
use meili_snap::*;
use urlencoding::encode as urlencode;

use crate::common::encoder::Encoder;
use crate::common::{GetAllDocumentsOptions, Server, Value};
use crate::json;

// TODO: partial test since we are testing error, amd error is not yet fully implemented in
// transplant
#[actix_rt::test]
async fn get_unexisting_index_single_document() {
    let server = Server::new_shared();
    let (_response, code) = server.unique_index().get_document(1, None).await;
    assert_eq!(code, 404);
}

#[actix_rt::test]
async fn error_get_unexisting_document() {
    let server = Server::new_shared();
    let index = server.unique_index();
    let (task, _code) = index.create(None).await;
    index.wait_task(task.uid()).await.succeeded();

    let (response, code) = index.get_document(1, None).await;

    let expected_response = json!({
        "message": "Document `1` not found.",
        "code": "document_not_found",
        "type": "invalid_request",
        "link": "https://docs.meilisearch.com/errors#document_not_found"
    });

    assert_eq!(response, expected_response);
    assert_eq!(code, 404);
}

#[actix_rt::test]
async fn get_document() {

Review comment (refactor):
```suggestion
    let (_response, code) = shared_does_not_exists_index().get_document(1, None).await;
```

Hey, that’s awesome and works.
I implemented a little helper to get an index that doesn’t exist with the following method:
https://github.com/meilisearch/meilisearch/blob/28274292d87317e41250c37f5e504bae170768b6/crates/meilisearch/tests/common/mod.rs#L143

Otherwise, it’s perfect 👌 

Code after:
use actix_web::http::header::ACCEPT_ENCODING;
use actix_web::test;
use meili_snap::*;
use urlencoding::encode as urlencode;

use crate::common::encoder::Encoder;
use crate::common::{
    shared_does_not_exists_index, shared_empty_index, shared_index_with_test_set,
    GetAllDocumentsOptions, Server, Value,
};
use crate::json;

// TODO: partial test since we are testing error, amd error is not yet fully implemented in
// transplant
#[actix_rt::test]
async fn get_unexisting_index_single_document() {
    let (_response, code) = shared_does_not_exists_index().await.get_document(1, None).await;
    assert_eq!(code, 404);
}

#[actix_rt::test]
async fn error_get_unexisting_document() {
    let server = Server::new_shared();
    let index = server.unique_index();
    let (task, _code) = index.create(None).await;
    index.wait_task(task.uid()).await.succeeded();

    let (response, code) = index.get_document(1, None).await;

    let expected_response = json!({
        "message": "Document `1` not found.",
        "code": "document_not_found",
        "type": "invalid_request",
        "link": "https://docs.meilisearch.com/errors#document_not_found"
    });

    assert_eq!(response, expected_response);
    assert_eq!(code, 404);
}
Repository: meilisearch/meilisearch
File: crates/meilisearch/tests/search/formatted.rs (Rust)

Code before:
use insta::{allow_duplicates, assert_json_snapshot};

use super::*;
use crate::common::Server;
use crate::json;

#[actix_rt::test]
async fn formatted_contain_wildcard() {
    let server = Server::new().await;
    let index = server.index("test");

    index.update_settings(json!({ "displayedAttributes": ["id", "cattos"] })).await;

    let documents = NESTED_DOCUMENTS.clone();
    index.add_documents(documents, None).await;
    index.wait_task(1).await;

    index.search(json!({ "q": "pésti", "attributesToRetrieve": ["father", "mother"], "attributesToHighlight": ["father", "mother", "*"], "attributesToCrop": ["doggos"], "showMatchesPosition": true }),
        |response, code|
        {
            assert_eq!(code, 200, "{}", response);
            allow_duplicates! {
              assert_json_snapshot!(response["hits"][0],
                    { "._rankingScore" => "[score]" },
                    @r###"
              {
                "_formatted": {
                  "id": "852",
                  "cattos": "<em>pésti</em>"
                },
                "_matchesPosition": {
                  "cattos": [
                    {
                      "start": 0,
                      "length": 5

Review comment (suggestion):
We were really that close to be able to use a [shared nested index](https://github.com/meilisearch/meilisearch/blob/6b67f9fc4c04bc07cd38e0ff67faff96b0078299/crates/meilisearch/tests/common/mod.rs#L302), but we can’t because we’re playing with the displayed attributes here.
So sad

Code after:
use insta::{allow_duplicates, assert_json_snapshot};

use super::*;
use crate::common::Server;
use crate::json;

#[actix_rt::test]
async fn formatted_contain_wildcard() {
    let server = Server::new_shared();
    let index = server.unique_index();

    index.update_settings(json!({ "displayedAttributes": ["id", "cattos"] })).await;

    let documents = NESTED_DOCUMENTS.clone();
    let (response, _) = index.add_documents(documents, None).await;
    index.wait_task(response.uid()).await;

    index.search(json!({ "q": "pésti", "attributesToRetrieve": ["father", "mother"], "attributesToHighlight": ["father", "mother", "*"], "attributesToCrop": ["doggos"], "showMatchesPosition": true }),
        |response, code|
        {
            assert_eq!(code, 200, "{}", response);
            allow_duplicates! {
              assert_json_snapshot!(response["hits"][0],
                    { "._rankingScore" => "[score]" },
                    @r###"
              {
                "_formatted": {
                  "id": "852",
                  "cattos": "<em>pésti</em>"
                },
                "_matchesPosition": {
                  "cattos": [
                    {
                      "start": 0,
                      "length": 5
Repository: meilisearch/meilisearch
File: crates/meilitool/src/upgrade/v1_12.rs (Rust)

Code before:
        })?;
        let (mut cursor, index) = reader.into_cursor_and_fields_index();

        while let Some(document) = cursor.next_document().with_context(|| {
            format!(
                "while reading documents from batch reader for update file {update_file_path:?}"
            )
        })? {
            let mut json_document = IndexMap::new();
            for (fid, value) in document {
                let field_name = index
                    .name(fid)
                    .with_context(|| format!("while getting field name for fid {fid} for update file {update_file_path:?}"))?;
                let value: &RawValue = serde_json::from_slice(value)?;
                json_document.insert(field_name, value);
            }

            serde_json::to_writer(&mut file, &json_document)?;
        }

        let file = file.into_inner().map_err(|e| e.into_error()).context(format!(
            "while flushing update file bufwriter for update file {update_file_path:?}"
        ))?;
        let _ = file
            .persist(&update_file_path)
            .with_context(|| format!("while persisting update file {update_file_path:?}"))?;
    }

    Ok(())
}


Review comment (suggestion):
```suggestion
            // atomically replace the obkv file with the rewritten NDJSON file
            .persist(&update_file_path)
```

Code after:
        })?;
        let (mut cursor, index) = reader.into_cursor_and_fields_index();

        while let Some(document) = cursor.next_document().with_context(|| {
            format!(
                "while reading documents from batch reader for update file {update_file_path:?}"
            )
        })? {
            let mut json_document = IndexMap::new();
            for (fid, value) in document {
                let field_name = index
                    .name(fid)
                    .with_context(|| format!("while getting field name for fid {fid} for update file {update_file_path:?}"))?;
                let value: &RawValue = serde_json::from_slice(value)?;
                json_document.insert(field_name, value);
            }

            serde_json::to_writer(&mut file, &json_document)?;
        }

        let file = file.into_inner().map_err(|e| e.into_error()).context(format!(
            "while flushing update file bufwriter for update file {update_file_path:?}"
        ))?;
        let _ = file
            // atomically replace the obkv file with the rewritten NDJSON file
            .persist(&update_file_path)
            .with_context(|| format!("while persisting update file {update_file_path:?}"))?;
    }

    Ok(())
}
Repository: meilisearch/meilisearch
File: crates/meilitool/src/upgrade/v1_12.rs (Rust)

Code before:
//! The breaking changes that happened between the v1.11 and the v1.12 are:
//! - The new indexer changed the update files format from OBKV to ndjson. https://github.com/meilisearch/meilisearch/pull/4900

use std::borrow::Cow;
use std::io::BufWriter;
use std::path::Path;
use std::sync::atomic::AtomicBool;

use anyhow::Context;
use file_store::FileStore;
use indexmap::IndexMap;
use meilisearch_types::milli::documents::DocumentsBatchReader;
use milli::heed::types::{SerdeJson, Str};
use milli::heed::{Database, EnvOpenOptions, RoTxn, RwTxn};
use milli::progress::Step;
use milli::{FieldDistribution, Index};
use serde::Serialize;
use serde_json::value::RawValue;
use tempfile::NamedTempFile;
use time::OffsetDateTime;
use uuid::Uuid;

use crate::try_opening_database;
use crate::uuid_codec::UuidCodec;

pub fn v1_11_to_v1_12(
    db_path: &Path,
    _origin_major: &str,
    _origin_minor: &str,
    _origin_patch: &str,
) -> anyhow::Result<()> {
    println!("Upgrading from v1.11.0 to v1.12.0");

    convert_update_files(db_path)?;

    Ok(())
}

pub fn v1_12_to_v1_12_3(
    db_path: &Path,
    origin_major: &str,

Review comment (suggestion):
```suggestion
use meilisearch_types::milli::heed::types::{SerdeJson, Str};
use meilisearch_types::milli::heed::{Database, EnvOpenOptions, RoTxn, RwTxn};
use meilisearch_types::milli::progress::Step;
use meilisearch_types::milli::{FieldDistribution, Index};
```

Code after:
//! The breaking changes that happened between the v1.11 and the v1.12 are:
//! - The new indexer changed the update files format from OBKV to ndjson. https://github.com/meilisearch/meilisearch/pull/4900

use std::borrow::Cow;
use std::io::BufWriter;
use std::path::Path;
use std::sync::atomic::AtomicBool;

use anyhow::Context;
use file_store::FileStore;
use indexmap::IndexMap;
use meilisearch_types::milli::documents::DocumentsBatchReader;
use meilisearch_types::milli::heed::types::{SerdeJson, Str};
use meilisearch_types::milli::heed::{Database, EnvOpenOptions, RoTxn, RwTxn};
use meilisearch_types::milli::progress::Step;
use meilisearch_types::milli::{FieldDistribution, Index};
use serde::Serialize;
use serde_json::value::RawValue;
use tempfile::NamedTempFile;
use time::OffsetDateTime;
use uuid::Uuid;

use crate::try_opening_database;
use crate::uuid_codec::UuidCodec;

pub fn v1_11_to_v1_12(
    db_path: &Path,
    _origin_major: &str,
    _origin_minor: &str,
    _origin_patch: &str,
) -> anyhow::Result<()> {
    println!("Upgrading from v1.11.0 to v1.12.0");

    convert_update_files(db_path)?;

    Ok(())
}

pub fn v1_12_to_v1_12_3(
    db_path: &Path,
    origin_major: &str,
Repository: meilisearch/meilisearch
File: crates/meilitool/src/main.rs (Rust)

Code before:
                    non_compacted_index_file_path.display(),
                )
            })?
            .len();

        let reduction = before_size as f64 / after_size as f64;
        println!("Compaction successful. Took around {:.2?}", before_compaction.elapsed());
        eprintln!("The index went from {before_size} bytes to {after_size} bytes ({reduction:.2}x reduction)");

        eprintln!("Replacing the non-compacted index by the compacted one...");
        std::fs::rename(&compacted_index_file_path, &non_compacted_index_file_path).with_context(
            || {
                format!(
                    "While renaming {} into {}",
                    compacted_index_file_path.display(),
                    non_compacted_index_file_path.display(),
                )
            },
        )?;

        drop(new_file);

        println!("Everything's done 🎉");
    }

    Ok(())
}


Review comment (suggestion):
```suggestion
    bail!("Target index {index_name} not found!")
```

Code after:
                    non_compacted_index_file_path.display(),
                )
            })?
            .len();

        let reduction = before_size as f64 / after_size as f64;
        println!("Compaction successful. Took around {:.2?}", before_compaction.elapsed());
        eprintln!("The index went from {before_size} bytes to {after_size} bytes ({reduction:.2}x reduction)");

        eprintln!("Replacing the non-compacted index by the compacted one...");
        std::fs::rename(&compacted_index_file_path, &non_compacted_index_file_path).with_context(
            || {
                format!(
                    "While renaming {} into {}",
                    compacted_index_file_path.display(),
                    non_compacted_index_file_path.display(),
                )
            },
        )?;

        drop(new_file);

        println!("Everything's done 🎉");
        return Ok(());
    }

    bail!("Target index {index_name} not found!")
}
Repository: meilisearch/meilisearch
File: crates/milli/src/update/new/steps.rs (Rust)

Code before:
use std::borrow::Cow;

use enum_iterator::Sequence;

use crate::progress::Step;

#[derive(Debug, Clone, Copy, PartialEq, Eq, Sequence)]
#[repr(u8)]
pub enum IndexingStep {
    PreparingPayloads,
    ExtractingDocuments,
    ExtractingFacets,
    ExtractingWords,
    ExtractingWordProximity,
    ExtractingEmbeddings,
    MergingFacetCaches,
    MergingWordCaches,
    MergingWordProximity,
    WritingGeoPoints,
    WaitingForDatabaseWrites,
    WaitingForExtractors,
    WritingEmbeddingsToDatabase,
    PostProcessingFacets,
    PostProcessingWords,
    Finalizing,
}

impl Step for IndexingStep {
    fn name(&self) -> Cow<'static, str> {
        match self {
            IndexingStep::PreparingPayloads => "preparing update file",
            IndexingStep::ExtractingDocuments => "extracting documents",
            IndexingStep::ExtractingFacets => "extracting facets",
            IndexingStep::ExtractingWords => "extracting words",
            IndexingStep::ExtractingWordProximity => "extracting word proximity",
            IndexingStep::ExtractingEmbeddings => "extracting embeddings",
            IndexingStep::MergingFacetCaches => "merging facet caches",
            IndexingStep::MergingWordCaches => "merging word caches",
            IndexingStep::MergingWordProximity => "merging word proximity",
            IndexingStep::WritingGeoPoints => "writing geo points",
            IndexingStep::WaitingForDatabaseWrites => "waiting for database writes",
            IndexingStep::WaitingForExtractors => "waiting for extractors",
            IndexingStep::WritingEmbeddingsToDatabase => "writing embeddings to database",
            IndexingStep::PostProcessingFacets => "post-processing facets",
            IndexingStep::PostProcessingWords => "post-processing words",
            IndexingStep::Finalizing => "finalizing",

Review comment (suggestion):
Huuum, not really required in this PR but if we were using the `make_enum_progress!` macro it would handle all the derive, repr and implementation of the `IndexingStep` trait

Code after:
use crate::make_enum_progress;

make_enum_progress! {
    pub enum IndexingStep {
        PreparingPayloads,
        ExtractingDocuments,
        ExtractingFacets,
        ExtractingWords,
        ExtractingWordProximity,
        ExtractingEmbeddings,
        MergingFacetCaches,
        MergingWordCaches,
        MergingWordProximity,
        WritingGeoPoints,
        WaitingForDatabaseWrites,
        WaitingForExtractors,
        WritingEmbeddingsToDatabase,
        PostProcessingFacets,
        PostProcessingWords,
        Finalizing,
    }
}
Repository: meilisearch/meilisearch
File: crates/milli/src/update/indexer_config.rs (Rust)

Code before:
use std::sync::RwLock;

use grenad::CompressionType;

use super::GrenadParameters;
use crate::thread_pool_no_abort::ThreadPoolNoAbort;

#[derive(Debug)]
pub struct IndexerConfig {
    pub log_every_n: Option<usize>,
    pub max_nb_chunks: Option<usize>,
    pub documents_chunk_size: Option<usize>,
    pub max_memory: Option<usize>,
    pub chunk_compression_type: CompressionType,
    pub chunk_compression_level: Option<u32>,
    pub thread_pool: RwLock<Option<ThreadPoolNoAbort>>,
    pub max_positions_per_attributes: Option<u32>,
    pub skip_index_budget: bool,
}

impl IndexerConfig {
    pub fn grenad_parameters(&self) -> GrenadParameters {
        GrenadParameters {
            chunk_compression_type: self.chunk_compression_type,
            chunk_compression_level: self.chunk_compression_level,
            max_memory: self.max_memory,
            max_nb_chunks: self.max_nb_chunks,
        }
    }
}

impl Default for IndexerConfig {
    fn default() -> Self {
        Self {
            log_every_n: None,
            max_nb_chunks: None,
            documents_chunk_size: None,
            max_memory: None,
            chunk_compression_type: CompressionType::None,
            chunk_compression_level: None,
            thread_pool: RwLock::new(None),

Review comment (suggestion):
It seems that you complicated the problem a bit. I would prefer that we keep this type unchanged but propose a new method to create an `IndexerConfig::clone_ignore_thread_pool` and then assign the `thread_pool` by hand. This method would clone the `IndexerConfig` but always keep the `thread_pool` at `None`.

Code after:
use grenad::CompressionType;

use super::GrenadParameters;
use crate::{thread_pool_no_abort::ThreadPoolNoAbort, ThreadPoolNoAbortBuilder};

#[derive(Debug)]
pub struct IndexerConfig {
    pub log_every_n: Option<usize>,
    pub max_nb_chunks: Option<usize>,
    pub documents_chunk_size: Option<usize>,
    pub max_memory: Option<usize>,
    pub max_threads: Option<usize>,
    pub chunk_compression_type: CompressionType,
    pub chunk_compression_level: Option<u32>,
    pub thread_pool: ThreadPoolNoAbort,
    pub max_positions_per_attributes: Option<u32>,
    pub skip_index_budget: bool,
}

impl IndexerConfig {
    pub fn grenad_parameters(&self) -> GrenadParameters {
        GrenadParameters {
            chunk_compression_type: self.chunk_compression_type,
            chunk_compression_level: self.chunk_compression_level,
            max_memory: self.max_memory,
            max_nb_chunks: self.max_nb_chunks,
        }
    }
}

/// By default use only 1 thread for indexing in tests
#[cfg(test)]
pub fn default_thread_pool_and_threads() -> (ThreadPoolNoAbort, Option<usize>) {
    let pool = ThreadPoolNoAbortBuilder::new_for_indexing()
        .num_threads(1)
        .build()
        .expect("failed to build default rayon thread pool");

    (pool, Some(1))
}
Repository: meilisearch/meilisearch
File: crates/meilisearch/tests/search/mod.rs (Rust)

Code before:
                {
                    "prout": "truc 123980612321",
                    "machin": "lol 12345645333447879",
                },
            ],
        },
        {
            "id": 3,
            "title": "The third document 12333",
            "nested": "I lied 98878",
        },
    ]);

    // Test prefix search
    test_settings_documents_indexing_swapping_and_search(
        &documents,
        &json!({
            "searchableAttributes": ["title", "nested.object", "nested.machin"],
            "typoTolerance": {
              "enabled": true,
              "disableOnNumbers": true
            }
        }),
        &json!({"q": "12345"}),
        |response, code| {
            assert_eq!(code, 200, "{}", response);
            snapshot!(json_string!(response["hits"]), @r###"
            [
              {
                "id": 2,
                "title": "The second document 3398499",
                "nested": [
                  "array",
                  {
                    "object": "field 23245121,23223"
                  },
                  {
                    "prout": "truc 123980612321",
                    "machin": "lol 12345645333447879"
                  }
                ]
              }
            ]
            "###);
        },
    )
    .await;

    // Test typo search
    test_settings_documents_indexing_swapping_and_search(
        &documents,

Review comment (suggestion):
nit: 
```suggestion
            assert_eq!(code, 200, "{response}");
```

Code after:
        {
            "id": 0,
            "title": "The zeroth document 1298484",
        },
        {
            "id": 1,
            "title": "The first document 234342",
            "nested": {
                "object": "field 22231",
                "machin": "bidule 23443.32111",
            },
        },
        {
            "id": 2,
            "title": "The second document 3398499",
            "nested": [
                "array",
                {
                    "object": "field 23245121,23223",
                },
                {
                    "prout": "truc 123980612321",
                    "machin": "lol 12345645333447879",
                },
            ],
        },
        {
            "id": 3,
            "title": "The third document 12333",
            "nested": "I lied 98878",
        },
    ]);

    // Test prefix search
    test_settings_documents_indexing_swapping_and_search(
        &documents,
        &json!({
            "searchableAttributes": ["title", "nested.object", "nested.machin"],
            "typoTolerance": {
              "enabled": true,
              "disableOnNumbers": true
            }
        }),
        &json!({"q": "12345"}),
        |response, code| {
            assert_eq!(code, 200, "{response}");
            snapshot!(json_string!(response["hits"]), @r###"
            [
              {
                "id": 2,
                "title": "The second document 3398499",