laskoviymishka commented on code in PR #1895:
URL: https://github.com/apache/iceberg-go/pull/1895#discussion_r3874746074
##########
manifest.go:
##########
@@ -1222,38 +1233,328 @@ func (p *unknownPartitionFieldStats) update(any) error
{
return nil
}
+func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T])
fieldStats {
Review Comment:
`newPartitionFieldStats` differs from the pre-existing
`newPartitionFieldStat` by a single trailing `s`, with totally different
signatures and roles. In a file this size that's easy to misread, and the
plural `s` collides with the Go convention where plural reads as a
slice/collection. I'd rename the new one to something that signals a generic
factory, e.g. `makePartitionFieldStats[T]`. Non-blocking.
##########
manifest.go:
##########
@@ -1222,38 +1233,328 @@ func (p *unknownPartitionFieldStats) update(any) error
{
return nil
}
+func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T])
fieldStats {
+ return &partitionFieldStats[T]{
+ cmp: getComparator[T](),
+ convert: convert,
+ }
+}
+
+func convertPartitionBool(value any) (bool, bool) {
+ converted, ok := value.(bool)
+
+ return converted, ok
+}
+
+func convertPartitionInt32(value any) (int32, bool) {
Review Comment:
The four numeric converters (`Int32`/`Int64`/`Float32`/`Float64`) are
structurally identical, same 16 source cases and fallback, differing only in
the cast target, so adding a source type means editing four places with nothing
enforcing consistency. Generics can't parameterize a type switch, but a shared
`numericAny(any) (int64, float64, bool)` normalizer plus per-type casts would
collapse most of it without bringing reflection back. Not a blocker, just
flagging while it's fresh.
##########
manifest.go:
##########
@@ -1199,7 +1198,19 @@ type partitionFieldStats[T LiteralType] struct {
min *T
max *T
- cmp Comparator[T]
+ cmp Comparator[T]
+ convert partitionValueConverter[T]
+}
+
+type partitionValueConverter[T LiteralType] func(any) (T, bool)
+
+func partitionLiteralValue(value any) (any, bool) {
+ literal, ok := value.(Literal)
+ if !ok {
+ return nil, false
+ }
+
+ return literal.Any(), true
Review Comment:
Flip side of the bool case below: this now accepts things the old path
rejected. `aboveMaxLiteral`/`belowMinLiteral` are structs that implement
`Literal`, so old `reflect.CanConvert` returned false and `update` errored.
Here `partitionLiteralValue` unwraps them via `.Any()` to their clamped
boundary value, the recursion converts it, and we silently record a boundary
min/max instead of erroring.
In practice partition maps come from real data values so a sentinel
shouldn't show up, but "shouldn't" isn't "can't", and right now nothing
documents or tests the new accept-set. I'd either guard the default arm (bail
if the unwrapped value is itself a `Literal`) or add a comment plus a case
pinning the intended behavior. wdyt?
##########
manifest.go:
##########
@@ -1222,38 +1233,328 @@ func (p *unknownPartitionFieldStats) update(any) error
{
return nil
}
+func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T])
fieldStats {
+ return &partitionFieldStats[T]{
+ cmp: getComparator[T](),
+ convert: convert,
+ }
+}
+
+func convertPartitionBool(value any) (bool, bool) {
+ converted, ok := value.(bool)
+
+ return converted, ok
+}
+
+func convertPartitionInt32(value any) (int32, bool) {
+ switch value := value.(type) {
+ case int:
+ return int32(value), true
+ case int8:
+ return int32(value), true
+ case int16:
+ return int32(value), true
+ case int32:
+ return value, true
+ case int64:
+ return int32(value), true
+ case uint:
+ return int32(value), true
+ case uint8:
+ return int32(value), true
+ case uint16:
+ return int32(value), true
+ case uint32:
+ return int32(value), true
+ case uint64:
+ return int32(value), true
+ case uintptr:
+ return int32(value), true
+ case float32:
+ return int32(value), true
+ case float64:
+ return int32(value), true
+ case Date:
+ return int32(value), true
+ case Time:
+ return int32(value), true
+ case Timestamp:
+ return int32(value), true
+ case TimestampNano:
+ return int32(value), true
+ case time.Duration:
+ return int32(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionInt32(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionInt64(value any) (int64, bool) {
+ switch value := value.(type) {
+ case int:
+ return int64(value), true
+ case int8:
+ return int64(value), true
+ case int16:
+ return int64(value), true
+ case int32:
+ return int64(value), true
+ case int64:
+ return value, true
+ case uint:
+ return int64(value), true
+ case uint8:
+ return int64(value), true
+ case uint16:
+ return int64(value), true
+ case uint32:
+ return int64(value), true
+ case uint64:
+ return int64(value), true
+ case uintptr:
+ return int64(value), true
+ case float32:
+ return int64(value), true
+ case float64:
+ return int64(value), true
+ case Date:
+ return int64(value), true
+ case Time:
+ return int64(value), true
+ case Timestamp:
+ return int64(value), true
+ case TimestampNano:
+ return int64(value), true
+ case time.Duration:
+ return int64(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionInt64(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionFloat32(value any) (float32, bool) {
+ switch value := value.(type) {
+ case int:
+ return float32(value), true
+ case int8:
+ return float32(value), true
+ case int16:
+ return float32(value), true
+ case int32:
+ return float32(value), true
+ case int64:
+ return float32(value), true
+ case uint:
+ return float32(value), true
+ case uint8:
+ return float32(value), true
+ case uint16:
+ return float32(value), true
+ case uint32:
+ return float32(value), true
+ case uint64:
+ return float32(value), true
+ case uintptr:
+ return float32(value), true
+ case float32:
+ return value, true
+ case float64:
+ return float32(value), true
+ case Date:
+ return float32(value), true
+ case Time:
+ return float32(value), true
+ case Timestamp:
+ return float32(value), true
+ case TimestampNano:
+ return float32(value), true
+ case time.Duration:
+ return float32(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionFloat32(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionFloat64(value any) (float64, bool) {
+ switch value := value.(type) {
+ case int:
+ return float64(value), true
+ case int8:
+ return float64(value), true
+ case int16:
+ return float64(value), true
+ case int32:
+ return float64(value), true
+ case int64:
+ return float64(value), true
+ case uint:
+ return float64(value), true
+ case uint8:
+ return float64(value), true
+ case uint16:
+ return float64(value), true
+ case uint32:
+ return float64(value), true
+ case uint64:
+ return float64(value), true
+ case uintptr:
+ return float64(value), true
+ case float32:
+ return float64(value), true
+ case float64:
+ return value, true
+ case Date:
+ return float64(value), true
+ case Time:
+ return float64(value), true
+ case Timestamp:
+ return float64(value), true
+ case TimestampNano:
+ return float64(value), true
+ case time.Duration:
+ return float64(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionFloat64(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionString(value any) (string, bool) {
+ switch value := value.(type) {
+ case string:
+ return value, true
+ case []byte:
+ return string(value), true
+ case []rune:
+ return string(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionString(literal)
+ }
+
+ return "", false
+ }
+}
+
+func convertPartitionBytes(value any) ([]byte, bool) {
+ switch value := value.(type) {
+ case []byte:
+ return value, true
+ case string:
+ return []byte(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionBytes(literal)
+ }
+
+ return nil, false
+ }
+}
+
+func convertPartitionUUID(value any) (uuid.UUID, bool) {
+ switch value := value.(type) {
+ case uuid.UUID:
+ return value, true
+ case [16]byte:
+ return uuid.UUID(value), true
+ case []byte:
+ if len(value) < len(uuid.UUID{}) {
Review Comment:
Behavior-preserving (the old reflect path truncated the same way), but now
that the length check is explicit I'd make it `!= len(uuid.UUID{})`. A 20-byte
slice is almost certainly a caller bug, and `< 16` silently keeps the first 16
rather than rejecting it. wdyt?
##########
manifest.go:
##########
@@ -1222,38 +1233,328 @@ func (p *unknownPartitionFieldStats) update(any) error
{
return nil
}
+func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T])
fieldStats {
+ return &partitionFieldStats[T]{
+ cmp: getComparator[T](),
+ convert: convert,
+ }
+}
+
+func convertPartitionBool(value any) (bool, bool) {
Review Comment:
This is the one converter without the type-switch + `partitionLiteralValue`
fallback that all the others have, and I think it's a real behavior change.
`BoolLiteral` is `type BoolLiteral bool`, so the old reflect path accepted it
via `CanConvert`, but `value.(bool)` here rejects it and `update` returns an
error where it used to succeed.
I'd mirror the other converters: switch on `value` with a default arm that
recurses through `partitionLiteralValue`. A `{typ: PrimitiveTypes.Bool, value:
BoolLiteral(true), want: NewLiteral(true)}` row would pin it, and would've
caught this. wdyt?
##########
manifest.go:
##########
@@ -1222,38 +1233,328 @@ func (p *unknownPartitionFieldStats) update(any) error
{
return nil
}
+func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T])
fieldStats {
+ return &partitionFieldStats[T]{
+ cmp: getComparator[T](),
+ convert: convert,
+ }
+}
+
+func convertPartitionBool(value any) (bool, bool) {
+ converted, ok := value.(bool)
+
+ return converted, ok
+}
+
+func convertPartitionInt32(value any) (int32, bool) {
+ switch value := value.(type) {
+ case int:
+ return int32(value), true
+ case int8:
+ return int32(value), true
+ case int16:
+ return int32(value), true
+ case int32:
+ return value, true
+ case int64:
+ return int32(value), true
+ case uint:
+ return int32(value), true
+ case uint8:
+ return int32(value), true
+ case uint16:
+ return int32(value), true
+ case uint32:
+ return int32(value), true
+ case uint64:
+ return int32(value), true
+ case uintptr:
+ return int32(value), true
+ case float32:
+ return int32(value), true
+ case float64:
+ return int32(value), true
+ case Date:
+ return int32(value), true
+ case Time:
+ return int32(value), true
+ case Timestamp:
+ return int32(value), true
+ case TimestampNano:
+ return int32(value), true
+ case time.Duration:
+ return int32(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionInt32(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionInt64(value any) (int64, bool) {
+ switch value := value.(type) {
+ case int:
+ return int64(value), true
+ case int8:
+ return int64(value), true
+ case int16:
+ return int64(value), true
+ case int32:
+ return int64(value), true
+ case int64:
+ return value, true
+ case uint:
+ return int64(value), true
+ case uint8:
+ return int64(value), true
+ case uint16:
+ return int64(value), true
+ case uint32:
+ return int64(value), true
+ case uint64:
+ return int64(value), true
+ case uintptr:
+ return int64(value), true
+ case float32:
+ return int64(value), true
+ case float64:
+ return int64(value), true
+ case Date:
+ return int64(value), true
+ case Time:
+ return int64(value), true
+ case Timestamp:
+ return int64(value), true
+ case TimestampNano:
+ return int64(value), true
+ case time.Duration:
+ return int64(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionInt64(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionFloat32(value any) (float32, bool) {
+ switch value := value.(type) {
+ case int:
+ return float32(value), true
+ case int8:
+ return float32(value), true
+ case int16:
+ return float32(value), true
+ case int32:
+ return float32(value), true
+ case int64:
+ return float32(value), true
+ case uint:
+ return float32(value), true
+ case uint8:
+ return float32(value), true
+ case uint16:
+ return float32(value), true
+ case uint32:
+ return float32(value), true
+ case uint64:
+ return float32(value), true
+ case uintptr:
+ return float32(value), true
+ case float32:
+ return value, true
+ case float64:
+ return float32(value), true
+ case Date:
+ return float32(value), true
+ case Time:
+ return float32(value), true
+ case Timestamp:
+ return float32(value), true
+ case TimestampNano:
+ return float32(value), true
+ case time.Duration:
+ return float32(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionFloat32(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionFloat64(value any) (float64, bool) {
+ switch value := value.(type) {
+ case int:
+ return float64(value), true
+ case int8:
+ return float64(value), true
+ case int16:
+ return float64(value), true
+ case int32:
+ return float64(value), true
+ case int64:
+ return float64(value), true
+ case uint:
+ return float64(value), true
+ case uint8:
+ return float64(value), true
+ case uint16:
+ return float64(value), true
+ case uint32:
+ return float64(value), true
+ case uint64:
+ return float64(value), true
+ case uintptr:
+ return float64(value), true
+ case float32:
+ return float64(value), true
+ case float64:
+ return value, true
+ case Date:
+ return float64(value), true
+ case Time:
+ return float64(value), true
+ case Timestamp:
+ return float64(value), true
+ case TimestampNano:
+ return float64(value), true
+ case time.Duration:
+ return float64(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionFloat64(literal)
+ }
+
+ return 0, false
+ }
+}
+
+func convertPartitionString(value any) (string, bool) {
+ switch value := value.(type) {
+ case string:
+ return value, true
+ case []byte:
+ return string(value), true
+ case []rune:
+ return string(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionString(literal)
+ }
+
+ return "", false
+ }
+}
+
+func convertPartitionBytes(value any) ([]byte, bool) {
+ switch value := value.(type) {
+ case []byte:
+ return value, true
+ case string:
+ return []byte(value), true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionBytes(literal)
+ }
+
+ return nil, false
+ }
+}
+
+func convertPartitionUUID(value any) (uuid.UUID, bool) {
+ switch value := value.(type) {
+ case uuid.UUID:
+ return value, true
+ case [16]byte:
+ return uuid.UUID(value), true
+ case []byte:
+ if len(value) < len(uuid.UUID{}) {
+ return uuid.UUID{}, false
+ }
+
+ var converted uuid.UUID
+ copy(converted[:], value)
+
+ return converted, true
+ default:
+ if literal, ok := partitionLiteralValue(value); ok {
+ return convertPartitionUUID(literal)
+ }
+
+ return uuid.UUID{}, false
+ }
+}
+
+func convertPartitionDecimal(value any) (Decimal, bool) {
+ switch value := value.(type) {
+ case Decimal:
+ return value, true
+ case DecimalLiteral:
Review Comment:
`DecimalLiteral` would reach `case Decimal:` in one hop through
`partitionLiteralValue` anyway, so this explicit arm is an optimization, but no
other converter special-cases its `Literal` subtype, so a reader will wonder
why decimal is different and int32 isn't. I'd either drop the arm for
consistency or add a one-line comment explaining it's there on purpose.
##########
manifest_test.go:
##########
@@ -507,6 +508,67 @@ func TestConstructPartitionSummariesWithDroppedSource(t
*testing.T) {
}
}
+func TestPartitionFieldStatsAcceptsConvertibleValues(t *testing.T) {
+ tests := []struct {
Review Comment:
Every case here is a happy path, but the rejection path is really the half
of this change that matters: it decides which inputs are no longer accepted.
Both the bool regression and the sentinel change I flagged in `manifest.go`
would surface immediately with a single rejection case per converter.
I'd add at least one `stats.update(struct{}{})` (or a plainly-wrong type)
asserting a non-nil error per converter. While we're here, a couple of overflow
cases (`int64` past `MaxInt32`, `float64` past `MaxFloat32` -> `+Inf`) would
lock in the current wrapping so a later range check can't silently change it.
##########
manifest_test.go:
##########
@@ -507,6 +508,67 @@ func TestConstructPartitionSummariesWithDroppedSource(t
*testing.T) {
}
}
+func TestPartitionFieldStatsAcceptsConvertibleValues(t *testing.T) {
+ tests := []struct {
+ name string
+ typ PrimitiveType
+ value any
+ want Literal
+ }{
+ {name: "boolean", typ: PrimitiveTypes.Bool, value: true, want:
NewLiteral(true)},
+ {name: "int32 from int", typ: PrimitiveTypes.Int32, value:
int(7), want: NewLiteral(int32(7))},
+ {name: "int32 from literal", typ: PrimitiveTypes.Int32, value:
Int32Literal(7), want: NewLiteral(int32(7))},
+ {name: "int64 from int", typ: PrimitiveTypes.Int64, value:
int(8), want: NewLiteral(int64(8))},
+ {name: "float32 from float64", typ: PrimitiveTypes.Float32,
value: float64(1.5), want: NewLiteral(float32(1.5))},
+ {name: "float64 from int", typ: PrimitiveTypes.Float64, value:
int(9), want: NewLiteral(float64(9))},
+ {name: "string from bytes", typ: PrimitiveTypes.String, value:
[]byte("east"), want: NewLiteral("east")},
+ {name: "string from literal", typ: PrimitiveTypes.String,
value: StringLiteral("east"), want: NewLiteral("east")},
+ {name: "date from int32", typ: PrimitiveTypes.Date, value:
int32(10), want: NewLiteral(Date(10))},
+ {name: "time from duration", typ: PrimitiveTypes.Time, value:
time.Duration(11), want: NewLiteral(Time(11))},
+ {name: "timestamp from int64", typ: PrimitiveTypes.Timestamp,
value: int64(12), want: NewLiteral(Timestamp(12))},
+ {name: "timestamp with timezone from nanoseconds", typ:
PrimitiveTypes.TimestampTz, value: TimestampNano(13), want:
NewLiteral(Timestamp(13))},
Review Comment:
Not a regression, the old path bit-cast the same way, but this row quietly
codifies nanoseconds landing on a microsecond stat with no unit conversion.
`13` hides it; a real `TimestampNano` (~1.7e18) would produce a us bound
somewhere around year 55000.
I'd either add a comment that this is a deliberate raw-integer passthrough,
or drop the row so we're not documenting it as intended behavior. wdyt?
--
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]
