search

amazon-ion / ion-python

A Python implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
260 stars 51 forks source link

Changes IonPyObjects' constructor to be same as their parents' classes. #298

Closed cheqianh closed 1 year ago

cheqianh commented 1 year ago

Description:

This PR is working on fixing https://github.com/amazon-ion/ion-python/issues/297. Users use the constructor and from_value method to construct Ion values. After the deprecation of IonNature, the constructor of IonPyObjects now have a from_value-like signature.

Problem statement:

there are a few concerns we have, we need to

In this PR, I allowed both (1) Users should be able to use from_value method to construct Ion values. (2) Users should be able to use IonPyObjects' constructor to construct Ion values, where it's inherited from their parent class (E.g., most of them are native python class such as Int, string)

However, to minimize the risk of breaking existing usage, the C extension return value constructions fall back to using from_value method so that IonValues constructions can reuse the existing signature.

Perf Benchmarking

name file_size(B) time_min(ns) time_mean(ns) memory_usage_peak(B)
Before the construction refactor PR 5208 1353876.58 1359532.68 475016
Before this PR 5208 935335.43 937701.30 365776
After this PR 5208 979675.29 989224.61 366144

which introduces a 5% perf decrement. (3.5% comparing the original refactor PR)

  By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

cheqianh commented 1 year ago

This looks good, but would you mind adding a few more tests that demonstrate that constructor invocations that were possible before the migration away from _IonNature are still possible after this change?

Sure, I had tests in the PR, but they were reverted by accident. I'll include them in the next commit.