perf(manifest): remove reflection from partition summary updates - #1895
perf(manifest): remove reflection from partition summary updates#1895fallintoplace wants to merge 2 commits into
Conversation
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice cleanup: choosing the converter once per field instead of reflecting on every row is the right call, and it's good that the manifest bytes come out identical.
I'd hold this before merging though. My main concern is behavioral parity: the typed converters don't quite reproduce the old reflect path's accept/reject set, and there's no test pinning that they should.
The clearest case is convertPartitionBool, the one converter that skipped the partitionLiteralValue fallback, so a BoolLiteral input now errors where the old CanConvert path silently accepted it. The mirror image is the shared unwrap helper, which now accepts aboveMaxLiteral/belowMinLiteral sentinels that reflection previously rejected. Neither is likely in normal use, but both are undocumented and untested, and since the tests are all happy-path nothing here would flag a regression.
A few things I'd want settled before merge:
- restore the
partitionLiteralValuefallback inconvertPartitionBool(plus aBoolLiteraltest row) - decide whether the sentinel literals should be accepted or rejected, and pin it with a test
- add rejection-path coverage, one
update(struct{}{})-> error per converter, plus a couple of overflow cases to lock in the current wrapping
The UUID < 16 check, the ns->us timestamp test row, and the numeric-converter duplication are all smaller and non-blocking; I left notes inline. One thing worth a follow-up rather than this PR: the refactor makes newPartitionFieldStat's switch look exhaustive, but TimestampNsType/TimestampTzNsType still fall through to the error branch (pre-existing, not introduced here).
Once the parity items land, happy to take another pass and approve.
| return nil, false | ||
| } | ||
|
|
||
| return literal.Any(), true |
There was a problem hiding this 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?
| return nil | ||
| } | ||
|
|
||
| func newPartitionFieldStats[T LiteralType](convert partitionValueConverter[T]) fieldStats { |
There was a problem hiding this 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.
| } | ||
| } | ||
|
|
||
| func convertPartitionBool(value any) (bool, bool) { |
There was a problem hiding this 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?
| return converted, ok | ||
| } | ||
|
|
||
| func convertPartitionInt32(value any) (int32, bool) { |
There was a problem hiding this 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.
| case [16]byte: | ||
| return uuid.UUID(value), true | ||
| case []byte: | ||
| if len(value) < len(uuid.UUID{}) { |
There was a problem hiding this 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?
| switch value := value.(type) { | ||
| case Decimal: | ||
| return value, true | ||
| case DecimalLiteral: |
There was a problem hiding this 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.
| } | ||
|
|
||
| func TestPartitionFieldStatsAcceptsConvertibleValues(t *testing.T) { | ||
| tests := []struct { |
There was a problem hiding this 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.
| {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))}, |
There was a problem hiding this 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?
bd3ca1c to
d99bd73
Compare
2 participants
What changed
partitionFieldStats.updateused reflection for every non-null partition value. This runs once for every partition field in every manifest entry.This PR:
Benchmark
Existing
BenchmarkManifestWriterPartitionSummaries, 10,000 entries, Apple M1 Pro, Go 1.26.3:Tests
go test . -count=1go test -race . -count=1go test ./... -run ^$ -count=1go vet ./...