lib/collection/src/problems/unindexed_field.rs (2)

413-427: Fix documentation comment for update_needs_range.

The documentation comment for update_needs_range incorrectly refers to "Match capability" instead of "Range capability".

-/// Capture that an index with Match capability is required.
+/// Capture that an index with Range capability is required.
/// Does not toggle off if has been set once already.
fn update_needs_range(&mut self, is_range: bool) {

---

415-419: Consider optimizing the conditional logic in update methods.

Both helper methods have a conditional that checks if the flag is false before setting it based on the input. Since these methods are only called to potentially set the flag to true (never to set it back to false), you could simplify them:

fn update_needs_match(&mut self, is_match: bool) {
-    if !self.needs_match_index {
-        self.needs_match_index = is_match
-    }
+    self.needs_match_index |= is_match;
}

fn update_needs_range(&mut self, is_range: bool) {
-    if !self.needs_range_index {
-        self.needs_range_index = is_range
-    }
+    self.needs_range_index |= is_range;
}

This uses the bitwise OR assignment operator to set the flag to true if either the current value is true or is_match/is_range is true, which achieves the same result more concisely.

Also applies to: 424-427

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (1)

578-605: Consider a more generic approach in a future refactoring.

While this implementation addresses the immediate needs, consider exploring the abstraction mentioned in the PR comments for a future refactoring. A proper abstraction could make the code more maintainable and reduce the need for explicit conversions between types.

A future improvement could define a trait that represents index functionality and implements it for both PayloadFieldSchema and FieldIndexType, allowing for more flexible comparisons without explicit conversions. This would align with the suggestion in the PR comments about introducing a new abstraction focused solely on capturing the functionality of the index.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (1)

608-623: Extend unit test coverage for integer index parameters

The current unit test only covers one case: when both lookup and range are true. Based on the past review comments, there are additional cases that should be tested:

1. lookup = true, range = false => only IntMatch is inserted
2. lookup = false, range = true => only IntRange is inserted
3. lookup = false, range = false => triggers the debug assertion

#[cfg(test)]
mod tests {
    use super::*;
    #[test]
    fn integer_index_capacities() {
        let params = PayloadSchemaParams::Integer(IntegerIndexParams {
            lookup: Some(true),
            range: Some(true),
            ..Default::default()
        });
        let schema = PayloadFieldSchema::FieldParams(params);
        let index_types = schema_capacities(&schema);
        assert!(index_types.contains(&FieldIndexType::IntMatch));
        assert!(index_types.contains(&FieldIndexType::IntRange));
    }
+    
+    #[test]
+    fn integer_index_lookup_only() {
+        let params = PayloadSchemaParams::Integer(IntegerIndexParams {
+            lookup: Some(true),
+            range: Some(false),
+            ..Default::default()
+        });
+        let schema = PayloadFieldSchema::FieldParams(params);
+        let index_types = schema_capacities(&schema);
+        assert!(index_types.contains(&FieldIndexType::IntMatch));
+        assert!(!index_types.contains(&FieldIndexType::IntRange));
+    }
+    
+    #[test]
+    fn integer_index_range_only() {
+        let params = PayloadSchemaParams::Integer(IntegerIndexParams {
+            lookup: Some(false),
+            range: Some(true),
+            ..Default::default()
+        });
+        let schema = PayloadFieldSchema::FieldParams(params);
+        let index_types = schema_capacities(&schema);
+        assert!(!index_types.contains(&FieldIndexType::IntMatch));
+        assert!(index_types.contains(&FieldIndexType::IntRange));
+    }
+    
+    #[test]
+    #[should_panic(expected = "lookup or range must be true")]
+    fn integer_index_no_capabilities() {
+        let params = PayloadSchemaParams::Integer(IntegerIndexParams {
+            lookup: Some(false),
+            range: Some(false),
+            ..Default::default()
+        });
+        let schema = PayloadFieldSchema::FieldParams(params);
+        let _index_types = schema_capacities(&schema);
+    }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (1)

591-604: Consider handling more parameter combinations

Your implementation only provides conversions for single-capability integer indexes (either lookup or range). Consider adding a combined option that sets both capabilities when needed.

  match val {
      FieldIndexType::IntMatch => {
          let params = IntegerIndexParams {
              lookup: Some(true),
              ..Default::default()
          };
          PayloadFieldSchema::FieldParams(PayloadSchemaParams::Integer(params))
      }
      FieldIndexType::IntRange => {
          let params = IntegerIndexParams {
              range: Some(true),
              ..Default::default()
          };
          PayloadFieldSchema::FieldParams(PayloadSchemaParams::Integer(params))
      }
+     // Consider adding this combined option if needed in the future
+     // FieldIndexType::IntBoth => {
+     //     let params = IntegerIndexParams {
+     //         lookup: Some(true),
+     //         range: Some(true),
+     //         ..Default::default()
+     //     };
+     //     PayloadFieldSchema::FieldParams(PayloadSchemaParams::Integer(params))
+     // }
      FieldIndexType::KeywordMatch => {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (1)

539-586: ⚠️ Potential issue

Replace debug_assert with explicit error handling

The debug_assert on lines 567-570 could be problematic in release builds, as assertions are stripped out during release compilation. For integer indexes, the case where neither lookup nor range is true should be handled explicitly.

 if integer_index_params.range == Some(true) {
     index_types.insert(FieldIndexType::IntRange);
 }
-debug_assert!(
-    !index_types.is_empty(),
-    "lookup or range must be true for Integer payload index"
-);
+// Handle the case where neither lookup nor range is enabled
+if index_types.is_empty() {
+    // Either return false or add a sensible default
+    log::warn!("Integer index with neither lookup nor range capability detected");
+    // Default to providing at least one capability to avoid silent failure
+    index_types.insert(FieldIndexType::IntMatch);
+}
 // unifying match arm types
 true

lib/collection/src/problems/unindexed_field.rs (1)

588-619: Consider simplifying From<FieldIndexType> to use unparametrized types

As mentioned in a previous comment by coszio, it might be better to convert to unparametrized types for simplicity, especially since we don't make recommendations for other parameters like on_disk or is_tenant.

For integer indexes, consider using PayloadFieldSchema::FieldType(PayloadSchemaType::Integer) instead of creating parametrized versions, or provide a clearer explanation in a comment about why parametrized versions are needed here.

 impl From<FieldIndexType> for PayloadFieldSchema {
     fn from(val: FieldIndexType) -> Self {
         match val {
-            FieldIndexType::IntMatch => {
-                let params = IntegerIndexParams {
-                    lookup: Some(true),
-                    ..Default::default()
-                };
-                PayloadFieldSchema::FieldParams(PayloadSchemaParams::Integer(params))
-            }
-            FieldIndexType::IntRange => {
-                let params = IntegerIndexParams {
-                    range: Some(true),
-                    ..Default::default()
-                };
-                PayloadFieldSchema::FieldParams(PayloadSchemaParams::Integer(params))
-            }
+            FieldIndexType::IntMatch | FieldIndexType::IntRange => {
+                // Using unparametrized type as we're just recommending the basic index type
+                PayloadFieldSchema::FieldType(PayloadSchemaType::Integer)
+            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (3)

621-636: 🛠️ Refactor suggestion

Expand test coverage for integer index parameters

The test for integer index capabilities is a good start, but it only covers the case where both lookup and range are true. As mentioned in previous review comments, you should add tests for other combinations: lookup-only, range-only, and neither enabled (which should trigger the debug assertion).

#[test]
fn integer_index_lookup_only() {
    let params = PayloadSchemaParams::Integer(IntegerIndexParams {
        lookup: Some(true),
        range: Some(false),
        ..Default::default()
    });
    let schema = PayloadFieldSchema::FieldParams(params);
    let index_types = schema_capabilities(&schema);
    assert!(index_types.contains(&FieldIndexType::IntMatch));
    assert!(!index_types.contains(&FieldIndexType::IntRange));
}

#[test]
fn integer_index_range_only() {
    let params = PayloadSchemaParams::Integer(IntegerIndexParams {
        lookup: Some(false),
        range: Some(true),
        ..Default::default()
    });
    let schema = PayloadFieldSchema::FieldParams(params);
    let index_types = schema_capabilities(&schema);
    assert!(!index_types.contains(&FieldIndexType::IntMatch));
    assert!(index_types.contains(&FieldIndexType::IntRange));
}

#[test]
#[should_panic(expected = "lookup or range must be true for Integer payload index")]
fn integer_index_none_enabled() {
    let params = PayloadSchemaParams::Integer(IntegerIndexParams {
        lookup: Some(false),
        range: Some(false),
        ..Default::default()
    });
    let schema = PayloadFieldSchema::FieldParams(params);
    let _ = schema_capabilities(&schema);
}

---

560-573: ⚠️ Potential issue

Handle case when both lookup and range are false

The current implementation uses a debug assertion that is only enforced in debug builds. In release builds, this assertion is ignored, potentially allowing an invalid state where both lookup and range are false.

As noted in the previous review comment, you should replace this debug assertion with explicit error handling to ensure the code behaves correctly in all build configurations.

if integer_index_params.lookup == Some(true) {
    index_types.insert(FieldIndexType::IntMatch);
}
if integer_index_params.range == Some(true) {
    index_types.insert(FieldIndexType::IntRange);
}
- debug_assert!(
-     !index_types.is_empty(),
-     "lookup or range must be true for Integer payload index"
- );
- // unifying match arm types
- true
+ if index_types.is_empty() {
+     // Both lookup and range are false or None, which is an invalid configuration
+     log::warn!("Invalid integer index configuration: both lookup and range are disabled");
+     false
+ } else {
+     true
+ }

---

539-586: 🛠️ Refactor suggestion

Strong implementation of schema capability mapping

The schema_capabilities function properly maps PayloadFieldSchema to index capabilities, with good handling of parametrized integer indexes that separately track lookup and range capabilities.

However, there's a potential issue with the debug assertion at lines 567-570. In release builds, this assertion would be compiled out, potentially allowing both lookup and range to be false without any error handling.

- debug_assert!(
-     !index_types.is_empty(),
-     "lookup or range must be true for Integer payload index"
- );
- // unifying match arm types
- true
+ if index_types.is_empty() {
+     // Handle the case where both lookup and range are false
+     log::warn!("Integer payload index with neither lookup nor range capability");
+     // Return false to indicate no capabilities, or consider adding a default capability
+     false
+ } else {
+     true
+ }

lib/collection/src/problems/unindexed_field.rs (1)

588-619: Implementation creates appropriate schema recommendations

The From<FieldIndexType> implementation properly creates schema representations for issue recommendations. For parameterizable types like IntMatch and IntRange, it creates field parameters with appropriate options, and for basic types it uses field type schemas.

A minor observation: both UuidMatch and UuidRange convert to the same PayloadSchemaType::Uuid (lines 611-612). This appears to match the system design where UUID fields inherently support both matching and range operations, but it might be worth adding a comment to clarify this intentional behavior.

            FieldIndexType::KeywordMatch => {
                PayloadFieldSchema::FieldType(PayloadSchemaType::Keyword)
            }
            FieldIndexType::FloatRange => PayloadFieldSchema::FieldType(PayloadSchemaType::Float),
            FieldIndexType::Text => PayloadFieldSchema::FieldType(PayloadSchemaType::Text),
            FieldIndexType::BoolMatch => PayloadFieldSchema::FieldType(PayloadSchemaType::Bool),
            FieldIndexType::UuidMatch => PayloadFieldSchema::FieldType(PayloadSchemaType::Uuid),
-           FieldIndexType::UuidRange => PayloadFieldSchema::FieldType(PayloadSchemaType::Uuid),
+           FieldIndexType::UuidRange => {
+               // UUID fields inherently support both match and range operations
+               PayloadFieldSchema::FieldType(PayloadSchemaType::Uuid)
+           },
            FieldIndexType::DatetimeRange => {
                PayloadFieldSchema::FieldType(PayloadSchemaType::Datetime)
            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

lib/collection/src/problems/unindexed_field.rs (3)

610-628: 🛠️ Refactor suggestion

Expand test coverage for integer index parameters

The current test only verifies that integer indexes with both lookup and range set to true map to both IntMatch and IntRange capabilities. Tests for other combinations are missing.

Add tests for the following cases:

• lookup = true, range = false → only FieldIndexType::IntMatch is inserted
• lookup = false, range = true → only FieldIndexType::IntRange is inserted
• lookup = false, range = false → should trigger the assertion or handle the empty case

 #[test]
 fn integer_index_capacities() {
     let params = PayloadSchemaParams::Integer(IntegerIndexParams {
         lookup: Some(true),
         range: Some(true),
         ..Default::default()
     });
     let schema = PayloadFieldSchema::FieldParams(params);
     let index_types = schema_capabilities(&schema);
     assert!(index_types.contains(&FieldIndexType::IntMatch));
     assert!(index_types.contains(&FieldIndexType::IntRange));
 }
+
+#[test]
+fn integer_index_lookup_only() {
+    let params = PayloadSchemaParams::Integer(IntegerIndexParams {
+        lookup: Some(true),
+        range: Some(false),
+        ..Default::default()
+    });
+    let schema = PayloadFieldSchema::FieldParams(params);
+    let index_types = schema_capabilities(&schema);
+    assert!(index_types.contains(&FieldIndexType::IntMatch));
+    assert!(!index_types.contains(&FieldIndexType::IntRange));
+}
+
+#[test]
+fn integer_index_range_only() {
+    let params = PayloadSchemaParams::Integer(IntegerIndexParams {
+        lookup: Some(false),
+        range: Some(true),
+        ..Default::default()
+    });
+    let schema = PayloadFieldSchema::FieldParams(params);
+    let index_types = schema_capabilities(&schema);
+    assert!(!index_types.contains(&FieldIndexType::IntMatch));
+    assert!(index_types.contains(&FieldIndexType::IntRange));
+}

---

564-578: ⚠️ Potential issue

Handle the case when both lookup and range are false in IntegerIndexParams

This is a response to the previous review comment about handling invalid IntegerIndexParams. As suggested, we should explicitly handle the case when both lookup and range are false (or None) rather than relying on the debug assertion.

Replace the debug assertion with explicit handling:

 if integer_index_params.lookup == Some(true) {
     index_types.insert(FieldIndexType::IntMatch);
 }
 if integer_index_params.range == Some(true) {
     index_types.insert(FieldIndexType::IntRange);
 }
-debug_assert!(
-    !index_types.is_empty(),
-    "lookup or range must be true for Integer payload index"
-);
+if index_types.is_empty() {
+    // Either return false to indicate this does not satisfy indexing requirements
+    // or insert a default capability
+    return false;
+}
 // unifying match arm types
 true

---

544-591: 🛠️ Refactor suggestion

Add explicit error handling for invalid integer index parameters

Currently, the debug_assert! on lines 572-575 checks that at least one of lookup or range is true for integer indexes. However, this assertion is skipped in release builds, potentially allowing invalid configurations through.

Consider replacing the debug assertion with explicit error handling:

 if integer_index_params.lookup == Some(true) {
     index_types.insert(FieldIndexType::IntMatch);
 }
 if integer_index_params.range == Some(true) {
     index_types.insert(FieldIndexType::IntRange);
 }
-debug_assert!(
-    !index_types.is_empty(),
-    "lookup or range must be true for Integer payload index"
-);
+if index_types.is_empty() {
+    log::warn!("Neither lookup nor range is enabled for Integer payload index");
+    // Provide a sensible default or return false to indicate invalid configuration
+    index_types.insert(FieldIndexType::IntRange); // Default to range capability
+}
 // unifying match arm types
 true

lib/collection/src/problems/unindexed_field.rs (1)

593-608: Consider preserving index capability information in the schema conversion

The current conversion from FieldIndexType to PayloadSchemaType loses information about whether an integer index needs lookup or range capability. This might lead to generating suboptimal schema recommendations.

You could consider extending the conversion to preserve capability information:

 impl From<FieldIndexType> for PayloadSchemaType {
     fn from(val: FieldIndexType) -> Self {
         match val {
-            FieldIndexType::IntMatch => PayloadSchemaType::Integer,
-            FieldIndexType::IntRange => PayloadSchemaType::Integer,
+            FieldIndexType::IntMatch | FieldIndexType::IntRange => PayloadSchemaType::Integer,
             FieldIndexType::KeywordMatch => PayloadSchemaType::Keyword,
             FieldIndexType::FloatRange => PayloadSchemaType::Float,
             FieldIndexType::Text => PayloadSchemaType::Text,
             FieldIndexType::BoolMatch => PayloadSchemaType::Bool,
-            FieldIndexType::UuidMatch => PayloadSchemaType::Uuid,
-            FieldIndexType::UuidRange => PayloadSchemaType::Uuid,
+            FieldIndexType::UuidMatch | FieldIndexType::UuidRange => PayloadSchemaType::Uuid,
             FieldIndexType::DatetimeRange => PayloadSchemaType::Datetime,
             FieldIndexType::Geo => PayloadSchemaType::Geo,
         }
     }
 }

Then, you could also consider implementing From<FieldIndexType> for PayloadSchemaParams to generate more specific schema recommendations:

impl From<FieldIndexType> for PayloadSchemaParams {
    fn from(val: FieldIndexType) -> Self {
        match val {
            FieldIndexType::IntMatch => PayloadSchemaParams::Integer(IntegerIndexParams {
                lookup: Some(true),
                range: Some(false),
                ..Default::default()
            }),
            FieldIndexType::IntRange => PayloadSchemaParams::Integer(IntegerIndexParams {
                lookup: Some(false),
                range: Some(true),
                ..Default::default()
            }),
            // ... other conversions ...
        }
    }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro