-
Notifications
You must be signed in to change notification settings - Fork 13
Adds ability to remove annotation keys #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bbengfort
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - my comments were about making the documentation more clear to users. I always forget how this works, so I'd certainly appreciate some guideposts in the documentation!
As we just discussed over my monitor, this change does not affect tags, so we should be much clearer in the documentation that only annotations are affected by upsert. In particular, we could note that tags cannot be deleted, only set to empty string (we should check if this is true) and that if a tag is set that is not allowed a btrdb.exceptions.BTrDBError: [408] (408: tag key "foo" is invalid) is raised.
(If tags can be deleted, though, it might just be easier to make them behave the same way).
btrdb/stream.py
Outdated
| def update(self, tags=None, annotations=None, collection=None, encoder=AnnotationEncoder, replace=False): | ||
| """ | ||
| Updates metadata including tags, annotations, and collection. | ||
| Updates metadata (as an UPSERT operation) including tags, annotations, and collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a bit more documentation might be helpful here, along with an example. I keep getting confused and forgetting what's happening with update and have to try it on husky before I do it in real life. I'm thinking something to the effect of:
Updates metadata including tags, annotations, and collection as an UPSERT operation.
By default, the update will only affect the keys and values in the specified tags and annotations
dictionaries, inserting them if they don't exist, or updating the value for the key if it does exist. If
any of the update arguments are None, they will remain unchanged in the database.
To delete either tags or annotations, you must specify exactly which keys and values you want
set for the field and set `replace=True`. For example:
>>> annotations, _ = stream.anotations()
>>> del annotations["key_to_delete"]
>>> stream.update(annotations=annotations, replace=True)
This ensures that all of the keys and values for the annotations are preserved except for the
key to be deleted.
Parameters
-----------
tags : dict, optional
Specify the tag key/value pairs as a dictionary to upsert or replace. If None, the tags
will remain unchanged in the database.
annotations : dict, optional
Specify the annotations key/value pairs as a dictionary to upsert or replace. If None,
the annotations will remain unchanged in the database.
collection : str, optional
Specify a new collection for the stream. If None, the collection will remain unchanged.
encoder : json.JSONEncoder or None
JSON encoder class to use for annotation serialization. Set to None to prevent JSON
encoding of the annotations.
replace : bool, default: False
Replace all annotations or tags with the specified dictionaries instead of performing
the normal upsert operation. Specifying True is the only way to remove annotation keys.
| By default, annotations are updated as an UPSERT operation. If you would like | ||
| to remove any keys you must use the `replace=True` keyword argument. | ||
| This will ensure that the dictionary you provide completely replaces the existing | ||
| annotations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add the example from above about how to delete annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I can add this after I'm done with current tasking and will then merge this PR
immesys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is for annotations only. Adds the ability to completely replace annotations.
Behind the scenes, this is done by pulling the most recent annotations, comparing keys, and providing a list of keys that should be removed (as this is how the protobuf call is structured).
Tests have been updated as well as the documentation.