RSDK-9503 accept bson queries in MQL function #803
Conversation
src/viam/app/data_client.py
Outdated
| @@ -269,17 +269,18 @@ async def tabular_data_by_sql(self, organization_id: str, sql_query: str) -> Lis | |||
| response: TabularDataBySQLResponse = await self._data_client.TabularDataBySQL(request, metadata=self._metadata) | |||
| return [bson.decode(bson_bytes) for bson_bytes in response.raw_data] | |||
|
|
|||
| async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes]) -> List[Dict[str, Union[ValueTypes, datetime]]]: | |||
| async def tabular_data_by_mql( | |||
| self, organization_id: str, mql_queries: Union[List[bytes], List[Dict[str, Union[ValueTypes, datetime]]]] | |||
There was a problem hiding this comment.
The parameter name change from mql_binary to mql_query is questionable. Even though we only want to accept queries, we wanted to make a union so that it wasn't a breaking change yet. Changing the name, however, is a breaking change to any user using named parameters, so one could argue we might as well change the input type completely now.
On the other hand, leaving it as mql_binary is now incorrect, and might push users to keep using bytes when we don't want that.
Fine with keeping or changing the parameter name, as long as it's in the direction we want the SDK to go.
There was a problem hiding this comment.
Hmm... I think we definitely want this to not be breaking. This functionality has been around in python long enough - and python is well used enough - that it seems highly likely that we would in fact break someone's workflow.
One option is, we could make a decorator that allows for argument aliasing. Then we can change the name, but set mql_binary as an accepted alias for backwards compatibility. Something like the following (cribbed from https://stackoverflow.com/questions/41784308/keyword-arguments-aliases-in-python)
from typing import Callable
import functools
...
def _alias_param(param_name: str, param_alias: str) -> Callable:
"""
Decorator for aliasing a param in a function. Intended for providing backwards compatibility on params with name changes.
Args:
param_name: name of param in function to alias
param_alias: alias that can be used for this param
Returns:
The input function, plus param alias.
"""
def decorator(func: Callable):
@functools.wraps(func)
def wrapper(*args, **kwargs):
alias_param_value = kwargs.get(param_alias)
if alias_param_value:
kwargs[param_name] = alias_param_value
del kwargs[param_alias]
result = func(*args, **kwargs)
return result
return wrapper
return decoratorThen, we'd define the function like so:
@_alias_param("mql_queries", param_alias="mql_binary")
async def tabular_data_by_mql(
self, organization_id: str, mql_queries: Union[List[bytes], List[Dict[str, Union[ValueTypes, datetime]]]]
) -> List[Dict[str, Union[ValueTypes, datetime]]]:
...After that, we should be providing support for the query as an unnamed param, a mql_queries named param, and a mql_binary named param.
This has a nice property of being applicable in the future if we need to change param names. I do think we should underscore-prefix the decorator name to try and keep it private and write some documentation that provides clarification to future developers about the intended use case.
There was a problem hiding this comment.
One thing to note: the above isn't a perfect solution. For one, the code block I provided doesn't prevent a user from specifying both argument names (i.e., one could write tabular_data_by_mql(mql_queries=<queries>, mql_binary=<binary>) and this would be allowed). And, it's technically preferencing the alias, so if I did provide both args it would choose the binary, not the query. This isn't great because we want the official name to be query.
I think these are definitely both fixable, but we'll want to play around with the decorator code and make sure it behaves properly.
src/viam/app/data_client.py
Outdated
| def _alias_param(param_name: str, param_alias: str) -> Callable: | ||
| """ | ||
| Decorator for aliasing a param in a function. Intended for providing backwards compatibility on params with name changes. | ||
|
|
||
| Args: | ||
| param_name: name of param in function to alias | ||
| param_alias: alias that can be used for this param | ||
| Returns: | ||
| The input function, plus param alias. | ||
| """ | ||
| def decorator(func: Callable): | ||
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| alias_param_value = kwargs.get(param_alias) | ||
| if alias_param_value: | ||
| # Only use alias value if param is not given. | ||
| if not kwargs.get(param_name): | ||
| kwargs[param_name] = alias_param_value | ||
| del kwargs[param_alias] | ||
| result = func(*args, **kwargs) | ||
| return result | ||
| return wrapper | ||
| return decorator |
There was a problem hiding this comment.
I suspect we want this to live in a more generic location, as this could easily end up having applications outside of a data client. Probably utils.py.
No description provided.