tanmayrauth commented on code in PR #1414:
URL: https://github.com/apache/iceberg-go/pull/1414#discussion_r3525965169
##########
table/internal/parquet_files.go:
##########
@@ -730,6 +736,18 @@ func (p parquetFormat) DataFileStatsFromMeta(meta
Metadata, statsCols map[int]St
}
}
+func safeUpdateAgg(agg StatsAgg, stats interface{ HasMinMax() bool }) (err
error) {
Review Comment:
`safeUpdateAgg` recovers from *any* panic during aggregation, but the actual
bug is narrower: `wrappedUUIDStats.Min()`/`Max()` (lines 508/517) panic when
`uuid.FromBytes` gets malformed non-16-byte stats. A blanket recover here means
a genuine bug in any other aggregator (decimal, string, FLBA) also gets
silently swallowed as "invalidate this column" instead of surfacing.
Would you consider fixing it at the source instead — have the UUID wrapper
(and by symmetry the FLBA/decimal-FLBA wrappers, which have the same
fixed-length assumption) detect wrong-length bytes and signal "no valid bounds"
so the column gets invalidated at the same spot as the `!set` / `stats == nil`
cases (lines 653/660)? That keeps invalidation scoped to malformed bounds and
preserves panics as real-bug signals everywhere else.
##########
partitions.go:
##########
@@ -210,6 +210,9 @@ func NewPartitionSpecOpts(opts ...PartitionOption)
(PartitionSpec, error) {
func WithSpecID(id int) PartitionOption {
return func(p *PartitionSpec) error {
+ if id < 0 {
Review Comment:
This `id < 0` check (the "Validate partition spec IDs in WithSpecID" commit)
looks unrelated to the UUID stats fix — can it go in its own PR? Bundling it
makes this one harder to review and revert.
Also, just to confirm: this only fires on the programmatic
`NewPartitionSpecOpts`/`BindToSchema` path — `UnmarshalJSON` at
partitions.go:436 sets `ps.id` directly and bypasses it, so existing metadata
still reads fine. Is there any internal caller that passes a transient negative
id before assigning the real one?
--
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]
