Feature/odata compact json #644
Conversation
|
Awesome, excited to see some performance metrics.
…Sent from my mobile phone
On Nov 14, 2021 9:22 AM, gbryant-dev ***@***.***> wrote:
Introduces the option to use compact JSON format when querying cellsets. Beneficial when querying large amounts of data and / or remote environments where network latency is typically higher.
Implemented through a new odata_compact_json decorator which handles the header and maps the data back to the cellset properties based on the odata context. Whether the properties are returned is based on return_props_with_data is True. The odata context returned in the response is used to parse the response. Currently only supports requests that query only cell properties (see notes in decorator function).
Core low level extract functions have been decorated, namely extract_cellset_raw and extract_cellset_values. Some refactoring was required, particularly with the former to separate the metadata and cell calls.
A new parameter use_compact_json has been added to all of the relevant functions. Defaults to False.
A couple of reservations / thoughts I have on the approach:
* Updating the Accept header - It's a little tricky because positioning matters and it needs to come after application/json. The default header defined in the RestService is application/json;odata.metadata=none,text/plain and in order to use the compact JSON format, it would be application/json;tm1.compact=v0;odata.metadata=none,text/plain. I've taken the approach of inserting the header in the right place as opposed to assuming what the default header contains and setting a new one directly.
* Using compact JSON for metadata calls - It would be nice to be able to support this as well. However, the odata context only contains properties explicitly asked for on each Entity and doesn't include properties that are returned by default e.g a request to Cellsets('...')?$expand=Axes would lead to an odata context of $metadata#Cellsets(Axes)/$entity but Ordinal and Cardinality properties would be included in the response by default. In order to parse the response correctly, the odata context would need to be $metadata#Cellsets(Axes(Ordinal,Cardinality)/$entity. In order to implement fully, based on what I know so far, besides the parser being very robust to handle all the variations and navigation options, possible workarounds could be to:
* Ensure all requests specify properties for each Entity, expand options included.
* Hold a lookup to the default properties for each Entity
I'm not a fan of either approach really but the 1st option would appear to be more in our control. That being said, I think this PR is a good start so maybe something we come back to.
Thanks,
George
________________________________
You can view, comment on, or merge this pull request online at:
#644
Commit Summary
* 419852c<419852c> Add utils to extract and map properties to data
* 793630d<793630d> Add odata_compact_json decorator
* 0604b70<0604b70> Refactor core extract functions to use compact json
* 2e8317e<2e8317e> Pass through use_compact_json parameter to functions
* c380223<c380223> Enforce parameter on decorator
File Changes
(2 files<https://github.com/cubewise-code/tm1py/pull/644/files>)
* M TM1py/Services/CellService.py<https://github.com/cubewise-code/tm1py/pull/644/files#diff-5242c6c19161de7f529a4e5b0d0b33ca9c15f69be65fa5409881b3d8310ff655> (295)
* M TM1py/Utils/Utils.py<https://github.com/cubewise-code/tm1py/pull/644/files#diff-dfa6efa267891cf338ac4ae2fb7d41b941f6ecef55b265005ff460b33c9e8f9d> (32)
Patch Links:
* https://github.com/cubewise-code/tm1py/pull/644.patch
* https://github.com/cubewise-code/tm1py/pull/644.diff
-
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#644>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEK7GZR3RLRYWVZBSJQUCQLUL7V3LANCNFSM5IACIQ4Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Thank you @gbryant-dev. This looks like more than a good start! I kinda think we should have been explicit about the expands from the beginning, anyways. I wouldn't mind doing a quick refactoring exercise to tidy this up. I will provide more feedback and help with the code this week after the other open MR is merged. |
|
Thanks @MariusWirtz and apologies for the late follow up. Agree with being explicit when requesting properties for each entity. I think we would need to go down this road in order to utilise compact JSON fully. My initial reservation was around the governance to ensure it's enforced across all the current and new functions in the CellService that use the decorator. That said, using the decorator is an opt-in approach so as long as it's clear that the decorated function is required to specify properties in the request, I think that covers it. I've pushed a couple of more commits to tidy up a bit and do some renaming so hopefully the utility functions make more sense but I welcome your feedback on this. I would like to take the opportunity to write some tests but I likely won't get round to that for a couple of weeks yet so don't wait for me on that. What I have done on testing so far is run the unit tests for the CellService using pytest and done some manual testing on running functions with |
|
Very nice work. I like the approach to using a decorator to manage the header. The Utils functions look very tidy too. I have started to go through the code and to solve the conflicts between this and #612. Agree. It would be nice to have some tests for it. Also for the Utils functions. |
|
I am currently working to resolve the conflicts, and I think maybe we should harmonize the naming of the two new features. I think it would be nice if we called them either:
Does anyone have preferences? Or other suggestions? |
|
I think the |
|
agree
From: gbryant-dev ***@***.***>
Reply-To: cubewise-code/tm1py ***@***.***>
Date: Thursday, December 2, 2021 at 11:04 AM
To: cubewise-code/tm1py ***@***.***>
Cc: "Clapp, Ryan" ***@***.***>, Comment ***@***.***>
Subject: Re: [cubewise-code/tm1py] Feature/odata compact json (PR #644)
I think the use_ convention makes more sense in this case. There are also two other arguments used in the CellService that align with this - use_ti and use_changeset.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#644 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEK7GZXBTDPD4BHU7KXDPEDUO67IVANCNFSM5IACIQ4Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Substituted by #650 |
Introduces the option to use compact JSON format when querying cellsets. Beneficial when querying large amounts of data and / or remote environments where network latency is typically higher.
Implemented through a new
odata_compact_jsondecorator which handles the header and maps the data back to the cellset properties based on the odata context. Whether the properties are returned is based onreturn_props_with_dataisTrue. The odata context returned in the response is used to parse the response. Currently only supports requests that query only cell properties (see notes in decorator function).Core low level extract functions have been decorated, namely
extract_cellset_rawandextract_cellset_values. Some refactoring was required, particularly with the former to separate the metadata and cell calls.A new parameter
use_compact_jsonhas been added to all of the relevant functions. Defaults toFalse.A couple of reservations / thoughts I have on the approach:
Updating the
Acceptheader - It's a little tricky because positioning matters and it needs to come after application/json. The default header defined in the RestService isapplication/json;odata.metadata=none,text/plainand in order to use the compact JSON format, it would beapplication/json;tm1.compact=v0;odata.metadata=none,text/plain. I've taken the approach of inserting the header in the right place as opposed to assuming what the default header contains and setting a new one directly.Using compact JSON for metadata calls - It would be nice to be able to support this as well. However, the odata context only contains properties explicitly asked for on each Entity and doesn't include properties that are returned by default e.g a request to
Cellsets('...')?$expand=Axeswould lead to an odata context of$metadata#Cellsets(Axes)/$entitybut Ordinal and Cardinality properties would be included in the response by default. In order to parse the response correctly, the odata context would need to be$metadata#Cellsets(Axes(Ordinal,Cardinality)/$entity. In order to implement fully, based on what I know so far, besides the parser being very robust to handle all the variations and navigation options, possible workarounds could be to:I'm not a fan of either approach really but the 1st option would appear to be more in our control. That being said, I think this PR is a good start so maybe something we come back to.
Thanks,
George