Re: [PR] Core: Cache splitOffsets list in BaseFile to avoid repeated allocations [iceberg]

["wombatu-kun (via GitHub)" <gi...@apache.org>]

wombatu-kun commented on code in PR #16265:
URL: https://github.com/apache/iceberg/pull/16265#discussion_r3353641181


##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -88,6 +88,12 @@ public PartitionData copy() {
   // cached schema
   private transient Schema avroSchema = null;
 
+  // Cached unmodifiable List<Long> view of splitOffsets, allocated lazily on
+  // the first splitOffsets() call and reused thereafter. Invalidated (set to
+  // null) whenever the underlying long[] is reassigned. Marked transient so
+  // it is not part of the wire/serialized state. See #15622.
+  private transient List<Long> splitOffsetsList = null;

Review Comment:
   This cache only saves an allocation when splitOffsets() is called more than once on the same BaseFile, but the manifest-encode path calls it once per file (V2Metadata.java:459); the only real repeat is ContentFileParser's adjacent double call (ContentFileParser.java:113-114). #15622 suggested routing in-core callers through the package-private splitOffsetArray() (raw long[]) instead, which avoids the allocation entirely with no extra field or invalidation sites. Consider deduping the ContentFileParser call, or note why the cached List is preferred over the long[] route for the manifest path.



-- 
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