-
Notifications
You must be signed in to change notification settings - Fork 46
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
Update tabular data examples #611
Conversation
src/viam/app/data_client.py
Outdated
@@ -255,10 +255,14 @@ async def tabular_data_by_sql(self, organization_id: str, sql_query: str) -> Lis | |||
|
|||
async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes]) -> List[Dict[str, ValueTypes]]: | |||
"""Obtain unified tabular data and metadata, queried with MQL. | |||
You must install the `pymongo` package and `import bson` to use the `tabular_data_by_mql` method. |
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.
Is this accurate? I don't think this is strictly necessary -- you can use any bson serialization library. I would change this to be more of a suggestion, something like:
mql_binary
is an array of encodedbson
queries. You can encode yourbson
queries using a library likepymongo
.
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.
well you cannot use the regular bson library that I first had installed. hyperopt/hyperopt#547
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.
What happens when you try to use the bson
library? I was just able to do this:
import bson
bson.dumps({ '$match': { 'location_id': '<location-id>' }})
And that outputs a valid bson binary. I'm curious as to the functionality you are seeing.
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.
that does indeed work. You can't call encode on that. I think that's what I was trying before and it has just a varietey of encode_*
methods
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 reasonable to me though I don't have a ton of experience with bson. I assume this was all tested and works?
tested both, yes |
src/viam/app/data_client.py
Outdated
@@ -255,10 +255,14 @@ async def tabular_data_by_sql(self, organization_id: str, sql_query: str) -> Lis | |||
|
|||
async def tabular_data_by_mql(self, organization_id: str, mql_binary: List[bytes]) -> List[Dict[str, ValueTypes]]: | |||
"""Obtain unified tabular data and metadata, queried with MQL. | |||
You must `import bson` to create valid bson binary objects as input for the `tabular_data_by_mql` method. |
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.
Again, I don't want to prescribe a library, particularly given that the bson
library hasn't been updated in 3 years. Further, a library isn't even technically required -- you could encode the bson by hand.
My suggestion would be to remove the must
, and then provide an example of using a library:
async def tabular_data_by_mql(...):
"""Obtain unified tabular data and metadata, queried with MQL.
`mql_binary` is an array of encoded `bson` queries. You can encode your bson queries using a library like `pymongo` or `bson`.
::
# using bson
import bson
data = ...(..., mql_binary=[bosn.dumps(...)])
# using pymongo
import bson
data = ...(..., mql_binary=[bosn.encode(...)])
"""
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.
You didn't actually say that before. I'm ok using your proposal.
No description provided.