-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVRO-3779: [java] any big decimal conversion #2282
Conversation
What I like: the scale is part of the value, giving greater flexibility when reading (schema resolution does not provide a widening conversion for logical types like Personally, I would really like this to be documented in this PR. |
thanks, |
Doc added. |
Just wondering: Could this be done with I'm asking because the new |
In rust, this big decimal definition is near the one of Java, as it contains constructor as In C#, big rational seems even better, as it's directly a fraction of 2 big integer (so, support easily 10/3, which is not obvious in Java), but not directly compatible (but we can build seralizer / deserializer that can deal with this format). i didn't look in other language, in any case, it should be possible to use the proposed stored format to a defined serialization (even if i first code it for Java). For the moment, i didn't see logical type with "java-type" arguments; may be i could add "lang" arg ? |
this PR does same for rust BigDecimal |
|
||
@Override | ||
public String getLogicalTypeName() { | ||
return "bigdecimal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name does not equals the one in the documentation and the tests. Can we please adjust so they're the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
4289c51
to
973af4a
Compare
For C#, the Apache.Avro library already defines an AvroDecimal type with a |
So, we can merge this and create a PR for C# that will map this big-decimal type with C# AvroDecimal type ? |
Yes I think that would be okay. However, I feel there should be a list of Avro spec features that are not yet supported in all languages. So that spec changes can be approved and merged without waiting for them to be implemented in all languages, but developers of schemas can avoid using features that might not be supported in languages used by their partners. |
**alternative** | ||
|
||
As it's not always possible to fix scale and precision in advance for a decimal field, `big-decimal` is another `decimal` logical type restrict to Avro _bytes_. | ||
|
||
_only available in Java_ | ||
|
||
```json | ||
{ | ||
"type": "bytes", | ||
"logicalType": "big-decimal" | ||
} | ||
``` | ||
Here, as scale property is stored in value itself it needs more bytes than preceding `decimal` type, but it allows more flexibility. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No comments on the Java code, but I'd like to see the specification clarified a bit.
-
Separate the "big-decimal" description under its own
### Decimal (variable scale)
heading, for these purposes:- Gives it an ID in HTML so that it can be linked to.
- Makes it show up in the table of contents.
- Makes it clear that the schema-resolution matching rule of "decimal" does not apply.
-
Describe how the decimal value is encoded. AFAICT, it goes like this:
A
big-decimal
logical type annotates an Avrobytes
type, which stores the binary encoding of the following record:{ "type": "record", "name": "BigDecimal", "doc": "Avro 'big-decimal' value encoded within 'bytes'", "fields": [ { "name": "unscaled", "type": "bytes", "doc": "The two’s-complement representation of the unscaled integer value in big-endian byte order" }, { "name": "scale", "type": "int", "doc": "How many digits of the unscaled value lie at the right side of the decimal point. Must not be negative." } ] }
Or describe it in prose -- as long as the specification is clear enough that the logical type can be implemented without looking at how the Java code does it.
I don't intend this comment to delay the merging of this pull request. So far, all Avro logical types annotate {
"type": "record",
"namespace": "apache.avro.test",
"name": "RecordOfTwoBigDecimals",
"fields": [
{
"name": "dec1",
"type": {
"type": "record",
"name": "apache.avro.BigDecimal",
"fields": [
{
"name": "unscaled",
"type": "bytes"
},
{
"name": "scale",
"type": "int"
}
],
"logicalType": "big-decimal"
}
},
{
"name": "dec2",
"type": "apache.avro.BigDecimal"
}
]
}
Overall, the difficulties of that approach seem to outweigh the benefits. |
Java accept named schema to have logical type, but, specially with second Con, i agree with you. |
That would not actually be a problem, because the type of the "unscaled" field would be "bytes", and its value would thus be encoded in JSON as a string rather than a number. |
Hey guys! Do you have an ETA for the release of this change by any chance? |
I don't know when the 1.12.0 is planned for release, i can put this in 1.11 branch for the future 1.11.4 release |
That would be awesome @clesaec ! |
PR ready to be reviewed @rotilho-bv |
* AVRO-3779: any big decimal conversion
What is the purpose of the change
AVRO-3779 propose a new feature for big decimal conversion. Current decimal conversion require to anticipate precision and scale, and so, to know in advance approximative value of it.
This conversions, added to existing, allow to deal with completly random big decimal value. Value is store in byte buffer, completly non human readable, but keep the value.
Verifying this change
This change added tests 'TestBigDecimalConversion' and can be verified as follows:
Run junit new test, or run all unit test of java module
Documentation