laskoviymishka commented on code in PR #2375:
URL: https://github.com/apache/iceberg-go/pull/2375#discussion_r4431210989
##########
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:
Could this also check the `typed_value` annotation and a few edge values?
The physical type is INT32 on `main` too, so only the INT logical-type bit
width (32 vs 8) tells the two apart
([pqarrow](https://github.com/apache/arrow-go/blob/72e9a5695337bab9ac01ec1e4d58177bc4949dc3/parquet/pqarrow/schema.go#L639-L648)).
`columnPhysicalType`
([here](https://github.com/apache/iceberg-go/blob/e87a05c88be46703f390a3c617f8f364cf164d97/table/variant_shredded_write_test.go#L782-L800))
is a starting point. None of the shredding tests use negative values, or
`math.MinInt32`/`math.MaxInt32` and one past each.
##########
table/internal/variant_shredding.go:
##########
@@ -347,11 +347,8 @@ func primitiveArrowType(t variant.Type, info *fieldInfo)
arrow.DataType {
switch t {
case variant.Bool:
return arrow.FixedWidthTypes.Boolean
- case variant.Int8:
- return arrow.PrimitiveTypes.Int8
- case variant.Int16:
- return arrow.PrimitiveTypes.Int16
- case variant.Int32:
+ case variant.Int8, variant.Int16, variant.Int32:
+ // Never narrower than INT32: same physical type, and later
values within int32 range stay typed.
Review Comment:
Could the comment also say the sampled width is dropped on purpose? Java's
`VariantReaderBuilder` picks the type from the annotation's bit width
([link](https://github.com/apache/iceberg/blob/be2528185a1d37e6590bce193c2760202d90a0cb/parquet/src/main/java/org/apache/iceberg/parquet/VariantReaderBuilder.java#L232-L261)),
so Java readers now see int32 where `main` gave them int8/int16. Go
re-minimizes on read, so a Go round-trip wouldn't show it.
```suggestion
// Never narrower than INT32: same physical type, and later
values within int32 range stay typed.
// The sampled width is intentionally not kept: readers that
rebuild from the annotation (e.g. Java) see int32.
```
##########
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:
Could we write both bounds at the `typed_value` column's width, the way Java
does? This pins an int8 lower (`1`) and an int32 upper (`40003`), while the
spec says bounds "must have the same Variant type"
([spec.md](https://github.com/apache/iceberg/blob/e5f8ab19c82fcf24be430cc6c0cecf9a23f5afe4/format/spec.md?plain=1#L939-L946)),
and int8 and int32 are separate physical types. Java's `ParquetMetrics` takes
one physical type from the column for both
([link](https://github.com/apache/iceberg/blob/e5f8ab19c82fcf24be430cc6c0cecf9a23f5afe4/parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetrics.java#L604-L652)).
The smallest-fit `AppendInt` in `appendLiteralToVariant` predates this PR
([link](https://github.com/apache/iceberg-go/blob/e87a05c88be46703f390a3c617f8f364cf164d97/table/internal/variant_bounds.go#L265-L272)),
and as I read it neither Go nor Java pruning compares type ids. But this
change brings bounds back in the files where the pair usually comes out mixed,
and this is the first test to lock that in.
--
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]
