From 8419fd9b3b8dbadf8edfed87c6dca3bb1d798f07 Mon Sep 17 00:00:00 2001 From: Mubelotix Date: Tue, 1 Jul 2025 13:42:38 +0200 Subject: [PATCH] Ditch usage of check_sort_criteria --- crates/meilisearch-types/src/error.rs | 3 +- crates/milli/src/error.rs | 18 +++- .../milli/src/facet/facet_sort_recursive.rs | 89 +++++++++---------- crates/milli/src/search/new/mod.rs | 4 +- 4 files changed, 63 insertions(+), 51 deletions(-) diff --git a/crates/meilisearch-types/src/error.rs b/crates/meilisearch-types/src/error.rs index 9cf1b93a0..e43b28fc6 100644 --- a/crates/meilisearch-types/src/error.rs +++ b/crates/meilisearch-types/src/error.rs @@ -467,7 +467,8 @@ impl ErrorCode for milli::Error { UserError::InvalidDistinctAttribute { .. } => Code::InvalidSearchDistinct, UserError::SortRankingRuleMissing => Code::InvalidSearchSort, UserError::InvalidFacetsDistribution { .. } => Code::InvalidSearchFacets, - UserError::InvalidSortableAttribute { .. } => Code::InvalidSearchSort, + UserError::InvalidSearchSortableAttribute { .. } => Code::InvalidSearchSort, + UserError::InvalidDocumentSortableAttribute { .. } => Code::InvalidDocumentSort, UserError::InvalidSearchableAttribute { .. } => { Code::InvalidSearchAttributesToSearchOn } diff --git a/crates/milli/src/error.rs b/crates/milli/src/error.rs index 2624a9824..f3b390690 100644 --- a/crates/milli/src/error.rs +++ b/crates/milli/src/error.rs @@ -191,7 +191,21 @@ and can not be more than 511 bytes.", .document_id.to_string() ), } )] - InvalidSortableAttribute { field: String, valid_fields: BTreeSet, hidden_fields: bool }, + InvalidSearchSortableAttribute { + field: String, + valid_fields: BTreeSet, + hidden_fields: bool, + }, + #[error("Attribute `{}` is not sortable. {}", + .field, + match .sortable_fields.is_empty() { + true => "This index does not have configured sortable attributes.".to_string(), + false => format!("Available sortable attributes are: `{}`.", + sortable_fields.iter().map(AsRef::as_ref).collect::>().join(", ") + ), + } + )] + InvalidDocumentSortableAttribute { field: String, sortable_fields: BTreeSet }, #[error("Attribute `{}` is not filterable and thus, cannot be used as distinct attribute. {}", .field, match (.valid_patterns.is_empty(), .matching_rule_index) { @@ -614,7 +628,7 @@ fn conditionally_lookup_for_error_message() { ]; for (list, suffix) in messages { - let err = UserError::InvalidSortableAttribute { + let err = UserError::InvalidSearchSortableAttribute { field: "name".to_string(), valid_fields: list, hidden_fields: false, diff --git a/crates/milli/src/facet/facet_sort_recursive.rs b/crates/milli/src/facet/facet_sort_recursive.rs index 19bf5afb9..ab62ebcfd 100644 --- a/crates/milli/src/facet/facet_sort_recursive.rs +++ b/crates/milli/src/facet/facet_sort_recursive.rs @@ -1,16 +1,15 @@ -use std::collections::VecDeque; +use std::collections::{BTreeSet, VecDeque}; use crate::{ + constants::RESERVED_GEO_FIELD_NAME, documents::{geo_sort::next_bucket, GeoSortParameter}, heed_codec::{ facet::{FacetGroupKeyCodec, FacetGroupValueCodec}, BytesRefCodec, }, - search::{ - facet::{ascending_facet_sort, descending_facet_sort}, - new::check_sort_criteria, - }, - AscDesc, DocumentId, Member, + is_faceted, + search::facet::{ascending_facet_sort, descending_facet_sort}, + AscDesc, DocumentId, Member, UserError, }; use heed::Database; use roaring::RoaringBitmap; @@ -366,49 +365,47 @@ pub fn recursive_facet_sort<'ctx>( sort: Vec, candidates: &'ctx RoaringBitmap, ) -> crate::Result> { - check_sort_criteria(index, rtxn, Some(&sort))?; - - let mut fields = Vec::new(); + let sortable_fields: BTreeSet<_> = index.sortable_fields(rtxn)?.into_iter().collect(); let fields_ids_map = index.fields_ids_map(rtxn)?; + + let mut fields = Vec::new(); let mut need_geo_candidates = false; - for sort in sort { - match sort { - AscDesc::Asc(Member::Field(field)) => { - if let Some(field_id) = fields_ids_map.id(&field) { - fields.push(AscDescId::Facet { field_id, ascending: true }); - } - } - AscDesc::Desc(Member::Field(field)) => { - if let Some(field_id) = fields_ids_map.id(&field) { - fields.push(AscDescId::Facet { field_id, ascending: false }); - } - } - AscDesc::Asc(Member::Geo(target_point)) => { - if let (Some(lat), Some(lng)) = - (fields_ids_map.id("_geo.lat"), fields_ids_map.id("_geo.lng")) - { - need_geo_candidates = true; - fields.push(AscDescId::Geo { - field_ids: [lat, lng], - target_point, - ascending: true, - }); - } - } - AscDesc::Desc(Member::Geo(target_point)) => { - if let (Some(lat), Some(lng)) = - (fields_ids_map.id("_geo.lat"), fields_ids_map.id("_geo.lng")) - { - need_geo_candidates = true; - fields.push(AscDescId::Geo { - field_ids: [lat, lng], - target_point, - ascending: false, - }); - } - } + for asc_desc in sort { + let (field, geofield) = match asc_desc { + AscDesc::Asc(Member::Field(field)) => (Some((field, true)), None), + AscDesc::Desc(Member::Field(field)) => (Some((field, false)), None), + AscDesc::Asc(Member::Geo(target_point)) => (None, Some((target_point, true))), + AscDesc::Desc(Member::Geo(target_point)) => (None, Some((target_point, false))), }; - // FIXME: Should this return an error if the field is not found? + if let Some((field, ascending)) = field { + if is_faceted(&field, &sortable_fields) { + if let Some(field_id) = fields_ids_map.id(&field) { + fields.push(AscDescId::Facet { field_id, ascending }); + continue; + } + } + return Err(UserError::InvalidDocumentSortableAttribute { + field: field.to_string(), + sortable_fields: sortable_fields.clone(), + } + .into()); + } + if let Some((target_point, ascending)) = geofield { + if sortable_fields.contains(RESERVED_GEO_FIELD_NAME) { + if let (Some(lat), Some(lng)) = + (fields_ids_map.id("_geo.lat"), fields_ids_map.id("_geo.lng")) + { + need_geo_candidates = true; + fields.push(AscDescId::Geo { field_ids: [lat, lng], target_point, ascending }); + continue; + } + } + return Err(UserError::InvalidDocumentSortableAttribute { + field: RESERVED_GEO_FIELD_NAME.to_string(), + sortable_fields: sortable_fields.clone(), + } + .into()); + } } let geo_candidates = if need_geo_candidates { diff --git a/crates/milli/src/search/new/mod.rs b/crates/milli/src/search/new/mod.rs index b5258413e..3983aa07a 100644 --- a/crates/milli/src/search/new/mod.rs +++ b/crates/milli/src/search/new/mod.rs @@ -903,7 +903,7 @@ pub(crate) fn check_sort_criteria( let (valid_fields, hidden_fields) = index.remove_hidden_fields(rtxn, sortable_fields)?; - return Err(UserError::InvalidSortableAttribute { + return Err(UserError::InvalidSearchSortableAttribute { field: field.to_string(), valid_fields, hidden_fields, @@ -914,7 +914,7 @@ pub(crate) fn check_sort_criteria( let (valid_fields, hidden_fields) = index.remove_hidden_fields(rtxn, sortable_fields)?; - return Err(UserError::InvalidSortableAttribute { + return Err(UserError::InvalidSearchSortableAttribute { field: RESERVED_GEO_FIELD_NAME.to_string(), valid_fields, hidden_fields,