[ENH]: Limit users from setting index params #5694
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Add Validation to Restrict User-Supplied Vector Index Parameters This PR introduces systematic input validation for user-supplied parameters in vector index configuration structs ( Key Changes• Added Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| fn validate_float_list_value_type(float_list: &FloatListValueType) -> Result<(), String> { | ||
| if let Some(vector_index) = &float_list.vector_index { | ||
| if let Some(hnsw) = &vector_index.config.hnsw { | ||
| hnsw.validate() | ||
| .map_err(|e| format!("Invalid HNSW configuration: {}", e))?; | ||
| } | ||
| if let Some(spann) = &vector_index.config.spann { | ||
| spann | ||
| .validate() | ||
| .map_err(|e| format!("Invalid SPANN configuration: {}", e))?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[BestPractice]
Validating the individual HNSW and SPANN configurations is a great addition. However, it appears a merged configuration could potentially have both hnsw and spann fields set to Some, which represents an invalid state as enforced later during the conversion to InternalCollectionConfiguration. It would be more robust to validate this constraint here to prevent the creation of an invalid schema.
Consider adding a check to ensure that both configurations are not present simultaneously.
Suggested Change
| fn validate_float_list_value_type(float_list: &FloatListValueType) -> Result<(), String> { | |
| if let Some(vector_index) = &float_list.vector_index { | |
| if let Some(hnsw) = &vector_index.config.hnsw { | |
| hnsw.validate() | |
| .map_err(|e| format!("Invalid HNSW configuration: {}", e))?; | |
| } | |
| if let Some(spann) = &vector_index.config.spann { | |
| spann | |
| .validate() | |
| .map_err(|e| format!("Invalid SPANN configuration: {}", e))?; | |
| } | |
| } | |
| Ok(()) | |
| } | |
| fn validate_float_list_value_type(float_list: &FloatListValueType) -> Result<(), String> { | |
| if let Some(vector_index) = &float_list.vector_index { | |
| if vector_index.config.hnsw.is_some() && vector_index.config.spann.is_some() { | |
| return Err( | |
| "A vector index can have HNSW or SPANN configuration, but not both".to_string(), | |
| ); | |
| } | |
| if let Some(hnsw) = &vector_index.config.hnsw { | |
| hnsw.validate() | |
| .map_err(|e| format!("Invalid HNSW configuration: {}", e))?; | |
| } | |
| if let Some(spann) = &vector_index.config.spann { | |
| spann | |
| .validate() | |
| .map_err(|e| format!("Invalid SPANN configuration: {}", e))?; | |
| } | |
| } | |
| Ok(()) | |
| } |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**BestPractice**]
Validating the individual HNSW and SPANN configurations is a great addition. However, it appears a merged configuration could potentially have both `hnsw` and `spann` fields set to `Some`, which represents an invalid state as enforced later during the conversion to `InternalCollectionConfiguration`. It would be more robust to validate this constraint here to prevent the creation of an invalid schema.
Consider adding a check to ensure that both configurations are not present simultaneously.
<details>
<summary>Suggested Change</summary>
```suggestion
fn validate_float_list_value_type(float_list: &FloatListValueType) -> Result<(), String> {
if let Some(vector_index) = &float_list.vector_index {
if vector_index.config.hnsw.is_some() && vector_index.config.spann.is_some() {
return Err(
"A vector index can have HNSW or SPANN configuration, but not both".to_string(),
);
}
if let Some(hnsw) = &vector_index.config.hnsw {
hnsw.validate()
.map_err(|e| format!("Invalid HNSW configuration: {}", e))?;
}
if let Some(spann) = &vector_index.config.spann {
spann
.validate()
.map_err(|e| format!("Invalid SPANN configuration: {}", e))?;
}
}
Ok(())
}
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: rust/types/src/collection_schema.rs
Line: 988
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Added unit tests
Tested on local tilt
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
None
Observability plan
Validate in staging once deployed
Documentation Changes
None