Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1659426311
##########
lang/c++/api/LogicalType.hh:
##########
@@ -47,17 +47,17 @@ public:
// Precision must be positive and scale must be either positive or zero.
The
// setters will throw an exception if they are called on any type other
// than DECIMAL.
- void setPrecision(int precision);
- int precision() const { return precision_; }
- void setScale(int scale);
- int scale() const { return scale_; }
+ void setPrecision(int64_t precision);
Review Comment:
Fair enough, I've instead added `static_cast` to where we read this field
##########
lang/c++/impl/Node.cc:
##########
@@ -146,7 +146,7 @@ void Node::setLogicalType(LogicalType logicalType) {
if (type_ == AVRO_FIXED) {
// Max precision that can be supported by the current size of
// the FIXED type.
- long maxPrecision = floor(log10(2.0) * (8.0 * fixedSize() -
1));
+ int32_t maxPrecision = static_cast<int32_t>(floor(log10(2.0) *
(8.0 * static_cast<double>(fixedSize()) - 1)));
Review Comment:
@Fokko I didn't introduce this here, so I don't think I should fix it
here... but I believe this calculation is incorrect.
According to [the
spec](https://avro.apache.org/docs/1.11.1/specification/#decimal), the maximum
precision should be:
$$
floor(log_{10}(2^{8 \times n - 1} - 1))
$$
Rust is the only language implementation that appears to implement this
correctly. Python, Java, and C++ all implement this as:
$$
floor(log_{10}(2^{8 \times n - 1}))
$$
JavaScript appears to have an example of a `DecimalType` with this same
issue, but it doesn't appear to be in any library code shipped by the library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
