zeroshade commented on code in PR #1478:
URL: https://github.com/apache/iceberg-go/pull/1478#discussion_r3625777430
##########
table/internal/parquet_files.go:
##########
@@ -989,15 +964,199 @@ func (p parquetFormat) DataFileStatsFromMeta(meta
Metadata, statsCols map[int]St
return ok
})
+ vlo, vup := p.collectVariantBounds(pqmeta, arrowSchema, colMapping,
statsCols)
+
return &DataFileStatistics{
- RecordCount: pqmeta.GetNumRows(),
- ColSizes: colSizes,
- ValueCounts: valueCounts,
- NullValueCounts: nullValueCounts,
- NanValueCounts: nanValueCounts,
- SplitOffsets: splitOffsets,
- ColAggs: colAggs,
+ RecordCount: pqmeta.GetNumRows(),
+ ColSizes: colSizes,
+ ValueCounts: valueCounts,
+ NullValueCounts: nullValueCounts,
+ NanValueCounts: nanValueCounts,
+ SplitOffsets: splitOffsets,
+ ColAggs: colAggs,
+ VariantLowerBounds: vlo,
+ VariantUpperBounds: vup,
+ }
+}
+
+// wrapStatsForType adapts Parquet stats so a StatsAgg reads Iceberg-typed
min/max.
+func wrapStatsForType(stats metadata.TypedStatistics, typ
iceberg.PrimitiveType) metadata.TypedStatistics {
+ switch t := typ.(type) {
+ case iceberg.BinaryType:
+ return
&wrappedBinaryStats{stats.(*metadata.ByteArrayStatistics)}
+ case iceberg.UUIDType:
+ return
&wrappedUUIDStats{stats.(*metadata.FixedLenByteArrayStatistics)}
+ case iceberg.StringType:
+ return
&wrappedStringStats{stats.(*metadata.ByteArrayStatistics)}
+ case iceberg.FixedType:
+ return &wrappedFLBAStats{
+ FixedLenByteArrayStatistics:
stats.(*metadata.FixedLenByteArrayStatistics),
+ expectedLen: t.Len(),
+ }
+ case iceberg.DecimalType:
+ switch s := stats.(type) {
+ case *metadata.FixedLenByteArrayStatistics:
+ return &wrappedDecStats{
+ FixedLenByteArrayStatistics: s,
+ expectedLen:
internal.DecimalRequiredBytes(t.Precision()),
+ scale: t.Scale(),
+ }
+ case *metadata.ByteArrayStatistics:
+ return &wrappedDecByteArrayStats{s, t.Scale()}
+ }
+ }
+
+ return stats
+}
+
+// residualAllVariantNull reports whether a residual value column holds only
variant-null.
+func residualAllVariantNull(st metadata.TypedStatistics) bool {
+ ba, ok := st.(*metadata.ByteArrayStatistics)
+ if !ok {
+ return false
+ }
+
+ return isVariantNull(ba.Min()) && isVariantNull(ba.Max())
+}
+
+// isVariantNull reports whether a serialized variant value is primitive null
(byte 0x00).
+func isVariantNull(b []byte) bool {
+ return len(b) == 1 && b[0] == 0x00
+}
+
+// collectVariantBounds builds spec "Bounds for Variant" objects per shredded
variant column.
+func (p parquetFormat) collectVariantBounds(meta *metadata.FileMetaData,
arrowSchema *arrow.Schema, colMapping map[string]int, statsCols
map[int]StatisticsCollector) (lower, upper map[int][]byte) {
+ if arrowSchema == nil {
+ return nil, nil
+ }
+
+ type leafAgg struct {
+ leaf variantLeaf
+ parentID int
+ agg StatsAgg
+ invalid bool
+ }
+
+ byTyped := make(map[string]*leafAgg)
+ residualParent := make(map[string]struct{}) // residual value columns
to inspect
+ residualBad := make(map[string]bool) // residual value column
has/may have non-null
+
+ for _, f := range arrowSchema.Fields() {
+ vt, ok := f.Type.(*extensions.VariantType)
+ if !ok || vt.TypedValue().Type == nil {
+ continue
+ }
+ parentID, ok := colMapping[f.Name]
+ if !ok {
+ continue
+ }
+ // Honor the parent variant column's metrics mode: none/counts
skip min/max bounds.
+ if sc, ok := statsCols[parentID]; !ok || sc.Mode.Typ ==
MetricModeNone || sc.Mode.Typ == MetricModeCounts {
+ continue
+ }
+ for _, lf := range enumerateVariantLeaves([]string{f.Name},
vt.TypedValue()) {
+ byTyped[lf.typedPath] = &leafAgg{leaf: lf, parentID:
parentID}
+ for _, vp := range lf.valuePaths {
+ residualParent[vp] = struct{}{}
+ }
+ }
+ }
+ if len(byTyped) == 0 {
+ return nil, nil
+ }
+
+ for rg := range meta.NumRowGroups() {
+ rowGroup := meta.RowGroup(rg)
+ for pos := range rowGroup.NumColumns() {
+ cc, err := rowGroup.ColumnChunk(pos)
+ if err != nil {
+ continue
+ }
+ path := cc.PathInSchema().String()
+
+ if _, isResidual := residualParent[path]; isResidual {
+ // Non-null residual entries invalidate the
bound unless they are all variant-null.
+ set, serr := cc.StatsSet()
+ st, terr := cc.Statistics()
+ switch {
+ case serr != nil || !set || terr != nil || st
== nil || !st.HasNullCount():
+ residualBad[path] = true
+ case cc.NumValues()-st.NullCount() > 0:
+ if !st.HasMinMax() ||
!residualAllVariantNull(st) {
+ residualBad[path] = true
+ }
+ }
+
+ continue
+ }
+
+ e, isTyped := byTyped[path]
+ if !isTyped || e.invalid {
+ continue
+ }
+ set, serr := cc.StatsSet()
+ st, terr := cc.Statistics()
+ switch {
+ case serr != nil || !set || terr != nil || st == nil:
+ e.invalid = true
+ case st.HasMinMax():
+ if e.agg == nil {
+ agg, aerr :=
p.createStatsAgg(e.leaf.icebergType, st.Type().String(), 0)
Review Comment:
Non-blocking: this passes `truncLen=0`, so a variant column configured with
`truncate(N)` still writes full-length string/binary child bounds, whereas the
primitive path honors the mode via `createStatsAgg(..., statsCol.Mode.Len)`
(parquet_files.go:933). Full bounds are valid and read-equivalent, so this
isn't a blocker — but it does mean `truncate(N)` is silently ignored for
shredded variant leaves. Worth either threading the parent's truncate length
through here, or documenting it next to the value/null-count follow-up so the
metrics-mode semantics stay consistent. Was skipping truncation for variant
child bounds intentional?
--
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]
