You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/26 22:06:50 UTC
[PR] perf(manifest): remove reflection from partition summary updates [iceberg-go]
fallintoplace opened a new pull request, #1895:
URL: https://github.com/apache/iceberg-go/pull/1895
## What changed
`partitionFieldStats.update` used reflection for every non-null partition value. This runs once for every partition field in every manifest entry.
This PR:
- stores a typed converter when each field stat is created
- handles scalar, logical, literal, UUID, binary, and decimal values without reflection
- keeps the defensive copy for binary bounds
- keeps the existing error for unsupported values
## Benchmark
Existing `BenchmarkManifestWriterPartitionSummaries`, 10,000 entries, Apple M1 Pro, Go 1.26.3:
- allocations: 140,485 -> 120,485, about 14% fewer
- allocated bytes: about 22.4 MB -> 22.3 MB
- time: about 38.7 ms -> 38.0 ms
## Tests
- `go test . -count=1`
- `go test -race . -count=1`
- `go test ./... -run '^$' -count=1`\n- tests for all non-integration packages\n- `go vet ./...`
--
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
Re: [PR] perf(manifest): remove reflection from partition summary updates [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] perf(manifest): remove reflection from partition summary updates [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1895:
URL: https://github.com/apache/iceberg-go/pull/1895
--
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