laskoviymishka commented on code in PR #2375:
URL: https://github.com/apache/iceberg-go/pull/2375#discussion_r4490287496
##########
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.
+ return b.UnsafeAppendEncoded(binary.LittleEndian.AppendUint32(
+ []byte{byte(variant.PrimitiveInt32) << 2},
uint32(lit.Any().(int32))))
Review Comment:
Could these two encodings move into small helpers in `internal` that the
tests reuse? The `type << 2` header is now hand-built in four places: these
two,
[`appendVariantInt32`](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/variant_shredded_write_test.go#L805-L808)
and
[`buildObj`](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/variant_shredded_test.go#L607).
`UnsafeAppendEncoded` doesn't validate, so a typo in any copy would write an
invalid variant without an error.
##########
table/variant_shredded_write_test.go:
##########
@@ -779,8 +781,8 @@ func TestShreddedVariantWriteLargeDecimal(t *testing.T) {
}
}
-// columnPhysicalType returns the Parquet physical type of a top-level leaf
column.
-func columnPhysicalType(t *testing.T, path, name string) parquet.Type {
+// columnPhysicalType returns the Parquet physical and logical type of a leaf
column.
+func columnPhysicalType(t *testing.T, path, name string) (parquet.Type,
pqschema.LogicalType) {
Review Comment:
Could this stay physical-only, with a separate `columnLogicalType` for the
new test, or get a name like `columnTypes`? Two of the three callers now drop
the logical type with `pt, _ :=`
([L869](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/variant_shredded_write_test.go#L869),
[L943](https://github.com/apache/iceberg-go/blob/e41be56098153e9447e80585f6404625f3a2ff36/table/variant_shredded_write_test.go#L943)),
and the name no longer says what it returns.
--
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]
