nssalian commented on code in PR #2375:
URL: https://github.com/apache/iceberg-go/pull/2375#discussion_r4474193978
##########
table/variant_shredded_write_test.go:
##########
@@ -1487,6 +1487,73 @@ func TestShreddedVariantWriteNullFieldKeepsBound(t
*testing.T) {
assert.Equal(t, buildObj(1006), files[0].UpperBoundValues()[2])
}
+func TestShreddedVariantWriteSmallSampleKeepsWideBound(t *testing.T) {
+ mem := memory.DefaultAllocator
+ iceSchema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "payload", Type:
iceberg.VariantType{}},
+ )
+ arrSchema, err := SchemaToArrowSchema(iceSchema, nil, true, false)
+ require.NoError(t, err)
+
+ vals := []int64{1, 2, 3, 4, 40000, 40001, 40002, 40003}
+ idb := array.NewInt64Builder(mem)
+ defer idb.Release()
+ vb := extensions.NewVariantBuilder(mem,
extensions.NewDefaultVariantType())
+ defer vb.Release()
+ for i, fv := range vals {
+ idb.Append(int64(i))
+ var b variant.Builder
+ require.NoError(t, b.Append(map[string]any{"f": fv}))
+ v, err := b.Build()
+ require.NoError(t, err)
+ vb.Append(v)
+ }
+ idArr := idb.NewArray()
+ defer idArr.Release()
+ pArr := vb.NewArray()
+ defer pArr.Release()
+ rec := array.NewRecordBatch(arrSchema, []arrow.Array{idArr, pArr},
int64(len(vals)))
+ defer rec.Release()
+
+ itr := func(yield func(arrow.RecordBatch, error) bool) { yield(rec,
nil) }
+ loc := strings.ReplaceAll(t.TempDir(), "\\", "/")
+ meta, err := NewMetadata(iceSchema, iceberg.UnpartitionedSpec,
UnsortedSortOrder, loc, iceberg.Properties{
+ PropertyFormatVersion: "3",
+ ParquetShredVariantsKey: "true",
+ ParquetVariantBufferSizeKey: "4",
+ })
+ require.NoError(t, err)
+ mb, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ u := uuid.New()
+ args := recordWritingArgs{sc: arrSchema, itr: itr, fs: iceio.LocalFS{},
writeUUID: &u, counter: infiniteCounter()}
+ factory, err := newWriterFactory(loc, args, mb, iceSchema,
512*1024*1024)
+ require.NoError(t, err)
+
+ var files []iceberg.DataFile
+ for df, err := range unpartitionedWrite(context.Background(), factory,
args.itr) {
+ require.NoError(t, err)
+ files = append(files, df)
+ }
+ require.Len(t, files, 1)
+
+ // Inference sees only 1-4 (int8); 40000+ must still land in
typed_value, or f loses its bounds.
+ buildObj := func(fv int64) []byte {
+ var b variant.Builder
+ start := b.Offset()
+ entries := []variant.FieldEntry{b.NextField(start, "$['f']")}
+ require.NoError(t, b.AppendInt(fv))
+ require.NoError(t, b.FinishObject(start, entries))
+ v, err := b.Build()
+ require.NoError(t, err)
+
+ return append(append([]byte{}, v.Metadata().Bytes()...),
v.Bytes()...)
+ }
+ assert.Equal(t, buildObj(1), files[0].LowerBoundValues()[2])
Review Comment:
Agreed, fixed in
https://github.com/apache/iceberg-go/pull/2375/commits/e41be56098153e9447e80585f6404625f3a2ff36.
Int32 and Int64 bounds are now written at a fixed width instead of through
`AppendInt`, so both bounds share one type, like `ParquetMetrics`. arrow-go has
no public fixed-width int append, so this uses `UnsafeAppendEncoded`, and the
test decodes both bounds to check they are int32.
One gap: `AddDataFilesByPath` on another writer's file with an
INT(8)/INT(16) `typed_value` still gets int32 bounds, where Java gives
INT8/INT16. I can fix that here or in a follow-up, whichever you prefer.
##########
table/variant_shredded_write_test.go:
##########
@@ -1487,6 +1487,73 @@ func TestShreddedVariantWriteNullFieldKeepsBound(t
*testing.T) {
assert.Equal(t, buildObj(1006), files[0].UpperBoundValues()[2])
}
+func TestShreddedVariantWriteSmallSampleKeepsWideBound(t *testing.T) {
+ mem := memory.DefaultAllocator
+ iceSchema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "payload", Type:
iceberg.VariantType{}},
+ )
+ arrSchema, err := SchemaToArrowSchema(iceSchema, nil, true, false)
+ require.NoError(t, err)
+
+ vals := []int64{1, 2, 3, 4, 40000, 40001, 40002, 40003}
Review Comment:
Added. The test now checks the `INT(32, signed)` annotation and covers
negatives, `MinInt32`/`MaxInt32`, and one past each limit (bounds dropped, as
expected).
--
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]
