zeroshade commented on code in PR #1991:
URL: https://github.com/apache/iceberg-go/pull/1991#discussion_r3937636314
##########
table/arrow_utils.go:
##########
@@ -1737,47 +1739,157 @@ func (a *arrowStatsCollector) Primitive(dt
iceberg.PrimitiveType) []tblutils.Sta
}
}
- isNested := strings.Contains(colName, ".")
if isNested && (metMode.Typ == tblutils.MetricModeTruncate ||
metMode.Typ == tblutils.MetricModeFull) {
metMode = tblutils.MetricsMode{Typ: tblutils.MetricModeCounts}
}
- return []tblutils.StatisticsCollector{{
+ return tblutils.StatisticsCollector{
FieldID: a.fieldID,
IcebergTyp: dt,
ColName: colName,
Mode: metMode,
- }}
+ }, true
}
-func (a *arrowStatsCollector) Variant(_ iceberg.VariantType)
[]tblutils.StatisticsCollector {
+func (a *arrowStatsCollector) Primitive(dt iceberg.PrimitiveType)
[]tblutils.StatisticsCollector {
colName, ok := a.schema.FindColumnName(a.fieldID)
if !ok {
Review Comment:
**minor** — Primitive resolves the column name twice per field
Primitive calls a.schema.FindColumnName(a.fieldID) at line 1755 solely to
compute strings.Contains(colName, "."), then primitiveCollector repeats the
identical lookup at line 1726 and repeats the !ok check. In a PR whose stated
goal is removing redundant per-field work this is a doubled map lookup on the
exact path being optimized. It is currently harmless only because this method
is test-only (see the major finding).
##########
table/arrow_utils.go:
##########
@@ -1706,24 +1708,24 @@ func (a *arrowStatsCollector) Map(m iceberg.MapType,
keyResult, valResult func()
Review Comment:
**major** — Visitor traversal is now dead production code; TestStatsTypes no
longer guards computeStatsPlan
computeStatsPlan no longer calls iceberg.PreOrderVisit; it uses the new
collectStatsPlanField traversal. That leaves
arrowStatsCollector.Schema/Struct/Field/List/Map (lines 1675-1707) and the
slice-wrapping Primitive (1754) / Variant (1781) reachable only from
arrow_utils_internal_test.go:431. The package now maintains two independent
traversals of the same schema, and the single test that asserts exact per-field
MetricsMode values validates the orphaned one. Fix: delete the visitor methods
and rewrite TestStatsTypes against computeStatsPlan, or keep PreOrderVisit as
the only traversal.
<details><summary>Evidence</summary>
```text
Mutating collectStatsPlanField to `if true { return }` (production plan
always empty): `go test ./table/ -run 'TestStatsTypes$' -v` => `--- PASS:
TestStatsTypes (0.00s)` / `ok github.com/apache/iceberg-go/table 0.565s`, while
the full package reports 26 `--- FAIL` tests. grep confirms the only
arrowStatsCollector+PreOrderVisit construction outside arrow_utils.go is
arrow_utils_internal_test.go:432.
```
</details>
##########
table/arrow_utils_bench_test.go:
##########
@@ -0,0 +1,72 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+)
+
+func BenchmarkComputeStatsPlan(b *testing.B) {
+ for _, fieldCount := range []int{100, 1000, 10000} {
+ for _, benchmarkCase := range []struct {
+ name string
+ defaultMode string
+ overrideFields int
+ }{
Review Comment:
**nit** — Benchmark field `overrideFields` is a stride, not a field count
The field is named as a count but is used as the loop increment (`i +=
benchmarkCase.overrideFields`), so the value 100 yields fieldCount/100
overrides. The arithmetic happens to produce the intended 1% for all three
sizes, but the name inverts the meaning and will mislead the next person who
tunes it. Rename to overrideStride, or express it as a count and derive the
stride.
##########
table/arrow_utils.go:
##########
@@ -1737,47 +1739,157 @@ func (a *arrowStatsCollector) Primitive(dt
iceberg.PrimitiveType) []tblutils.Sta
}
}
- isNested := strings.Contains(colName, ".")
if isNested && (metMode.Typ == tblutils.MetricModeTruncate ||
metMode.Typ == tblutils.MetricModeFull) {
metMode = tblutils.MetricsMode{Typ: tblutils.MetricModeCounts}
}
- return []tblutils.StatisticsCollector{{
+ return tblutils.StatisticsCollector{
FieldID: a.fieldID,
IcebergTyp: dt,
ColName: colName,
Mode: metMode,
- }}
+ }, true
}
-func (a *arrowStatsCollector) Variant(_ iceberg.VariantType)
[]tblutils.StatisticsCollector {
+func (a *arrowStatsCollector) Primitive(dt iceberg.PrimitiveType)
[]tblutils.StatisticsCollector {
colName, ok := a.schema.FindColumnName(a.fieldID)
if !ok {
return []tblutils.StatisticsCollector{}
}
- return []tblutils.StatisticsCollector{{
+ collector, ok := a.primitiveCollector(dt, strings.Contains(colName,
"."))
+ if !ok {
+ return []tblutils.StatisticsCollector{}
+ }
+
+ return []tblutils.StatisticsCollector{collector}
+}
+
+func (a *arrowStatsCollector) variantCollector()
(tblutils.StatisticsCollector, bool) {
+ colName, ok := a.schema.FindColumnName(a.fieldID)
+ if !ok {
+ return tblutils.StatisticsCollector{}, false
+ }
+
+ return tblutils.StatisticsCollector{
FieldID: a.fieldID,
ColName: colName,
Mode: a.resolveColumnMetricsMode(colName),
- }}
+ }, true
}
-func computeStatsPlan(sc *iceberg.Schema, props iceberg.Properties)
(map[int]tblutils.StatisticsCollector, error) {
- result := make(map[int]tblutils.StatisticsCollector)
+func (a *arrowStatsCollector) Variant(_ iceberg.VariantType)
[]tblutils.StatisticsCollector {
+ collector, ok := a.variantCollector()
+ if !ok {
+ return []tblutils.StatisticsCollector{}
+ }
+
+ return []tblutils.StatisticsCollector{collector}
+}
+
+func statsPlanFieldCount(field iceberg.NestedField) int {
+ switch typ := field.Type.(type) {
+ case *iceberg.StructType:
+ count := 0
+ for _, nestedField := range typ.FieldList {
+ count += statsPlanFieldCount(nestedField)
+ }
+
+ return count
+ case *iceberg.ListType:
+ return statsPlanFieldCount(typ.ElementField())
+ case *iceberg.MapType:
+ return statsPlanFieldCount(typ.KeyField()) +
statsPlanFieldCount(typ.ValueField())
+ default:
+ return 1
+ }
+}
+
+func collectStatsPlanField(visitor *arrowStatsCollector, result
map[int]tblutils.StatisticsCollector, field iceberg.NestedField, isNested bool)
{
+ switch typ := field.Type.(type) {
+ case *iceberg.StructType:
+ for _, nestedField := range typ.FieldList {
+ collectStatsPlanField(visitor, result, nestedField,
true)
+ }
+ case *iceberg.ListType:
+ collectStatsPlanField(visitor, result, typ.ElementField(), true)
+ case *iceberg.MapType:
+ collectStatsPlanField(visitor, result, typ.KeyField(), true)
+ collectStatsPlanField(visitor, result, typ.ValueField(), true)
+ case iceberg.VariantType:
+ visitor.fieldID = field.ID
+ if collector, ok := visitor.variantCollector(); ok {
+ result[collector.FieldID] = collector
+ }
+ default:
+ visitor.fieldID = field.ID
+ collector, ok :=
visitor.primitiveCollector(field.Type.(iceberg.PrimitiveType), isNested)
+ if ok {
+ result[collector.FieldID] = collector
+ }
+ }
+}
+
+func computeStatsPlan(sc *iceberg.Schema, props iceberg.Properties) (result
map[int]tblutils.StatisticsCollector, err error) {
+ defer func() {
+ if r := recover(); r != nil {
+ result = nil
+ switch e := r.(type) {
+ case string:
+ err = fmt.Errorf("%w: %s",
iceberg.ErrInvalidSchema, e)
+ case error:
+ err = fmt.Errorf("error encountered during
schema visitor: %w", e)
+ }
+ }
+ }()
+
+ if sc == nil {
+ return nil, fmt.Errorf("%w: cannot visit nil schema",
iceberg.ErrInvalidArgument)
+ }
+
+ defaultMode, defaultModeErr := tblutils.MatchMetricsMode(
+ props.Get(DefaultWriteMetricsModeKey,
DefaultWriteMetricsModeDefault))
+ var columnModes map[string]tblutils.MetricsMode
+ var columnModeErrors map[string]error
+ for key, rawMode := range props {
+ colName, ok := strings.CutPrefix(key,
MetricsModeColumnConfPrefix+".")
+ if !ok {
+ continue
+ }
+
+ mode, err := tblutils.MatchMetricsMode(rawMode)
+ if err != nil {
+ if columnModeErrors == nil {
+ columnModeErrors = make(map[string]error,
len(props))
+ }
Review Comment:
**nit** — Override maps pre-sized to len(props) rather than the override
count
columnModes and columnModeErrors are allocated with capacity len(props),
i.e. the count of ALL table properties, not the count of
write.metadata.metrics.column.* keys. Real tables carry many unrelated
properties, so this over-allocates the very maps the PR is pre-sizing to save
allocations.
--
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]
