Skip to content

Add python serde support for varopt sketches#297

Merged
jmalkin merged 7 commits intoapache:masterfrom
jmalkin:python_serde
Aug 16, 2022
Merged

Add python serde support for varopt sketches#297
jmalkin merged 7 commits intoapache:masterfrom
jmalkin:python_serde

Conversation

@jmalkin
Copy link
Contributor

@jmalkin jmalkin commented Jul 26, 2022

The immediate result is for varopt, but having this working should be an important part of adding tuple support.

I'm least confident in the error recovery during deserialization. I think it's most likely that Python will throw an error which propagates through the c++ code. In this case I'm intercepting it to try to decrement reference counts for items we've already allocated but it's hard to tell if that's actually preventing a leak or not. Happy for suggestions on how to clean up that code.

Also, fixed a size bug in varopt.

@coveralls
Copy link

coveralls commented Jul 26, 2022

Pull Request Test Coverage Report for Build 2752291174

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.9%) to 98.227%

Totals Coverage Status
Change from base Build 2713877899: 5.9%
Covered Lines: 9420
Relevant Lines: 9590

💛 - Coveralls

Jon added 2 commits July 27, 2022 18:34
…d ctor with incorret use of filled_data_, and tidy up use of that flag.
Copy link
Contributor

@AlexanderSaydakov AlexanderSaydakov left a comment

Choose a reason for hiding this comment

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

I don't think I completely understand how all this comes together, but looks fine to me

@jmalkin jmalkin merged commit 1774ce3 into apache:master Aug 16, 2022
@jmalkin jmalkin deleted the python_serde branch August 16, 2022 08:31
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