Fix #176 - indexing support in collection creation #177
Conversation
| @@ -1941,6 +1941,7 @@ def create_collection( | |||
| options: Optional[Dict[str, Any]] = None, | |||
| dimension: Optional[int] = None, | |||
| metric: Optional[str] = None, | |||
| indexing: Optional[Dict[str, Any]] = None, | |||
There was a problem hiding this comment.
shouldn't be better to expose the deny and allow parameters directly ? like
create_collection("test", allow_indexing=["field1"], deny_indexing=["field2"])
There was a problem hiding this comment.
So i totally agree @nicoloboschi ... it's just that we kind of move away from the API signature. In the past we've typically had syntactic sugar on separate functions that are wrappers, if that makes sense. With that said, if we're kind of slowly working towards just having AstraPy be more pythonic i totally agree and would happily update this. its definitely cleaner
|
Is there a way to add a test ? |
| for k, v in { | ||
| "name": collection_name, | ||
| "options": options, | ||
| "indexing": indexing, |
There was a problem hiding this comment.
Careful. "indexing" is not a new top-level param as it seemed initially.
It was later confirmed that it is inside the "options" dict.
So...
- not urgent to sugar the indexing into a friendlier way since one can take advantage of the feature right away
- in case of separate indexing parameters to the method, similarly to 'dimension' and 'metric' it is necessary to check that it is not passed twice (once within the options and once as separate argument to create_collection)
And for what is worth, if sugar has to be, I would favor two parameters to the effect of allow and deny (and a check that no both are nonempty at the same time)
|
Everyone, do not merge as the mismatch above ("indexing" should be inside options) still stands. Pro: not so urgent as options is supported already everywhere and users can take advantage of the indexing right away |
|
Closing this for now. Will open a new PR for adding syntactic sugar to the indexing (allow, deny) |
Needs a test... wasn't actually sure the best way to go about that especially in lieu of the recent issues with timeouts...