Enhance error messages for filterable attributes and improve error handling

crates/milli/src/error.rs

@@ -2,6 +2,7 @@ use std::collections::BTreeSet;
 use std::convert::Infallible;
 use std::fmt::Write;
 use std::{io, str};
+use std::collections::HashMap;
 
 use bstr::BString;
 use heed::{Error as HeedError, MdbError};
@@ -78,7 +79,7 @@ pub enum InternalError {
     #[error(transparent)]
     ArroyError(#[from] arroy::Error),
     #[error(transparent)]
-    VectorEmbeddingError(#[from] crate::vector::Error),
+    VectorEmbeddingError(#[from] crate::vector::Error)
 }
 
 #[derive(Error, Debug)]
@@ -120,10 +121,34 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco
 and can not be more than 511 bytes.", .document_id.to_string()
     )]
     InvalidDocumentId { document_id: Value },
-    #[error("Invalid facet distribution, {}", format_invalid_filter_distribution(.invalid_facets_name, .valid_patterns))]
+    #[error("Invalid facet distribution: {}",
+        if .invalid_facets_name.len() == 1 {
+            let field = .invalid_facets_name.iter().next().unwrap();
+            match .matching_rule_indices.get(field) {
+                Some(rule_index) => format!("Attribute `{}` matched rule #{} in filterableAttributes, but this rule does not enable filtering.\nHint: enable filtering in rule #{} by modifying the features.filter object\nHint: prepend another rule matching `{}` with appropriate filter features before rule #{}", 
+                    field, rule_index, rule_index, field, rule_index),
+                None => match .valid_patterns.is_empty() {
+                    true => format!("Attribute `{}` is not filterable. This index does not have configured filterable attributes.", field),
+                    false => format!("Attribute `{}` is not filterable. Available filterable attributes patterns are: `{}`.", 
+                        field, 
+                        .valid_patterns.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(", ")),
+                }
+            }
+        } else {
+            format!("Attributes `{}` are not filterable. {}", 
+                .invalid_facets_name.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(", "),
+                match .valid_patterns.is_empty() {
+                    true => "This index does not have configured filterable attributes.".to_string(),
+                    false => format!("Available filterable attributes patterns are: `{}`.", 
+                        .valid_patterns.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(", ")),
+                }
+            )
+        }
+    )]
     InvalidFacetsDistribution {
         invalid_facets_name: BTreeSet<String>,
         valid_patterns: BTreeSet<String>,
+        matching_rule_indices: HashMap<String, usize>,
     },
     #[error(transparent)]
     InvalidGeoField(#[from] GeoError),
@@ -137,7 +162,12 @@ and can not be more than 511 bytes.", .document_id.to_string()
     InvalidFilter(String),
     #[error("Invalid type for filter subexpression: expected: {}, found: {}.", .0.join(", "), .1)]
     InvalidFilterExpression(&'static [&'static str], Value),
