You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/29 13:04:04 UTC
[PR] perf(manifest): project scan columns during reads [iceberg-go]
fallintoplace opened a new pull request, #1972:
URL: https://github.com/apache/iceberg-go/pull/1972
## Summary
- **Projects** manifest data-file reads to the fields needed for scan tasks.
- Keeps metric maps only when row pruning or positional-delete matching needs them.
- Drops transient metric maps before local Arrow tasks retain the files.
- Keeps `PlanFiles()` on the existing full-metadata path for callers that inspect task stats.
- Caches projected Avro schemas by writer schema and projection.
## Benchmark
Command: `go test . -run ^ -bench BenchmarkManifestEntryProjection -benchtime=1x -count=1`
Apple M1 Pro, 10,000 entries per case:
| Stats fields | Full bytes | Scan bytes | Full allocs | Scan allocs |
| --- | ---: | ---: | ---: | ---: |
| 10 | 32.6 MB | 15.6 MB | 443,928 | 65,797 |
| 100 | 260.2 MB | 100.5 MB | 2,257,763 | 77,930 |
| 1,000 | 2.93 GB | 1.45 GB | 20,512,582 | 332,865 |
## Validation
- `go test . ./table -count=1`
- Full package test matrix excluding the idle `io/gocloud` package
- `go test ./... -run '^$' -count=1`
- `go test -race ./table -run 'TestOpenManifestWithProjectionDropsStatsAfterFiltering|TestOpenManifestShortCircuitsMetricsEvaluation|TestBuildPositionalDeleteIndex|TestBuildEqualityDeleteIndex' -count=1`
- `go vet ./...`
- `golangci-lint v2.12.2`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(manifest): project scan columns during reads [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1972:
URL: https://github.com/apache/iceberg-go/pull/1972#discussion_r3903460812
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
+ includeColumnStats: projection.IncludeColumnStats,
+ }
+ if cached, ok := manifestEntryProjectionCache.Get(key); ok {
+ return cached, nil
+ }
+
+ root := writerSchema.Root()
+ projectedRoot := *root
+ projectedRoot.Fields = slices.Clone(root.Fields)
+ dataFileFound := false
+ for i := range projectedRoot.Fields {
+ if projectedRoot.Fields[i].Name != "data_file" {
+ continue
+ }
+
+ dataFileFound = true
+ dataFile := projectedRoot.Fields[i].Type
+ if dataFile.Type != "record" {
+ return nil, fmt.Errorf("manifest entry data_file has unexpected Avro type %q", dataFile.Type)
+ }
+
+ fields := make([]avro.SchemaField, 0, len(dataFile.Fields))
+ for _, field := range dataFile.Fields {
+ if manifestScanDataFileField(field.Name, projection.IncludeColumnStats) {
+ fields = append(fields, field)
+ }
+ }
+ dataFile.Fields = fields
+ projectedRoot.Fields[i].Type = dataFile
+
+ break
+ }
+ if !dataFileFound {
+ return nil, errors.New("manifest entry schema does not contain a data_file field")
+ }
+
+ projected, err := projectedRoot.Schema()
+ if err != nil {
+ return nil, fmt.Errorf("build projected manifest entry schema: %w", err)
+ }
+ manifestEntryProjectionCache.Add(key, projected)
+
+ return projected, nil
+}
+
+func manifestScanDataFileField(name string, includeColumnStats bool) bool {
+ switch name {
+ case "content", "file_path", "file_format", "partition", "record_count",
+ "file_size_in_bytes", "key_metadata", "split_offsets", "equality_ids",
+ "sort_order_id", "first_row_id", "referenced_data_file", "content_offset",
+ "content_size_in_bytes":
+ return true
+ case "value_counts", "null_value_counts", "nan_value_counts", "lower_bounds", "upper_bounds":
+ return includeColumnStats
+ default:
Review Comment:
The new version/delete-type test is good coverage for the fields that are kept, so this is softer than last round. The remaining edge: `default: return false` still silently zeroes `block_size_in_bytes` on the projected path while a full `NewManifestReader` carries its real value (a required long in v1). It's deprecated and unused for planning so it's harmless today, but the two readers returning different DataFiles for the same manifest is the kind of thing that bites a future field. A test cross-checking this whitelist against the avro-tagged fields on `dataFile` would make an omission fail loudly instead of vanishing. Non-blocking. wdyt?
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
Review Comment:
This one survived the rework, and it's the most worthwhile of what's left. We build the key with `writerSchema.String()` before the `Get`, so every lookup pays a full JSON serialization of the writer schema even on a hit. A scan opening a thousand manifests that share one schema does ~999 serializations of something that can run to tens of KB, and the same string is what we store as the key, so 256 entries can pin a few MB on a wide schema. For a PR whose whole point is cutting planning-time allocations, this quietly gives some of that back.
Could we key on a hash of the schema string, or the pointer identity of the `*avro.Schema` from `reader.Schema()`, and only touch `String()` on the miss path? wdyt?
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
+ includeColumnStats: projection.IncludeColumnStats,
+ }
+ if cached, ok := manifestEntryProjectionCache.Get(key); ok {
+ return cached, nil
+ }
+
+ root := writerSchema.Root()
+ projectedRoot := *root
Review Comment:
`projectedRoot := *root` shallow-copies the node and we clone `Fields`, but `Props` and `Aliases` still alias the writer schema's, which is cached in `ocf.Reader` and can be shared across goroutines. CI is green so twmb/avro isn't mutating those in `Schema()` today, but it's an unstated assumption. A one-line comment noting we rely on `Schema()` being non-mutating would be enough. wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(manifest): project scan columns during reads [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1972:
URL: https://github.com/apache/iceberg-go/pull/1972
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(manifest): project scan columns during reads [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1972:
URL: https://github.com/apache/iceberg-go/pull/1972#discussion_r3903424307
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
Review Comment:
This one survived the rework, and it's the most worthwhile of what's left. We build the key with `writerSchema.String()` before the `Get`, so every lookup pays a full JSON serialization of the writer schema even on a hit. A scan opening a thousand manifests that share one schema does ~999 serializations of something that can run to tens of KB, and the same string is what we store as the key, so 256 entries can pin a few MB on a wide schema. For a PR whose whole point is cutting planning-time allocations, this quietly gives some of that back.
Could we key on a hash of the schema string, or the pointer identity of the `*avro.Schema` from `reader.Schema()`, and only touch `String()` on the miss path? wdyt?
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
+ includeColumnStats: projection.IncludeColumnStats,
+ }
+ if cached, ok := manifestEntryProjectionCache.Get(key); ok {
+ return cached, nil
+ }
+
+ root := writerSchema.Root()
+ projectedRoot := *root
+ projectedRoot.Fields = slices.Clone(root.Fields)
+ dataFileFound := false
+ for i := range projectedRoot.Fields {
+ if projectedRoot.Fields[i].Name != "data_file" {
+ continue
+ }
+
+ dataFileFound = true
+ dataFile := projectedRoot.Fields[i].Type
+ if dataFile.Type != "record" {
+ return nil, fmt.Errorf("manifest entry data_file has unexpected Avro type %q", dataFile.Type)
+ }
+
+ fields := make([]avro.SchemaField, 0, len(dataFile.Fields))
+ for _, field := range dataFile.Fields {
+ if manifestScanDataFileField(field.Name, projection.IncludeColumnStats) {
+ fields = append(fields, field)
+ }
+ }
+ dataFile.Fields = fields
+ projectedRoot.Fields[i].Type = dataFile
+
+ break
+ }
+ if !dataFileFound {
+ return nil, errors.New("manifest entry schema does not contain a data_file field")
+ }
+
+ projected, err := projectedRoot.Schema()
+ if err != nil {
+ return nil, fmt.Errorf("build projected manifest entry schema: %w", err)
+ }
+ manifestEntryProjectionCache.Add(key, projected)
+
+ return projected, nil
+}
+
+func manifestScanDataFileField(name string, includeColumnStats bool) bool {
+ switch name {
+ case "content", "file_path", "file_format", "partition", "record_count",
+ "file_size_in_bytes", "key_metadata", "split_offsets", "equality_ids",
+ "sort_order_id", "first_row_id", "referenced_data_file", "content_offset",
+ "content_size_in_bytes":
+ return true
+ case "value_counts", "null_value_counts", "nan_value_counts", "lower_bounds", "upper_bounds":
+ return includeColumnStats
+ default:
Review Comment:
The new version/delete-type test is good coverage for the fields that are kept, so this is softer than last round. The remaining edge: `default: return false` still silently zeroes `block_size_in_bytes` on the projected path while a full `NewManifestReader` carries its real value (a required long in v1). It's deprecated and unused for planning so it's harmless today, but the two readers returning different DataFiles for the same manifest is the kind of thing that bites a future field. A test cross-checking this whitelist against the avro-tagged fields on `dataFile` would make an omission fail loudly instead of vanishing. Non-blocking. wdyt?
##########
manifest_projection.go:
##########
@@ -0,0 +1,207 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
+// reading a manifest. The fields needed to build a scan task are always read.
+// Column statistics are read only when IncludeColumnStats is true.
+//
+// A projected read is intended for planning paths that use statistics
+// transiently. Callers that need the complete DataFile metadata should use
+// ManifestFile.Entries or ReadManifest instead.
+type ManifestEntryProjection struct {
+ IncludeColumnStats bool
+}
+
+const manifestEntryProjectionCacheSize = 256
+
+type manifestEntryProjectionCacheKey struct {
+ writerSchema string
+ includeColumnStats bool
+}
+
+var manifestEntryProjectionCache = func() *lru.Cache[manifestEntryProjectionCacheKey, *avro.Schema] {
+ c, err := lru.New[manifestEntryProjectionCacheKey, *avro.Schema](manifestEntryProjectionCacheSize)
+ if err != nil {
+ panic(err)
+ }
+
+ return c
+}()
+
+// EntriesWithProjection streams manifest entries using a reader-schema
+// projection. It is the projected counterpart to ManifestFile.Entries and is
+// useful when a caller needs only the fields required for scan planning.
+func EntriesWithProjection(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return manifestEntries(fs, m, discardDeleted, &projection)
+}
+
+func manifestEntries(
+ fs iceio.IO,
+ m ManifestFile,
+ discardDeleted bool,
+ projection *ManifestEntryProjection,
+) iter.Seq2[ManifestEntry, error] {
+ return func(yield func(ManifestEntry, error) bool) {
+ f, err := fs.Open(m.FilePath())
+ if err != nil {
+ yield(nil, err)
+
+ return
+ }
+ aborted := false
+ defer func() {
+ if cerr := f.Close(); cerr != nil && !aborted {
+ yield(nil, cerr)
+ }
+ }()
+
+ for entry, err := range iterManifest(m, f, discardDeleted, projection) {
+ if !yield(entry, err) {
+ aborted = true
+
+ return
+ }
+ }
+ }
+}
+
+func projectedManifestEntrySchema(
+ writerSchema *avro.Schema,
+ projection ManifestEntryProjection,
+) (*avro.Schema, error) {
+ key := manifestEntryProjectionCacheKey{
+ writerSchema: writerSchema.String(),
+ includeColumnStats: projection.IncludeColumnStats,
+ }
+ if cached, ok := manifestEntryProjectionCache.Get(key); ok {
+ return cached, nil
+ }
+
+ root := writerSchema.Root()
+ projectedRoot := *root
Review Comment:
`projectedRoot := *root` shallow-copies the node and we clone `Fields`, but `Props` and `Aliases` still alias the writer schema's, which is cached in `ocf.Reader` and can be shared across goroutines. CI is green so twmb/avro isn't mutating those in `Schema()` today, but it's an unstated assumption. A one-line comment noting we rely on `Schema()` being non-mutating would be enough. wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(manifest): project scan columns during reads [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1972:
URL: https://github.com/apache/iceberg-go/pull/1972#discussion_r3937951988
##########
manifest_projection.go:
##########
@@ -0,0 +1,220 @@
+// 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 iceberg
+
+import (
+ "errors"
+ "fmt"
+ "iter"
+ "slices"
+
+ iceio "github.com/apache/iceberg-go/io"
+ lru "github.com/hashicorp/golang-lru/v2"
+ "github.com/twmb/avro"
+)
+
+// ManifestEntryProjection selects the optional data-file fields decoded while
Review Comment:
**major** — IncludeColumnStats=true still drops column_sizes, and rewriting a projected entry silently loses it with no error
ManifestEntryProjection.IncludeColumnStats reads as an all-stats toggle, but manifestScanDataFileField (manifest_projection.go:157-170) returns false for column_sizes and distinct_counts unconditionally. The exported surface added by this PR -- EntriesWithProjection (manifest_projection.go:63), NewManifestReaderWithProjection (manifest.go:715), DataFileWithoutColumnStats, ManifestEntryWithoutColumnStats -- can therefore hand a lossy DataFile to a ManifestWriter, which re-serializes it via cloneDataFileAvroFields (manifest.go:1973) and persists the loss without any error or guard. The 'must not be passed to a ManifestWriter' contract is documentation-only and pinned by no test. This is NOT reachable on the shipped internal path (only ToArrowRecords calls planFiles(ctx,true), and compaction/transaction.go/row_delta.go all use unprojected PlanFiles), so it is not blocking -- but it is permanent public API. Suggest unexporting these helpers, or having the writer reject entries flagged
as projected, or renaming the field to reflect that column_sizes is never included.
<details><summary>Evidence</summary>
```text
Probe pr1972_probe_test.go (root pkg), run and then removed: 'unprojected read: valueCounts=map[1:10] colSizes=map[1:512]'; 'projected read (IncludeColumnStats=true): valueCounts=map[1:10] colSizes=map[]'; 'after rewrite of projected entry: colSizes=map[]'; 'CONFIRMED SILENT LOSS: column_sizes map[1:512] -> map[] with no error'. WriteManifest returned nil error on the projected entry.
```
</details>
##########
table/scanner.go:
##########
@@ -1164,6 +1193,34 @@ func (scan *Scan) collectManifestEntriesWithSchemaMinSequenceNum(
return flattenClassifiedManifestEntries(manifestResults), nil
}
+func (scan *Scan) manifestProjectionForManifest(
+ manifest iceberg.ManifestFile,
+ retainDataFileStats bool,
+) (iceberg.ManifestEntryProjection, bool) {
Review Comment:
**minor** — ManifestContentDeletes clause and manifestProjectionRetainsDataStats are tautological at every call site
In manifestProjectionForManifest the `manifest.ManifestContent() == iceberg.ManifestContentDeletes` disjunct can never decide the result. Call site 1 (collectManifestEntriesWithSchemaMinSequenceNum, scanner.go:1637) is only reached under `if len(deleteManifests) > 0` with an all-Deletes list, so retainDataFileStats = manifestProjectionRetainsDataStats(deleteManifests) is already unconditionally true. Call site 2 (planDataManifestTasksWithOptions) only ever receives dataManifests, which splitManifestList guarantees contain no Deletes manifests. So manifestProjectionRetainsDataStats always returns true where it is used. TestManifestProjectionRetainsDataFileStatsForDeleteScans pins a mixed [dataManifest, deleteManifest] list that cannot occur in production, so it does not compensate. Consider collapsing the condition or documenting why the defensive clause is kept.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(manifest): project scan columns during reads [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1972:
URL: https://github.com/apache/iceberg-go/pull/1972#discussion_r3960398886
##########
manifest.go:
##########
@@ -917,6 +936,7 @@ func (c *ManifestReader) ReadEntry() (ManifestEntry, error) {
if c.isFallback {
tmp = tmp.(*fallbackManifestEntry).toEntry()
}
+ tmp.DataFile().(*dataFile).projected = c.projected
switch tmp.Status() {
Review Comment:
**nit** — Unchecked type assertion inconsistent with comma-ok form 28 lines below
tmp.DataFile().(*dataFile).projected = c.projected asserts unconditionally, while manifest.go:967 uses `if df, ok := tmp.DataFile().(*dataFile); ok` on the identical expression. Cannot panic today because tmp is always constructed locally as &manifestEntry{Data: &dataFile{}} (or the fallback wrapping the same), so this is a style/robustness inconsistency rather than a live defect.
##########
manifest.go:
##########
@@ -2571,7 +2605,8 @@ func fitDecimalBytes(bytes []byte, size int) ([]byte, error) {
}
type dataFile struct {
- Content ManifestEntryContent `avro:"content"`
+ Content ManifestEntryContent `avro:"content"`
+ projected bool
Path string `avro:"file_path"`
Review Comment:
**nit** — Non-wire `projected` field placed mid-struct inside the avro-tagged block
`projected bool` sits between Content and Path in the middle of the avro-tagged wire-shape struct. It is safe because avroFieldIndexes (data_file_codec.go:185) discovers indexes by tag lookup rather than position, so cloneDataFileAvroFields neither shifts fields nor launders the flag. Grouping non-wire state at the end of the struct would remove the positional-correspondence trap for future readers.
##########
table/scanner.go:
##########
@@ -1143,7 +1163,14 @@ func (scan *Scan) collectManifestEntriesWithSchemaMinSequenceNum(
if err != nil {
return fmt.Errorf("failed to build partition evaluator for spec %d: %w", mf.PartitionSpecID(), err)
}
- manifestEntries, err := openManifest(fs, mf, partEval, metricsEval)
+ var projection *iceberg.ManifestEntryProjection
+ if projectScanColumns {
+ // Projected collection reads delete manifests before classification.
Review Comment:
**nit** — Hardcoded IncludePruningStats:true assumes a delete-only manifest list
collectManifestEntriesWithSchemaMinSequenceNum accepts an arbitrary manifestList but, when projectScanColumns is true, unconditionally reads pruning stats. Correct today (the only projected caller at scanner.go:1615 passes deleteManifests, and the comment says so), but a future caller passing data manifests would silently lose the projection benefit. Perf pessimization only, never a correctness bug.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org