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: [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]