-    #[error("Filter operator `{operator}` is not allowed for the attribute `{field}`.\n  - Note: allowed operators: {}.\n  - Note: field `{field}` {} in `filterableAttributes`", allowed_operators.join(", "), format!("matched rule #{rule_index}"))]
+    #[error("Filter operator `{operator}` is not allowed for the attribute `{field}`.\n  - Note: allowed operators: {}.\n  - Note: field `{field}` matched rule #{rule_index} in `filterableAttributes`\n  - Hint: enable {} in rule #{rule_index} by modifying the features.filter object\n  - Hint: prepend another rule matching `{field}` with appropriate filter features before rule #{rule_index}", 
+        allowed_operators.join(", "), 
+        if operator == "=" || operator == "!=" || operator == "IN" {"equality"} 
+        else if operator == "<" || operator == ">" || operator == "<=" || operator == ">=" || operator == "TO" {"comparison"} 
+        else {"the appropriate filter operators"}
+    )]
     FilterOperatorNotAllowed {
         field: String,
         allowed_operators: Vec<String>,
@@ -157,33 +187,51 @@ and can not be more than 511 bytes.", .document_id.to_string()
     InvalidSortableAttribute { field: String, valid_fields: BTreeSet<String>, hidden_fields: bool },
     #[error("Attribute `{}` is not filterable and thus, cannot be used as distinct attribute. {}",
         .field,
-        match .valid_patterns.is_empty() {
+        match (.valid_patterns.is_empty(), .matching_rule_index) {
-            true => "This index does not have configured filterable attributes.".to_string(),
+            // No rules match and no filterable attributes
-            false => format!("Available filterable attributes patterns are: `{}{}`.",
+            (true, None) => "This index does not have configured filterable attributes.".to_string(),
+            
+            // No rules match but there are some filterable attributes
+            (false, None) => format!("Available filterable attributes patterns are: `{}{}`.",
                     valid_patterns.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(", "),
                     .hidden_fields.then_some(", <..hidden-attributes>").unwrap_or(""),
                 ),
+            
+            // A rule matched but filtering isn't enabled
+            (_, Some(rule_index)) => format!("Note: this attribute matches rule #{} in filterableAttributes, but this rule does not enable filtering.\nHint: enable filtering in rule #{} by adding appropriate filter features.\nHint: prepend another rule matching {} with filter features before rule #{}",
+                    rule_index, rule_index, .field, rule_index
+                ),
         }
     )]
     InvalidDistinctAttribute {
         field: String,
         valid_patterns: BTreeSet<String>,
         hidden_fields: bool,
+        matching_rule_index: Option<usize>,
     },
     #[error("Attribute `{}` is not facet-searchable. {}",
         .field,
-        match .valid_patterns.is_empty() {
+        match (.valid_patterns.is_empty(), .matching_rule_index) {
-            true => "This index does not have configured facet-searchable attributes. To make it facet-searchable add it to the `filterableAttributes` index settings.".to_string(),
+            // No rules match and no facet searchable attributes
-            false => format!("Available facet-searchable attributes patterns are: `{}{}`. To make it facet-searchable add it to the `filterableAttributes` index settings.",
+            (true, None) => "This index does not have configured facet-searchable attributes. To make it facet-searchable add it to the `filterableAttributes` index settings.".to_string(),
+            
+            // No rules match but there are some facet searchable attributes
+            (false, None) => format!("Available facet-searchable attributes patterns are: `{}{}`. To make it facet-searchable add it to the `filterableAttributes` index settings.",
                     valid_patterns.iter().map(AsRef::as_ref).collect::<Vec<&str>>().join(", "),
                     .hidden_fields.then_some(", <..hidden-attributes>").unwrap_or(""),
                 ),
+            
+            // A rule matched but facet search isn't enabled
+            (_, Some(rule_index)) => format!("Note: this attribute matches rule #{} in filterableAttributes, but this rule does not enable facetSearch.\nHint: enable facetSearch in rule #{} by adding `\"facetSearch\": true` to the rule.\nHint: prepend another rule matching {} with facetSearch: true before rule #{}",
+                    rule_index, rule_index, .field, rule_index
+                ),
         }
     )]
     InvalidFacetSearchFacetName {
         field: String,
         valid_patterns: BTreeSet<String>,
         hidden_fields: bool,
+        matching_rule_index: Option<usize>,
     },
     #[error("Attribute `{}` is not searchable. Available searchable attributes are: `{}{}`.",
         .field,
@@ -438,7 +486,7 @@ fn format_invalid_filter_distribution(
 /// ```ignore
 /// impl From<FieldIdMapMissingEntry> for Error {
 ///     fn from(error: FieldIdMapMissingEntry) -> Error {
-///         Error::from(InternalError::from(error))
+///         Error::from(<InternalError>::from(error))
 ///     }
 /// }
 /// ```

crates/milli/src/search/facet/facet_distribution.rs

@@ -378,13 +378,21 @@ impl<'a> FacetDistribution<'a> {
         filterable_attributes_rules: &[FilterableAttributesRule],
     ) -> Result<()> {
         let mut invalid_facets = BTreeSet::new();
+        let mut matching_rule_indices = HashMap::new();
+        
         if let Some(facets) = &self.facets {
             for field in facets.keys() {
-                let is_valid_filterable_field =
+                let matched_rule = matching_features(field, filterable_attributes_rules);
-                    matching_features(field, filterable_attributes_rules)
+                let is_filterable = matched_rule.map_or(false, |(_, features)| features.is_filterable());
-                        .map_or(false, |(_, features)| features.is_filterable());
+                
-                if !is_valid_filterable_field {
+                if !is_filterable {
                     invalid_facets.insert(field.to_string());
+                    
+                    // If the field matched a rule but that rule doesn't enable filtering,
+                    // store the rule index for better error messages
+                    if let Some((rule_index, _)) = matched_rule {
+                        matching_rule_indices.insert(field.to_string(), rule_index);
+                    }
                 }
             }
         }
@@ -400,6 +408,7 @@ impl<'a> FacetDistribution<'a> {
             return Err(Error::UserError(UserError::InvalidFacetsDistribution {
                 invalid_facets_name: invalid_facets,
                 valid_patterns,
+                matching_rule_indices,
             }));
         }
 

crates/milli/src/search/facet/search.rs

@@ -75,20 +75,25 @@ impl<'a> SearchForFacetValues<'a> {
         let rtxn = self.search_query.rtxn;
 
         let filterable_attributes_rules = index.filterable_attributes_rules(rtxn)?;
-        if !matching_features(&self.facet, &filterable_attributes_rules)
+        let matched_rule = matching_features(&self.facet, &filterable_attributes_rules);
-            .map_or(false, |(_, features)| features.is_facet_searchable())
+        let is_facet_searchable = matched_rule.map_or(false, |(_, features)| features.is_facet_searchable());
-        {
+        
+        if !is_facet_searchable {
             let matching_field_names =
                 filtered_matching_patterns(&filterable_attributes_rules, &|features| {
                     features.is_facet_searchable()
                 });
             let (valid_patterns, hidden_fields) =
                 index.remove_hidden_fields(rtxn, matching_field_names)?;
+                
+            // Get the matching rule index if any rule matched the attribute
+            let matching_rule_index = matched_rule.map(|(rule_index, _)| rule_index);
 
             return Err(UserError::InvalidFacetSearchFacetName {
                 field: self.facet.clone(),
                 valid_patterns,
                 hidden_fields,
+                matching_rule_index,
             }
             .into());
         };

crates/milli/src/search/mod.rs

@@ -190,9 +190,10 @@ impl<'a> Search<'a> {
         if let Some(distinct) = &self.distinct {
             let filterable_fields = ctx.index.filterable_attributes_rules(ctx.txn)?;
             // check if the distinct field is in the filterable fields
-            if !matching_features(distinct, &filterable_fields)
+            let matched_rule = matching_features(distinct, &filterable_fields);
-                .map_or(false, |(_, features)| features.is_filterable())
+            let is_filterable = matched_rule.map_or(false, |(_, features)| features.is_filterable());
-            {
+            
+            if !is_filterable {
                 // if not, remove the hidden fields from the filterable fields to generate the error message
                 let matching_patterns =
                     filtered_matching_patterns(&filterable_fields, &|features| {
@@ -200,11 +201,16 @@ impl<'a> Search<'a> {
                     });
                 let (valid_patterns, hidden_fields) =
                     ctx.index.remove_hidden_fields(ctx.txn, matching_patterns)?;
+                
+                // Get the matching rule index if any rule matched the attribute
+                let matching_rule_index = matched_rule.map(|(rule_index, _)| rule_index);
+                
                 // and return the error
                 return Err(Error::UserError(UserError::InvalidDistinctAttribute {
                     field: distinct.clone(),
                     valid_patterns,
                     hidden_fields,
+                    matching_rule_index,
                 }));
             }
         }