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.

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

remove deepcopy from cube response #394

Merged
merged 11 commits into from
Feb 4, 2024

Conversation

ernestoarbitrio
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (049c0e3) 100.00% compared to head (18d28f3) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #394   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines         4619      4620    +1     
=========================================
+ Hits          4619      4620    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@gaetano-guerriero gaetano-guerriero left a comment

Choose a reason for hiding this comment

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

Great simplification in removing the deepcopy, I have a small question for _Measures

@@ -602,7 +597,7 @@ def _measures(self) -> "_Measures":
Provides access to count based measures and numeric measures (e.g. mean, sum)
when available.
"""
return _Measures(self._cube_dict, self._all_dimensions, self._cube_idx_arg)
return _Measures(self._cube_response, self._all_dimensions, self._cube_idx_arg)

Choose a reason for hiding this comment

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

Here we build _Measures without the "dimension inflation". Do we still need it ?

Copy link
Contributor Author

@ernestoarbitrio ernestoarbitrio Feb 2, 2024

Choose a reason for hiding this comment

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

actually self._all_dimensions has the inflation, having the inflation in the cube_response is not needed

Choose a reason for hiding this comment

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

ok nice 👍

@ernestoarbitrio ernestoarbitrio merged commit 990ec99 into master Feb 4, 2024
6 checks passed
@ernestoarbitrio ernestoarbitrio deleted the memory-leakage-deepcopy-186966946 branch February 4, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants