Timestamps without precision does not serialize/deserialize correctly

[jackhxs-alt]

Description

• Ubuntu 20.04
• Python 3.9.9
• amazon.ion version 0.9.2

Actual:
Serializing then deserializing an instance of IonPyTimestamp without a precision field returns a different timestamp than the original.

Expected:
It should return a timestamp equal to the original.

Repro

On python interpreter:

Python 3.9.9 (main, Nov 23 2021, 15:07:55) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from amazon.ion import simpleion, simple_types
>>> a = simple_types.IonPyTimestamp.fromisoformat('2022-07-19T15:10:15.412119+00:00')
>>> a
Timestamp(2022-07-19T15:10:15.412119, datetime.timezone.utc, None, fractional_precision=6)
>>> b = simpleion.loads(simpleion.dumps(a))
>>> b
Timestamp(2022-01-01T00:00:00.000000, None, <TimestampPrecision.YEAR: 0>, fractional_precision=0)

[cheqianh]

Thanks for letting us know, I'll schedule time to investigate the issue.

[popematt]

Thank you for bringing this to our attention.

It looks like this is only an issue when using the C extension, so if your use case is not performance sensitive, there is a workaround. You can disable the C extension by setting simpleion.c_ext = False.

We'll need to look into this further to determine where exactly it is going wrong.

[popematt]

After looking into this some more, it seems like it should be possible for you to parse your ISO string to an IonPyTimestamp without disabling the C extension using simpleion.loads('2022-07-19T15:10:15.412119+00:00').

ISO 8601 defines many datetime formats, but Ion does not use all of them. simpleion.loads(...) will only work to parse Ion timestamps—practically, this includes anything allowed by the W3 note on datetimes except that T must always be present (e.g. 2022-07T) even if there is no time component.

[jackhxs-alt]

Thanks for investigating! Following up:

1. Considering that the precision field is nullable and that IonPyTimestamp is a subclass of datetime, so fromisoformat is in theory a supported operation. Given that, one would expect an instance with a None precision field to still serialize and deserialize correctly right?
2. Maybe related issue: if I clone an instance of IonPyTimestamp via copy.deepcopy, the precision field does not get copied and ends up being None, leading to this issue.

[popematt]

Considering that the precision field is nullable and that IonPyTimestamp is a subclass of datetime, so fromisoformat is in theory a supported operation. Given that, one would expect an instance with a None precision field to still serialize and deserialize correctly right?

Yes.

However, I think fromisoformat is just a symptom of a deeper issue because you can also reproduce the same behavior in other ways. This example produces the same output as your original example.

import datetime
from amazon.ion import simpleion, core
a = core.Timestamp(2022, 7, 19, 15, 10, 15, 412119, datetime.timezone.utc)
a
b = simpleion.loads(simpleion.dumps(a))
b

Maybe related issue: if I clone an instance of IonPyTimestamp via copy.deepcopy, the precision field does not get copied and ends up being None, leading to this issue.

This sounds like a different issue. It's related in that it may have helped to expose the serialization problem, but I think it deserves a new issue on GitHub because we can be reasonably sure that it will have a different root cause.

[popematt]

Well, I have a root cause for you. Here's how precision=None is handled in the various writer implementations:

Ion Binary, pure python

The writer assumes that when precision=None, it should use a default of SECOND.
https://github.com/amzn/ion-python/blob/d87a99f49e1ea1786fd5f3c674d49165e44235e4/amazon/ion/writer_binary_raw.py#L244-L246

Ion Text, pure python

The writer has no default, and so if precision=None, it raises an AttributeError because None doesn't have the attributes of TimestampPrecision (such as includes_month).
https://github.com/amzn/ion-python/blob/d87a99f49e1ea1786fd5f3c674d49165e44235e4/amazon/ion/writer_text.py#L145

C Extension

At first glance, the C extension looks like it has the same logic as found in text_writer.py.
https://github.com/amzn/ion-python/blob/d87a99f49e1ea1786fd5f3c674d49165e44235e4/amazon/ion/ioncmodule.c#L651
But, it defaults to a precision of 0 (or YEAR).
https://github.com/amzn/ion-python/blob/d87a99f49e1ea1786fd5f3c674d49165e44235e4/amazon/ion/ioncmodule.c#L126-L144

---

There are at least three possible solutions:

1. Update the logic in all of the writers and the C extension to have the same default value for precision. This is the easy solution.
2. Make precision a required field.
3. Update Timestamp to intelligently figure out the correct precision when it is constructed. E.g. if the constructor is invoked with only 3 args, then the precision is inferred to be DAY.

A few thoughts about them:

• (1) seems to be the simplest to implement
• (2) seems relatively simple to implement, but it would make Timestamp.fromisoformat (and IonPyTimestamp.fromisoformat) awkward to use because of the extra required parameter.
• (3) is the most complex to implement, but probably the most ergonomic for users of the library.
• (2) and (3) are backwards incompatible (though you could make a case that any fix is backwards incompatible)
• (2) and (3) are the least surprising solutions.

[popematt]

Here's a really scrappy proof-of-concept to show that solution 3 is possible. popematt@4ffd405

It doesn't fix the inconsistencies in the writers (which probably still needs to be done given that a date or datetime will not have a precision field), but it does demonstrate an implementation of fromisoformat, and it updates the Timestamp constructor to infer the correct precision if no precision is explicitly provided.

[cheqianh]

@jackhxs-alt Thanks for finding a bug, I addressed the issue in #211. it works as below,

# Before
a = simple_types.IonPyTimestamp.fromisoformat('2022-07-19T15:10:15.412119+00:00')
print(a)          # returns 2022-01-01 00:00:00.000000+00:00
# After
a = simple_types.IonPyTimestamp.fromisoformat('2022-07-19T15:10:15.412119+00:00')
print(a)          # returns 2022-07-19 15:10:15.412119+00:00

The question turns out how to construct an Ion timestamp object. In Ion data model, 2022-07-01T00:00:00.000000-00:00 and 2022-07-01T are not equivalence since they have different fraction precision.

For example, if someone wants to construct a timestamp by its constructor other than simpleion.loads,

obj = simple_types.IonPyTimestamp.fromisoformat("2022-07-01")

currently simpleion.dumps(obj) returns 2022-07-01T00:00:00.000000-00:00 instead of 2022-07-01T.

For three possible solutions @popematt provided, we are kind of doing the first one as C extension defaults timestamp's precision to 6 for native datetime type currently. And we should implement the third solution to avoid any future confusion. I opened #212 to keep this on track. I also think we need to implement fromisoformat constructor (E.g. we override the datetime's fromisoformat that Matthew implemented in his proof-of-concept PR above)

I'll close this issue out but feel free to reopen it for any further question, and I'll follow up with #212.