laskoviymishka commented on code in PR #2375:
URL: https://github.com/apache/iceberg-go/pull/2375#discussion_r4507850141
##########
table/internal/variant_bounds.go:
##########
@@ -267,9 +268,12 @@ func appendLiteralToVariant(b *variant.Builder, it
iceberg.PrimitiveType, lit ic
case iceberg.BooleanType:
return b.AppendBool(lit.Any().(bool))
case iceberg.Int32Type:
- return b.AppendInt(int64(lit.Any().(int32)))
+ // AppendInt picks the smallest width; bounds must keep the
column's width.
Review Comment:
Could the width here come from the Parquet annotation instead of the Iceberg
leaf type? `arrowLeafToIceberg` folds Int8/Int16/Int32 into Int32
([L101-L107](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/internal/variant_bounds.go#L101-L107)),
so an INT(8) `typed_value` imported through `AddDataFilesByPath` gets int32
bounds, where `main` and Java
([`ParquetVariantUtil`](https://github.com/apache/iceberg/blob/e5f8ab19c82fcf24be430cc6c0cecf9a23f5afe4/parquet/src/main/java/org/apache/iceberg/parquet/ParquetVariantUtil.java#L272-L285))
give int8. A scratch import of 0..4 decodes as `int32(0)`/`int32(4)` here and
`int8(0)`/`int8(4)` with the base `variant_bounds.go`. Fine to leave for the
follow-up issue from last round.
##########
table/variant_shredded_write_test.go:
##########
@@ -792,12 +794,17 @@ func columnPhysicalType(t *testing.T, path, name string)
parquet.Type {
sc := pf.MetaData().Schema
for i := range sc.NumColumns() {
if col := sc.Column(i); col.Name() == name {
- return col.PhysicalType()
+ return col.PhysicalType(), col.LogicalType()
}
}
t.Fatalf("column %q not found", name)
- return parquet.Types.Undefined
+ return parquet.Types.Undefined, nil
+}
+
+// appendVariantInt32 appends v as a variant int32, bypassing AppendInt's
smallest-width encoding.
+func appendVariantInt32(b *variant.Builder, v int32) error {
Review Comment:
Could the raw-byte bound checks decode the bound and check its type instead,
like `boundField` in the new test
([L1593-L1600](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/variant_shredded_write_test.go#L1593-L1600))?
`TestShreddedVariantWriteNullFieldKeepsBound`, `ScalarBounds` "int-small" and
`TestShreddedVariantAddFilesBounds` build expected bytes with the same `type <<
2` formula as `appendLiteralToVariant`, so a mistake shared by both would pass
all three. With the base encoder swapped back they do fail, so a typo in one
copy is caught.
--
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]
