Re: [PR] feat(table): type uniformity for variant shredding inference [iceberg-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade commented on code in PR #1846:
URL: https://github.com/apache/iceberg-go/pull/1846#discussion_r3808171196


##########
table/internal/variant_shredding.go:
##########
@@ -47,17 +47,6 @@ var decimalPriority = map[variant.Type]int{
 	variant.Decimal4: 0, variant.Decimal8: 1, variant.Decimal16: 2,
 }
 
-// tieBreakPriority breaks count ties in most-common-type selection; higher wins.
-// Types absent here (Null, Array, Object) resolve to -1.
-var tieBreakPriority = map[variant.Type]int{
-	variant.Bool: 0, variant.Int8: 1, variant.Int16: 2, variant.Int32: 3,
-	variant.Int64: 4, variant.Float: 5, variant.Double: 6, variant.Decimal4: 7,
-	variant.Decimal8: 8, variant.Decimal16: 9, variant.Date: 10, variant.Time: 11,
-	variant.TimestampMicros: 12, variant.TimestampMicrosNTZ: 13, variant.Binary: 14,
-	variant.String: 15, variant.TimestampNanos: 16, variant.TimestampNanosNTZ: 17,
-	variant.UUID: 18,
-}
-
 type fieldInfo struct {
 	typeCounts          map[variant.Type]int

Review Comment:
   Non-blocking simplification: now that majority selection is gone, nothing reads these per-type counts; only the map keys remain relevant. Consider storing admission state directly (`admitted`, `hasAdmitted`, `mixed`) and merging each observation through a small helper that widens integer/decimal pairs and rejects cross-family pairs. That would remove this per-node map allocation and most of the `widestInt`/`widestDec`/`otherFamilies` bookkeeping below. If retaining post-hoc calculation, at least model this as `observedTypes map[variant.Type]struct{}` so the representation matches the new semantics.



##########
table/internal/variant_shredding_test.go:
##########
@@ -268,22 +354,14 @@ func TestInferredTypedScalarsShred(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			mk := func() variant.Value {
-				var b variant.Builder
-				require.NoError(t, c.build(&b))
-				v, err := b.Build()
-				require.NoError(t, err)
-
-				return v
-			}
-			v := mk()
+			v := mkBuilt(t, c.build)
 			inner, ok := AnalyzeVariantShredding([]variant.Value{v})
 			require.Truef(t, ok, "%s should infer a shredded type", c.name)
 
 			st := extensions.NewShreddedVariantType(inner)
 			bldr := extensions.NewVariantBuilder(mem, st)
 			defer bldr.Release()
-			bldr.Append(mk())
+			bldr.Append(mkBuilt(t, c.build))

Review Comment:
   Non-blocking nit: `v` was already built above and `Append` does not require a fresh value, so this can simply be `bldr.Append(v)`. That avoids invoking the builder callback twice and makes it clearer that the value used for inference is the one being round-tripped.



##########
table/internal/variant_shredding_test.go:
##########
@@ -115,22 +127,96 @@ func TestAnalyzeIntegerWidening(t *testing.T) {
 		"widening must pick Int64, got %s", f[0].Type)
 }
 
-func TestAnalyzeMixedTypeMajority(t *testing.T) {
-	// "v" is an int in 7 rows, a string in 3 -> majority int wins.
+// TestAnalyzeMixedTypeNotShredded: a field mixing type families (int in 7 rows,
+// string in 3) is not uniform and must not shred - it is this object's only
+// field, so nothing shreds at all.
+func TestAnalyzeMixedTypeNotShredded(t *testing.T) {
 	var sample []variant.Value
 	for range 7 {
 		sample = append(sample, mkVar(t, map[string]any{"v": bigI64}))
 	}
 	for range 3 {
 		sample = append(sample, mkVar(t, map[string]any{"v": "s"}))
 	}
+	_, ok := AnalyzeVariantShredding(sample)
+	assert.False(t, ok, "mixed int+string field must not shred")
+}
+
+// TestAnalyzeCrossFamilyNotShredded: distinct scalar families that do not widen
+// into each other are not uniform and must not shred.
+func TestAnalyzeCrossFamilyNotShredded(t *testing.T) {
+	microsTS := func(b *variant.Builder) error {
+		return b.AppendTimestamp(arrow.Timestamp(1_700_000_000_000_000), true, true)
+	}
+	nanosTS := func(b *variant.Builder) error {
+		return b.AppendTimestamp(arrow.Timestamp(1_700_000_000_000_000), false, true)
+	}
+	dec := func(b *variant.Builder) error { return b.AppendDecimal8(2, decimal.Decimal64(12345)) }
+
+	for _, c := range []struct {
+		name   string
+		sample []variant.Value
+	}{
+		{"float+double", mkVars(t, float32(1.5), float64(2.5))},
+		{"timestamp micros+nanos", []variant.Value{mkBuilt(t, microsTS), mkBuilt(t, nanosTS)}},
+		{"int+decimal", []variant.Value{mkVar(t, bigI64), mkBuilt(t, dec)}},
+	} {
+		t.Run(c.name, func(t *testing.T) {
+			_, ok := AnalyzeVariantShredding(c.sample)
+			assert.Falsef(t, ok, "%s must not shred", c.name)
+		})
+	}
+}
+
+// TestAnalyzeMixedSiblingKeepsUniform: a mixed-type field is dropped from the
+// shredded type while its uniform sibling still shreds.
+func TestAnalyzeMixedSiblingKeepsUniform(t *testing.T) {
+	var sample []variant.Value
+	for range 8 {
+		sample = append(sample, mkVar(t, map[string]any{"good": bigI64, "bad": bigI64}))
+	}
+	for range 2 {
+		sample = append(sample, mkVar(t, map[string]any{"good": bigI64, "bad": "s"}))
+	}

Review Comment:
   Non-blocking simplification: the preceding test already uses a 7/3 split to prove that a majority no longer wins, so this sibling-focused test only needs two rows: one with `bad` as an integer and one with it as a string. You could then compare `dt` directly with `arrow.StructOf` containing only the expected `good` field; that removes the loops, unchecked struct assertion, and field lookups while making the no-extra-fields assertion stronger.



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org