[MINOR] Annotated decimal type #58
Conversation
Scale can be larger than precision
| def test_annotated_decimal_tuple_wrong_type(): | ||
| py_type = Annotated[decimal.Decimal, ("a", 1)] | ||
| with pytest.raises(pas.TypeNotSupportedError): | ||
| assert_schema(py_type, {}) |
There was a problem hiding this comment.
Would it make sense to have tests with all supported data classes to validate the type works?
So one for dataclasses and one for pydantic?
There was a problem hiding this comment.
that would only be necessary for the default value. The type itself can be tested without a data structure around it.
There was a problem hiding this comment.
hmm looks like it doesn't work like I though it would. this throws an error
from decimal import Decimal
from typing import Annotated
import py_avro_schema as pas
from pydantic import BeforeValidator, BaseModel
class Foo(BaseModel):
bar: Annotated[Decimal, (3, 2), BeforeValidator(lambda x: x)]
if __name__ == "__main__":
print(Foo(bar=Decimal("1.23")))
print(pas.generate(Foo))
Pydantic works, but pas.generate throws
py_avro_schema._schemas.TypeNotSupportedError: Cannot generate Avro schema for Python type <class 'decimal.Decimal'>
What am I doing wrong?
There was a problem hiding this comment.
@faph this still throws an error with the new DecimalMeta class used. Looks like py_type is decimal, and origin is none: what do i do wrong? 🤔
from decimal import Decimal
from typing import Annotated
import py_avro_schema as pas
from pydantic import Field, BaseModel
class Foo(BaseModel):
bar: Annotated[
Decimal, pas.DecimalMeta(3, 2), Field(max_digits=3, decimal_places=2)
]
if __name__ == "__main__":
print(Foo(bar=Decimal("1.23")))
print(pas.generate(Foo))
There was a problem hiding this comment.
It's because of this PydanticSchema code
# Pydantic 2 resolves forward references for us. To avoid infinite recursion, we check if the unresolved raw
# annoation is a forward reference. If so, we use that instead of Pydantic's resolved type hint. There might be
# a better way to un-resolve the forward reference...
# This does not support forward references from base model classes. If required we might need to traverse class
# hierarchy to get the raw annotations?
if isinstance(self.raw_annotations.get(name), (str, ForwardRef)):
py_type = self.raw_annotations[name]
else:
py_type = py_field.annotation
py_field.annotation is only decimal. The raw_annotations contains the fully annotated type definition.
There was a problem hiding this comment.
Fixed here @dada-engineer : f8ced26
There was a problem hiding this comment.
I am wondering whether this hashable requirement is not documented here: https://docs.python.org/3/library/typing.html?highlight=typing#typing.Annotated
There was a problem hiding this comment.
Raised the documentation issue here: python/cpython#112281
src/py_avro_schema/_schemas.py
Outdated
| """ | ||
|
|
||
| @classmethod | ||
| def handles_type(cls, py_type: Type) -> bool: | ||
| """Whether this schema class can represent a given Python class""" | ||
| try: | ||
| cls._size(py_type) # If we can find precision and scale, we are good |
There was a problem hiding this comment.
does the avro spec really require this? Can't one use decimals without precision / scale in avro? 🤔
There was a problem hiding this comment.
Good question, let's check.
I was going on the basis that to fully define a decimal, mathematically, you will need a value for scale and precision. But maybe there are some implicit defaults, e.g. zero scale.
There was a problem hiding this comment.
From https://avro.apache.org/docs/1.11.1/specification/#decimal
"scale, a JSON integer representing the scale (optional). If not specified the scale is 0."
"Precision must be a positive integer greater than zero"
There are some more restrictions.
The question though is whether there is a clean way to omit the scale from the Python (annotated) type hint.
There was a problem hiding this comment.
Options:
Annotated[decimal.Decimal, DecimalMeta(precision=4, scale=2)whereDecimalMetais a dataclass which defaults scale to zero if not specified.Annotated[decimal.Decimal, (4, 2)], usingAnnotated[decimal.Decimal, (4, None)]to default scale to zero.
Other options?
There was a problem hiding this comment.
I do like the first option as it would allow to set it as any arg and not strictly the second arg of Annotated. It's easier to iterate through and find exactly the arg needed. Seems reasonable as more and more libs use this mechanism.
There was a problem hiding this comment.
yeah, good point about the arg position
There was a problem hiding this comment.
The only downside is that it will again require a py-avro-schema specific "type". That might simply the price to pay for a robust type hint. I couldnt find anyone else having defined a decimal meta type struct...
There was a problem hiding this comment.
An option would be to get this from field info of the class itself using dataclasse.Field.metadata (py-avro-schema can choose how this metadata looks it is specifically for third party extension) and in pydantic from FieldInfo of a decimal. If there is non set some sane default?
…feature/annotated-decimal
Would fix #57