laskoviymishka commented on code in PR #1899:
URL: https://github.com/apache/iceberg-go/pull/1899#discussion_r3874740332
##########
table/evaluators.go:
##########
@@ -794,9 +804,16 @@ func (m *inclusiveMetricsEval) TestRowGroup(rgmeta
*metadata.RowGroupMetaData, c
fieldID := int(stats.Descr().SchemaNode().FieldID())
m.valueCounts[fieldID] = stats.NumValues()
if stats.HasNullCount() {
+ if m.nullCounts == nil {
+ m.nullCounts = make(map[int]int64,
len(colIndices))
+ }
m.nullCounts[fieldID] = stats.NullCount()
}
if stats.HasMinMax() {
+ if m.lowerBounds == nil {
Review Comment:
While we're in here: `upperBounds` only ever gets allocated inside this
`lowerBounds == nil` branch, so `lowerBounds` is effectively the sentinel for
the pair. A one-line comment noting they're always allocated together would
keep a future change from setting one without the other.
##########
table/evaluators.go:
##########
@@ -766,11 +766,21 @@ func (m *inclusiveMetricsEval) TestRowGroup(rgmeta
*metadata.RowGroupMetaData, c
return rowsCannotMatch, nil
}
- m.valueCounts = make(map[int]int64)
- m.nullCounts = make(map[int]int64)
+ if m.valueCounts == nil {
+ m.valueCounts = make(map[int]int64, len(colIndices))
+ } else {
+ clear(m.valueCounts)
+ }
+ if m.nullCounts != nil {
Review Comment:
I think this quietly changes pruning behavior, which is the one thing I'd
want fixed before merge on an allocation-only PR.
Before this change `nullCounts` got a fresh `make` every call, so
`mayContainNull`'s `if m.nullCounts == nil { return true }` branch was
effectively dead and a field with no null stats always came back `false`
(prune-eligible). Now `nullCounts` starts nil and is only allocated once some
row group has null stats; after that, a later no-null-stats row group runs
`clear()` and leaves a non-nil empty map instead of nil. So two structurally
identical row groups can return opposite results from `mayContainNull`
depending on whether an earlier call populated it, and `VisitNotStartsWith`
keys its pruning decision on exactly that.
Cleanest fix for a perf PR is to keep the pre-change invariant by handling
`nullCounts` the same as `valueCounts`, and dropping the lazy make in the loop:
```go
if m.nullCounts == nil {
m.nullCounts = make(map[int]int64, len(colIndices))
} else {
clear(m.nullCounts)
}
```
`valueCounts` is already fine, and `lowerBounds`/`upperBounds` are fine too
since every consumer reads them by key and nil-checks the returned `[]byte`;
nil and empty behave identically there. It's only `mayContainNull` that
hard-branches on `== nil`. wdyt?
##########
table/evaluators_row_group_bench_test.go:
##########
@@ -0,0 +1,134 @@
+// 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 (
+ "errors"
+ "fmt"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/parquet/metadata"
+ "github.com/apache/iceberg-go"
+)
+
+func BenchmarkParquetRowGroupMetricsMaps(b *testing.B) {
+ for _, tc := range []struct {
+ rowGroups int
+ columns int
+ }{
+ {rowGroups: 128, columns: 8},
+ {rowGroups: 1024, columns: 32},
+ } {
+ b.Run(fmt.Sprintf("row_groups=%d/columns=%d", tc.rowGroups,
tc.columns), func(b *testing.B) {
+ withStats := buildRowGroupMetricsMetadata(b,
tc.rowGroups, tc.columns, true)
+ withoutStats := buildRowGroupMetricsMetadata(b,
tc.rowGroups, tc.columns, false)
+ colIndices := make([]int, tc.columns)
+ for i := range colIndices {
+ colIndices[i] = i
+ }
+
+ for _, stats := range []struct {
+ name string
+ meta *metadata.FileMetaData
+ }{
+ {name: "no_stats", meta: withoutStats},
+ {name: "all_stats", meta: withStats},
+ } {
+ b.Run(stats.name+"/before", func(b *testing.B) {
+ benchmarkRowGroupMetricsMaps(b,
stats.meta, colIndices, benchmarkTestRowGroupBefore)
+ })
+ b.Run(stats.name+"/after", func(b *testing.B) {
+ benchmarkRowGroupMetricsMaps(b,
stats.meta, colIndices, (*inclusiveMetricsEval).TestRowGroup)
+ })
+ }
+ })
+ }
+}
+
+func benchmarkRowGroupMetricsMaps(
+ b *testing.B,
+ meta *metadata.FileMetaData,
+ colIndices []int,
+ testRowGroup func(*inclusiveMetricsEval, *metadata.RowGroupMetaData,
[]int) (bool, error),
+) {
+ b.Helper()
+ m := &inclusiveMetricsEval{expr: iceberg.AlwaysTrue{}}
+ b.ReportAllocs()
+ b.ResetTimer()
+
+ for b.Loop() {
+ for rowGroup := range meta.NumRowGroups() {
+ keep, err := testRowGroup(m, meta.RowGroup(rowGroup),
colIndices)
+ if err != nil {
+ b.Fatal(err)
+ }
+ if !keep {
+ b.Fatal("unexpected row-group rejection")
+ }
+ }
+ }
+}
+
+func benchmarkTestRowGroupBefore(m *inclusiveMetricsEval, rgmeta
*metadata.RowGroupMetaData, colIndices []int) (bool, error) {
Review Comment:
`benchmarkTestRowGroupBefore` is a hand-copied snapshot of the old
`TestRowGroup`, and nothing keeps it in sync. If `TestRowGroup` changes later,
the "before" baseline silently goes stale. A one-line comment marking it an
intentional frozen copy of the pre-#1899 implementation (that shouldn't track
future changes) would save the confusion.
##########
table/evaluators_row_group_test.go:
##########
@@ -0,0 +1,107 @@
+// 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/arrow-go/v18/parquet"
+ "github.com/apache/arrow-go/v18/parquet/metadata"
+ parquetschema "github.com/apache/arrow-go/v18/parquet/schema"
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func buildRowGroupMetricsMetadata(t testing.TB, rowGroups, columns int,
withStats bool) *metadata.FileMetaData {
+ t.Helper()
+ fields := make(parquetschema.FieldList, columns)
+ for i := range fields {
+ fields[i] =
parquetschema.NewByteArrayNode(fmt.Sprintf("field_%d", i),
parquet.Repetitions.Required, int32(i+1))
+ }
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required, fields, -1)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ builder :=
metadata.NewFileMetadataBuilder(parquetschema.NewSchema(root),
parquet.NewWriterProperties(), nil)
+ for rowGroup := range rowGroups {
+ rg := builder.AppendRowGroup()
+ rg.SetNumRows(1)
+ for column := range columns {
+ chunk := rg.NextColumnChunk()
+ if withStats {
+ var stats metadata.EncodedStatistics
+ stats.SetMin([]byte("a"))
+ stats.SetMax([]byte("z"))
+ stats.SetNullCount(0)
+ chunk.SetStats(stats)
+ }
+ if err := chunk.Finish(metadata.ChunkMetaInfo{
+ NumValues: 1,
+ DataPageOffset: int64(100 + rowGroup*columns
+ column),
+ IndexPageOffset: -1,
+ CompressedSize: 8,
+ UncompressedSize: 8,
+ }, false, false, metadata.EncodingStats{}); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if err := rg.Finish(int64(columns*8), int16(rowGroup)); err !=
nil {
+ t.Fatal(err)
+ }
+ }
+
+ meta, err := builder.Finish()
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ return meta
+}
+
+func TestInclusiveMetricsEvalRowGroupMetricsLifecycle(t *testing.T) {
+ withStats := buildRowGroupMetricsMetadata(t, 1, 2, true)
+ withoutStats := buildRowGroupMetricsMetadata(t, 1, 2, false)
+ eval := &inclusiveMetricsEval{expr: iceberg.AlwaysTrue{}}
Review Comment:
This lifecycle test only ever runs `AlwaysTrue`, so `VisitTrue` returns
before any of these maps get consulted: the reuse paths get built up, but the
pruning logic that actually reads them never runs.
I'd add a case that evaluates `NOT STARTS WITH` (or `IS NULL`/`NOT NULL`)
across the same no-stats, with-stats, no-stats sequence and asserts the result
is identical on the first and third calls. That's what locks the contract, and
it would catch the `nullCounts` leak from the other comment. wdyt?
##########
table/evaluators_row_group_test.go:
##########
@@ -0,0 +1,107 @@
+// 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/arrow-go/v18/parquet"
+ "github.com/apache/arrow-go/v18/parquet/metadata"
+ parquetschema "github.com/apache/arrow-go/v18/parquet/schema"
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func buildRowGroupMetricsMetadata(t testing.TB, rowGroups, columns int,
withStats bool) *metadata.FileMetaData {
+ t.Helper()
+ fields := make(parquetschema.FieldList, columns)
+ for i := range fields {
+ fields[i] =
parquetschema.NewByteArrayNode(fmt.Sprintf("field_%d", i),
parquet.Repetitions.Required, int32(i+1))
+ }
+ root, err := parquetschema.NewGroupNode("schema",
parquet.Repetitions.Required, fields, -1)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ builder :=
metadata.NewFileMetadataBuilder(parquetschema.NewSchema(root),
parquet.NewWriterProperties(), nil)
+ for rowGroup := range rowGroups {
+ rg := builder.AppendRowGroup()
+ rg.SetNumRows(1)
+ for column := range columns {
+ chunk := rg.NextColumnChunk()
+ if withStats {
+ var stats metadata.EncodedStatistics
+ stats.SetMin([]byte("a"))
+ stats.SetMax([]byte("z"))
+ stats.SetNullCount(0)
+ chunk.SetStats(stats)
+ }
+ if err := chunk.Finish(metadata.ChunkMetaInfo{
+ NumValues: 1,
+ DataPageOffset: int64(100 + rowGroup*columns
+ column),
+ IndexPageOffset: -1,
+ CompressedSize: 8,
+ UncompressedSize: 8,
+ }, false, false, metadata.EncodingStats{}); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if err := rg.Finish(int64(columns*8), int16(rowGroup)); err !=
nil {
+ t.Fatal(err)
+ }
+ }
+
+ meta, err := builder.Finish()
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ return meta
+}
+
+func TestInclusiveMetricsEvalRowGroupMetricsLifecycle(t *testing.T) {
+ withStats := buildRowGroupMetricsMetadata(t, 1, 2, true)
+ withoutStats := buildRowGroupMetricsMetadata(t, 1, 2, false)
+ eval := &inclusiveMetricsEval{expr: iceberg.AlwaysTrue{}}
+
+ keep, err := eval.TestRowGroup(withoutStats.RowGroup(0), []int{0, 1})
+ require.NoError(t, err)
+ assert.True(t, keep)
+ assert.NotNil(t, eval.valueCounts)
+ assert.Nil(t, eval.nullCounts)
+ assert.Nil(t, eval.lowerBounds)
+ assert.Nil(t, eval.upperBounds)
+
+ keep, err = eval.TestRowGroup(withStats.RowGroup(0), []int{0, 1})
+ require.NoError(t, err)
+ assert.True(t, keep)
+ assert.Len(t, eval.valueCounts, 2)
+ assert.Len(t, eval.nullCounts, 2)
+ assert.Len(t, eval.lowerBounds, 2)
+ assert.Len(t, eval.upperBounds, 2)
+
+ keep, err = eval.TestRowGroup(withoutStats.RowGroup(0), []int{0, 1})
+ require.NoError(t, err)
+ assert.True(t, keep)
+ assert.Empty(t, eval.valueCounts)
+ assert.Empty(t, eval.nullCounts)
Review Comment:
`assert.Empty` passes for both a nil map and a non-nil empty one, so this
third block doesn't distinguish "map reused and cleared" from "map reset to
nil": it can't catch the behavior change above or prove the reuse invariant the
PR is going for.
Exact assertions depend on which fix we take for `nullCounts`, but the shape
I'd want is `assert.NotNil` + `assert.Empty` on the reused maps so it actually
pins "same map, emptied." Heads up that flips the first-call
`assert.Nil(eval.nullCounts)` too if we go the always-non-nil route.
--
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]
