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: [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]
