should there be an enum or something for this?

This is an option, I did not choose it because the backend defined it to be an integer, and I think we can let the backend handle value validation. What do you think?

In other repos, this is something I'd define an enum for, and then accept either the enum or a raw int value

But I don't have too much context on this repo, so up to you if that makes sense here. Consistency with the rest of the code is probably more relevant

I see, I think adding enum makes sense here - with enum we can prevent invalid numbers here.

Updated to use Enum and updated documentation.

No mention of None?

(It says 6 is the default. Is the server enforcing that, or the client? Can this even return None?)

The server does return None, if we do not set the value. This docstring is copied from the proto files. I just tried to set value to 6, and received the following error:
Invalid value for timestampPrecision: 6 is not a valid value

So here we might need to give up consistency with the proto and make sure the doc is user friendly. WDYT? I will also open a bug with the API team.

opened internal bug 463739109 and updated docstring.

Usually in protos, None is equivalent to 0. So I'd recommend removing the Optional here, and have it return 0 if unset.

But yeah, we should make sure we understand the expected values here first

This is REST so empty is indeed an accepted value

In other repos, this is something I'd define an enum for, and then accept either the enum or a raw int value

But I don't have too much context on this repo, so up to you if that makes sense here. Consistency with the rest of the code is probably more relevant

Usually in protos, None is equivalent to 0. So I'd recommend removing the Optional here, and have it return 0 if unset.

But yeah, we should make sure we understand the expected values here first

can we have a test that reads back a timestamp, and makes sure its in the expected range? Or am I misunderstanding?

This PR only involves creating and reading table schema that has picosecond timestamp. I think we can add the tests in the PR supporting writing to and reading from the table.

Like I mentioned in a different comment, we should make sure it's stored in a single format, to make things easier when we go back to read it. I'd recommend making this just return the enum type

done, when accessing the property, the user will always get an enum, while the api representation is int or None. We also limited the acceptable inputs to enum or None.

Also, this doctring doesn't seem to match the format of the others. Shouldn't it be in a returns block? And I'd suggest adding the type as an annotation, so it can be caught by mypy

both docstring and type annotation done

This type annotation isn't actually right with the current code, since it won't ever be a TimestampPrecision instance

This annotation is for the type accepted, so I think it is accurate?

updated to reflect the new types accepted: None and enum. The reason why None is accepted is to ensure an empty api representation when there is no user input, so the string representation remains the same if timestamp_precision is not set

Looking at the code again after our talk, I still think this should be turned into a true enum.

With the current code, TimestampPrecision is just a shortcut to creating int or None values.The argument type can't be TimeStampPrecision. This means all the docstrings have to describe all the values asUnion[int | None], and then do more work to describe how to build one or what those special values mean, and we lose a lot of the value of encapsulating it in an enum to begin with

If we change this to actually be an enum, we will have to convert it back to it's raw value before sending it to the backend. But it should make typing a lot cleaner

But if you want to leave it as-is, that's fine. We'd just need to update the typing and docstrings to be accurate

As we have discussed offline, I have updated to use enum.Enum, and took the liberty to support None to ensure backward compatibility regarding string representation

if you decide to do an Enum subclass, this could look like: self.timestamp_precision.value

if you make TimestampPrecision into an enum, you can save this as a primitive here, and then rebuild in the property

self._properties["timestampPrecision"] = timestamp_precision.value if isinstance(TimestampPrecision) else timestamp_precision

This seems a bit surprising. The backend returns"12"? Or is this converted somewhere locally

Yes, the backend expects an integer but returns a string. I will open a bug with the BigQuery team.

I was going to point out there was an error here, because the docstrings sayMapping but you're only checking againstdict. But I guess the actual type annotation saysdict. So there's definitely an inconsistency there, but maybe this logic is ok.

Still, I think this could be a bit cleaner as a try/catch

try:  # ensure timestamp is in int format  api_repr["timestampPrecision"] = int(api_repr["timestampPrecision"])except TypeError, KeyError:  # ignore unset timestamps  pass