laskoviymishka commented on code in PR #1355:
URL: https://github.com/apache/iceberg-go/pull/1355#discussion_r3590897983
##########
manifest.go:
##########
@@ -2260,6 +2320,7 @@ func NewDataFileBuilder(
recordCount int64,
fileSize int64,
) (*DataFileBuilder, error) {
+ _ = fieldIDToFixedSize
Review Comment:
I said document-or-drop last round and you documented it — that's fair by
what I asked. Seeing `_ = fieldIDToFixedSize` actually land though, I've come
around to preferring drop.
Both reviewers on this pass read it as a live parameter and assumed it
mattered, and the round-trip test still passes a real
`internal.DecimalRequiredBytes(10)` into it that goes nowhere. We're pre-1.0,
so dropping it is cheaper than carrying a dead arg everyone has to be told to
ignore.
Not blocking — your call. Just flagging that's where I'd land now.
##########
manifest.go:
##########
@@ -2313,7 +2374,6 @@ func NewDataFileBuilder(
fieldIDToPartitionData: fieldIDToPartitionData,
fieldNameToID: fieldNameToID,
fieldIDToLogicalType: fieldIDToLogicalType,
Review Comment:
This is the scale half of what I asked last round and it's still open — a
dataFile built here has no way to populate `fieldIDToDecimalScale`; only
`ManifestReader.ReadEntry` sets it via `setFieldIDToDecimalScaleMap`. So a
builder-built file decoded outside a manifest read hits
`convertAvroValueToIcebergType` with scale=0 and mis-scales the decimal.
What I can't tell is whether that path is actually reachable — is anything
meant to decode a builder-built dataFile without going through
`ManifestReader`? If not, this is moot and I'll drop it; if it is, we'd want a
scale setter on the builder. wdyt?
##########
data_file_codec.go:
##########
@@ -107,7 +107,11 @@ func (d *dataFile) MarshalAvroEntry(spec PartitionSpec,
schema *Schema, version
return nil, err
}
clone := cloneDataFileAvroFields(d)
- clone.PartitionData = avroEncodePartitionData(d.Partition(),
maps.nameToID, maps.idToType)
+ partitionData, err := avroEncodePartitionData(d.Partition(),
maps.nameToID, maps.idToType, maps.idToFixedSize)
Review Comment:
There are two write paths that encode decimal partitions —
`ManifestWriter.addEntry` and this one — and the new tests only cover the
first. The old bug lived in both and this fix touches both, so I'd want a
round-trip here too: `MarshalAvroEntry` → `unmarshalAvroDataFileEntry` on a
decimal partition. Otherwise a regression on this path slips through. Could you
add one?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
