Skip to content
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.

Sign up for GitHub

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

RSDK-9503 accept bson queries in MQL function #803

Merged
merged 18 commits into from
Dec 20, 2024
Merged

RSDK-9503 accept bson queries in MQL function #803

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
44b6880
accept dictionary in mql function
purplenicole730 Dec 17, 2024
279049c
change comment
purplenicole730 Dec 17, 2024
59b6495
make lint
purplenicole730 Dec 17, 2024
fdc3de0
make queries a list of dictionaries
purplenicole730 Dec 17, 2024
e3367a3
explicitly type check
purplenicole730 Dec 17, 2024
c661368
explicitly state that mqlbinary is a list of bytes
purplenicole730 Dec 17, 2024
d0ea253
test with list
purplenicole730 Dec 17, 2024
c4014d9
more type checking for lint
purplenicole730 Dec 17, 2024
b5fb8f2
more excessive checking for the type checker
purplenicole730 Dec 17, 2024
a493936
remove unnccessary annotation
purplenicole730 Dec 17, 2024
f74d801
ignore lint
purplenicole730 Dec 18, 2024
7fa1b4e
ignore type
purplenicole730 Dec 18, 2024
f7d466b
add alias param
purplenicole730 Dec 19, 2024
d869456
move decorator outside of class
purplenicole730 Dec 19, 2024
2b67023
use query if both query and binary are given
purplenicole730 Dec 19, 2024
7fdb051
broaden comment
purplenicole730 Dec 19, 2024
fd316a1
change param type to use any
purplenicole730 Dec 19, 2024
c615944
move general command to utils
purplenicole730 Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions src/viam/app/data_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]]]]
Copy link
Member Author

@purplenicole730 purplenicole730 Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stuqdog stuqdog Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 decorator

Then, 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.

Copy link
Member

@stuqdog stuqdog Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) -> List[Dict[str, Union[ValueTypes, datetime]]]:
"""Obtain unified tabular data and metadata, queried with MQL.

::

import bson

# using pymongo package (pip install pymongo)
tabular_data = await data_client.tabular_data_by_mql(organization_id="<YOUR-ORG-ID>", mql_binary=[
bson.encode({ '$match': { 'location_id': '<YOUR-LOCATION-ID>' } }),
bson.encode({ "$limit": 5 })
tabular_data = await data_client.tabular_data_by_mql(organization_id="<YOUR-ORG-ID>", mql_query=[
{ '$match': { 'location_id': '<YOUR-LOCATION-ID>' } },
{ "$limit": 5 }
])

print(f"Tabular Data: {tabular_data}")
Expand All @@ -288,14 +289,18 @@ async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes
Args:
organization_id (str): The ID of the organization that owns the data.
You can obtain your organization ID from the Viam app's organization settings page.
mql_binary (List[bytes]): The MQL query to run as a list of BSON queries. You can encode your bson queries using a library like
`pymongo`.
mql_queries (Union[List[bytes], List[Dict[str, Union[ValueTypes, datetime]]]]): The MQL query to run as a list of BSON queries.
Note: Support for bytes will be removed in the future, so using a dictionary is preferred.

Returns:
List[Dict[str, Union[ValueTypes, datetime]]]: An array of decoded BSON data objects.

For more information, see `Data Client API <https://docs.viam.com/appendix/apis/data-client/>`_.
"""
if isinstance(mql_queries[0], dict):
mql_binary = [bson.encode(query) for query in mql_queries if isinstance(query, dict)]
else:
mql_binary = [query for query in mql_queries if isinstance(query, bytes)]
purplenicole730 marked this conversation as resolved.
Show resolved Hide resolved
request = TabularDataByMQLRequest(organization_id=organization_id, mql_binary=mql_binary)
response: TabularDataByMQLResponse = await self._data_client.TabularDataByMQL(request, metadata=self._metadata)
return [bson.decode(bson_bytes) for bson_bytes in response.raw_data]
Expand Down
2 changes: 1 addition & 1 deletion tests/test_data_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
)
BBOXES = [BBOX]
SQL_QUERY = "sql_query"
MQL_BINARY = [b"mql_binary"]
MQL_BINARY = [{"binary": "mql_binary"}]
TABULAR_DATA = {"key": "value"}
TABULAR_METADATA = CaptureMetadata(
organization_id=ORG_ID,
Expand Down
Loading