Implement describe_index_stats #28
Conversation
| Value { | ||
| kind: Some(Kind::BoolValue(false)), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Again, this is how the input is defined for tonic, but not sure if this is the most user friendly definition.
| DescribeIndexStatsError { | ||
| /// Source error | ||
| source: tonic::Status, | ||
| }, |
There was a problem hiding this comment.
Emily and I are likely going to merge these errors into a more general error type in the future, instead of having one for each operation
There was a problem hiding this comment.
I used DataPlaneError in #27 instead of defining ListVectorsError
pinecone_sdk/src/pinecone/data.rs Outdated
| /// ``` | ||
| pub async fn describe_index_stats( | ||
| &mut self, | ||
| filter: Option<BTreeMap<String, Value>>, |
There was a problem hiding this comment.
I'm not sure how much I like this type definition, but it matches how it is defined in the generated code. Thoughts?
There was a problem hiding this comment.
there are optional metadata filters in a few places. might want to define a type and encapsulate the BTreeMap<String, Value>. You could also define From/Into on that type to translate to/from the codegen definition.
There was a problem hiding this comment.
So something like this?
struct MetadataFilter { filter: BTreeMap<String, Value> } impl From<MetadataFilter> for Struct { // impl here } In this case, would it just be better to use the Struct that is defined by tonic and encapsulate filter inside, like how tonic's generated function has it?
There was a problem hiding this comment.
Up to you. It might make sense to define MetadataFilter. The data shape is more strict than tonic's Struct. You don't have to define that strictness here but having your own type might make the SDK surface area to the user better.
There was a problem hiding this comment.
For reference see https://docs.pinecone.io/guides/data/filter-with-metadata with particular attention to "Supported metadata types"
ericapywang requested review from austin-denoble, emily-emily, haruska and ssmith-pc 
pinecone_sdk/src/pinecone/data.rs Outdated
| /// ``` | ||
| pub async fn describe_index_stats( | ||
| &mut self, | ||
| filter: Option<BTreeMap<String, Value>>, |
There was a problem hiding this comment.
there are optional metadata filters in a few places. might want to define a type and encapsulate the BTreeMap<String, Value>. You could also define From/Into on that type to translate to/from the codegen definition.
pinecone_sdk/src/pinecone/data.rs Outdated
| let filter = match filter { | ||
| Some(filter) => Some(Struct { fields: filter }), | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
I find Option#map to be more elegant for these transformations.
| let mut index = pinecone | ||
| .index(host.as_str()) | ||
| .await | ||
| .expect("Failed to target index"); |
There was a problem hiding this comment.
We could look into adding a test fixture for these things since we'll need it often for data plane integration tests - for another PR
Problem
describe_index_statswas not implemented.Solution
I implemented a new function
describe_index_statsforIndex, which optionally takes in aMetadataFilterstruct. It calls tonic's generateddescribe_index_statsfunction.Type of Change
Test Plan
New and existing test cases should pass.