You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mzzz-zzm (via GitHub)" <gi...@apache.org> on 2026/05/06 13:55:25 UTC
[I] `convertDecimalValue` passes string length instead of declared precision to `DecimalRequiredBytes` [iceberg-go]
mzzz-zzm opened a new issue, #1027:
URL: https://github.com/apache/iceberg-go/issues/1027
### Apache Iceberg version
main (development)
### Please describe the bug 🐞
**Component**: `manifest.go`
**Affected version**: `main` (v0.5.0)
**Severity**: Data corruption / Avro encode error for decimal partition columns
---
`convertDecimalValue` computes the Avro `fixed[N]` byte size by passing
`len(dec.String())` to `internal.DecimalRequiredBytes`, but that function
expects the column's declared **precision**. These two quantities are unrelated,
causing the wrong number of bytes to be allocated and the Avro encoder to reject
the value.
---
## Affected code
`manifest.go`, `convertDecimalValue`:
```go
func convertDecimalValue(v any) any {
if dec, ok := v.(Decimal); ok {
fixedSize := internal.DecimalRequiredBytes(len(dec.String())) // ← BUG
bytes, err := DecimalLiteral(dec).MarshalBinary()
...
return padOrTruncateBytes(bytes, fixedSize)
}
return v
}
```
`internal.DecimalRequiredBytes` signature (`internal/avro_schemas.go`):
```go
// DecimalRequiredBytes returns the required number of bytes to store a
// decimal value with the given precision.
func DecimalRequiredBytes(precision int) int { ... }
```
---
## Root cause
`dec.String()` returns the human-readable decimal string (e.g. `"1.00"`).
Its character length is not the declared precision of the column — it varies
with the specific value being encoded.
| Column type | Value | `dec.String()` | `len(...)` | `DecimalRequiredBytes(len)` | Correct `DecimalRequiredBytes(precision)` |
|---------------|---------|----------------|------------|-----------------------------|-------------------------------------------|
| `decimal(10,2)` | `10.00` | `"10.00"` | 5 | 3 bytes | 5 bytes |
| `decimal(10,2)` | `1.00` | `"1.00"` | 4 | 2 bytes | 5 bytes ← **wrong** |
| `decimal(18,0)` | `1` | `"1"` | 1 | 1 byte | 8 bytes ← **wrong** |
The `decimal(10,2)` / `10.00` case produces the right size by coincidence
(`len("10.00") == 5 == DecimalRequiredBytes(10)`), masking the bug for that
specific value.
---
## Symptom
Any write of a manifest with a decimal partition column where the value's
string representation length differs from the column's precision produces:
```
avro: field data_file.partition.<field>: cannot use []uint8 with Avro type fixed
```
---
## Reproduction
1. Create a table with partition spec `identity(price)` where `price` is `decimal(10, 2)`.
2. Write a `RowDelta` that adds a data file with partition value `1.00`
(string length 4, but declared precision is 10).
3. Observe the Avro encode error when the manifest is flushed.
---
## Proposed fix
Thread the column's declared precision from the `DecimalType` of the partition
field through to `convertDecimalValue`:
```go
// Pass precision from the field's type, not from the value's string length.
func convertDecimalValue(v any, precision int) any {
if dec, ok := v.(Decimal); ok {
fixedSize := internal.DecimalRequiredBytes(precision)
bytes, err := DecimalLiteral(dec).MarshalBinary()
if err != nil {
return v
}
return padOrTruncateBytes(bytes, fixedSize)
}
return v
}
```
The call site in the partition value serialization loop already has access to the
`PartitionField` → `DecimalType` → `Precision`.
--
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: issues-unsubscribe@iceberg.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-4445845677
Thanks for the thorough analysis @mzzz-zzm, the bug is real and well-documented, and the Java reference walkthrough nails the correct shape.
I agree with the three-part approach in your second comment:
1. Rename fieldIDToFixedSize → fieldIDToScale (it's already being used as scale on the read path at line 1903, so the name is just wrong today)
2. Add fieldIDToPrecision map populated from typ.Precision in getFieldIDMap
3. Thread precision into convertDecimalValue via avroPartitionData → convertLogicalTypeValue
One additional thing worth noting from the write path: [getFieldIDMap is called at line 1174](https://github.com/tanmayrauth/iceberg-go/blob/775d8aa56e5f509ecd6a0be148787ec6230139bc/manifest.go#L1174-L1173) for ManifestWriter but the third return value (the current fixedSizes map) is discarded with _. The fix should wire the new precision map into the writer so it's available when serializing partition values.
Regarding the sign-extension point for negative decimals, let's keep that as a separate issue/PR since MarshalBinary on DecimalLiteral should already produce two's complement bytes, but it's worth verifying independently.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-5276415108
@zeroshade I think we can close this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-5016867440
Working on this to raise a new PR with fixes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "mzzz-zzm (via GitHub)" <gi...@apache.org>.
mzzz-zzm commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-4403317385
While digging into this, I noticed the precision/size confusion isn't isolated to `convertDecimalValue` — there's a sibling issue in `getFieldIDMap` that I think is worth resolving in the same patch, since a clean fix for #1027 needs to thread the right metadata through anyway.
In `manifest.go`, `getFieldIDMap` builds a `fixedSizes` map but actually populates it with the field's scale:
```go
fixedSizes := make(map[int]int)
// ...
if typ.LogicalType == atype.Decimal {
fixedSizes[fid] = typ.Scale // name says "size", value is scale
}
```
This map is then plumbed through `hasFieldToIDMap.setFieldIDToFixedSizeMap` into `dataFile.fieldIDToFixedSize`, and on the read path `convertAvroValueToIcebergType` consumes it as scale:
```go
case atype.Decimal:
if r, ok := v.(*big.Rat); ok {
scale := d.fieldIDToFixedSize[fieldID] // read back as scale
// ...
}
```
So the read path happens to work because both ends agree to (mis)use the field as scale, but:
1. The name `fieldIDToFixedSize` / `setFieldIDToFixedSizeMap` is actively misleading — it's really `fieldIDToScale`.
2. It also means we currently have **no** plumbed-through value for the actual fixed byte size (or for precision) on the write path, which is exactly what `convertDecimalValue` needs to fix #1027.
### Suggestion
Rather than just adding a `precision int` parameter to `convertDecimalValue`, it would be cleaner to:
1. Rename `fieldIDToFixedSize` → `fieldIDToScale` (and the setter accordingly), since that's what it actually holds. This is a localized rename — the field, its setter on `hasFieldToIDMap`, the local in `getFieldIDMap`, and the read in `convertAvroValueToIcebergType`.
2. Add a separate `fieldIDToPrecision map[int]int]` populated from `typ.Precision` in `getFieldIDMap`, plumbed through `hasFieldToIDMap` the same way.
3. Have `avroPartitionData` / `convertLogicalTypeValue` pass `precision` into `convertDecimalValue`, which then calls `internal.DecimalRequiredBytes(precision)` — exactly the fix proposed in the original report, just sourced from real schema metadata rather than `len(dec.String())`.
Happy to throw up a PR if you like this shape. Just wanted to flag the rename first — it's not strictly needed to fix the encoding error, but if we add precision plumbing right next to a field called `fixedSize` that actually holds scale, things are gonna get confusing fast.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "mzzz-zzm (via GitHub)" <gi...@apache.org>.
mzzz-zzm commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-4427121323
Confirmed numerically.
Across every realistic case I tested,
| precision | value | len | DRB(len) | DRB(precision) | match |
|-----------|-----------------|-----|----------|----------------|-------|
| 10 | "10.00" | 5 | 3 | 5 | ✗ |
| 10 | "1.00" | 4 | 2 | 5 | ✗ |
| 10 | "0.50" | 4 | 2 | 5 | ✗ |
| 10 | "1234567890.99" | 13 | 6 | 5 | ✗ |
| 18 | "1" | 1 | 1 | 8 | ✗ |
| 18 | "1.00" | 4 | 2 | 8 | ✗ |
| 38 | "1" | 1 | 1 | 16 | ✗ |
| 38 | "0.000000001" | 11 | 5 | 16 | ✗ |
So the bug actually triggers for all non-degenerate cases, not just some. One inaccuracy in the report: the claim that `decimal(10,2) / 10.00` "produces the right size by coincidence (`len("10.00") == 5 == DecimalRequiredBytes(10)`)" is wrong. `DRB(len("10.00")) = DRB(5) = 3`, not 5. The report's own table actually shows 3 vs 5 for that row, contradicting its prose. The bug bites that case too.
There is **no value that masks the bug** — `len(dec.String())` essentially never equals `precision` for decimals with a decimal point, and `DRB(len) == DRB(precision)` requires collision within the lookup table, which doesn't happen for any of the realistic cases checked.
So the bug is broader than the report itself states.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "mzzz-zzm (via GitHub)" <gi...@apache.org>.
mzzz-zzm commented on issue #1027:
URL: https://github.com/apache/iceberg-go/issues/1027#issuecomment-4427213619
## How the Java reference implementation handles this
Confirmed by reading `apache/iceberg` Java source. The invariant is:
> Precision is **always** sourced from the column's `DecimalType.precision()`,
> never from the value. It flows schema → writer → fixed buffer.
The pipeline:
### 1. Schema construction encodes precision directly
`core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java` (~line 268):
```java
case DECIMAL:
Types.DecimalType decimal = (Types.DecimalType) primitive;
primitiveSchema =
LogicalTypes.decimal(decimal.precision(), decimal.scale())
.addToSchema(
Schema.createFixed(
"decimal_" + decimal.precision() + "_" + decimal.scale(),
null, null,
TypeUtil.decimalRequiredBytes(decimal.precision())));
```
The Avro `fixed[N]` length is fixed at schema-build time to `decimalRequiredBytes(precision)`. Same `DecimalRequiredBytes` lookup table as in `iceberg-go`.
### 2. Writer is constructed from the schema's LogicalType
`core/src/main/java/org/apache/iceberg/avro/BaseWriteBuilder.java` (~line 92):
```java
case "decimal":
LogicalTypes.Decimal decimal = (LogicalTypes.Decimal) logicalType;
return ValueWriters.decimal(decimal.getPrecision(), decimal.getScale());
```
Precision is read off the *schema's logical type*, not the value.
### 3. Writer pre-allocates the fixed buffer once, off precision
`core/src/main/java/org/apache/iceberg/avro/ValueWriters.java` (~line 367):
```java
private static class DecimalWriter implements ValueWriter<BigDecimal> {
private final int precision;
private final int scale;
private final ThreadLocal<byte[]> bytes;
private DecimalWriter(int precision, int scale) {
this.precision = precision;
this.scale = scale;
this.bytes =
ThreadLocal.withInitial(() -> new byte[TypeUtil.decimalRequiredBytes(precision)]);
}
@Override
public void write(BigDecimal decimal, Encoder encoder) throws IOException {
encoder.writeFixed(
DecimalUtil.toReusedFixLengthBytes(precision, scale, decimal, bytes.get()));
}
}
```
### 4. The actual byte conversion validates and pads from the precision
`core/src/main/java/org/apache/iceberg/util/DecimalUtil.java` (~line 31):
```java
public static byte[] toReusedFixLengthBytes(
int precision, int scale, BigDecimal decimal, byte[] reuseBuf) {
Preconditions.checkArgument(
decimal.scale() == scale,
"Cannot write value as decimal(%s,%s), wrong scale: %s", precision, scale, decimal);
Preconditions.checkArgument(
decimal.precision() <= precision,
"Cannot write value as decimal(%s,%s), too large: %s", precision, scale, decimal);
byte[] unscaled = decimal.unscaledValue().toByteArray();
if (unscaled.length == reuseBuf.length) {
return unscaled;
}
byte fillByte = (byte) (decimal.signum() < 0 ? 0xFF : 0x00);
int offset = reuseBuf.length - unscaled.length;
for (int i = 0; i < reuseBuf.length; i += 1) {
reuseBuf[i] = (i < offset) ? fillByte : unscaled[i - offset];
}
return reuseBuf;
}
```
Note two write-time invariants that `iceberg-go` does **not** enforce today:
- `decimal.scale() == scale` — the value's scale must match the column's.
- `decimal.precision() <= precision` — the value must fit.
And the sign-extending pad (`0xFF` for negative, `0x00` for non-negative) is also worth comparing against `iceberg-go`'s `padOrTruncateBytes`, which always pads with `0x00`. That is a separate latent bug for negative decimals near a byte boundary, though it may be moot if `MarshalBinary` already returns sign-extended bytes (PR #745 era).
### Mapping to the Go fix
The Java approach maps cleanly onto the suggested `iceberg-go` fix:
| Java | Go equivalent |
|---------------------------------------------------------------------|----------------------------------------------------------------------|
| `TypeToSchema` encodes `decimalRequiredBytes(precision)` in schema | `internal.DecimalNode(precision, scale)` already does this |
| `BaseWriteBuilder` reads precision off the schema's logical type | `getFieldIDMap` needs to populate `fieldIDToPrecision` from `typ.Precision` |
| `DecimalWriter` carries `precision` per field | `convertDecimalValue` should take `precision` as a parameter |
| `toReusedFixLengthBytes(precision, scale, decimal, buf)` validates and sign-extends | Go `padOrTruncateBytes` should size off `DRB(precision)` and sign-extend for negatives |
The Go bug exists because `convertDecimalValue` was the only step in the pipeline that lost track of the per-field precision — the schema already had it (`DecimalNode` builds the fixed size from precision), but the runtime value serializer fell back to `len(dec.String())` because no precision was plumbed through. The Java code shows the correct shape: precision is captured once when the writer is constructed from the schema and reused for every value.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka closed issue #1027: Bug(manifest): convertDecimalValue uses string length instead of declared precision for Avro fixed-size encoding
URL: https://github.com/apache/iceberg-go/issues/1027
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org