nssalian commented on code in PR #1478:
URL: https://github.com/apache/iceberg-go/pull/1478#discussion_r3631936751
##########
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:
I fixed this. Variant string/binary leaf bounds now truncate to a fixed 16,
matching what Java's
[ParquetVariantUtil(https://github.com/apache/iceberg/blob/main/parquet/src/main/java/org/apache/iceberg/parquet/ParquetVariantUtil.java#L347-L375)
actually does: fixed length, independent of the column's configured metrics
length, applied even under `full`. Threading the parent's `truncate(N)` through
would've diverged from Java, so I didn't. Added string + binary tests for it.
One thing I found while matching it: there's an upper-bound bug in Java's
variant binary path that can emit a bound below the true max (wrong pruning for
binary variant fields > 16 bytes). Go does the correct thing here - the one
spot it intentionally differs from Java today. I'll file that against Java and
link it.
--
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]
