Core: Avoid HEAD on metadata file in synthetic metadata-table tasks - #16833
Core: Avoid HEAD on metadata file in synthetic metadata-table tasks#16833eholmer-pltr wants to merge 1 commit into
Conversation
HistoryTable, SnapshotsTable, MetadataLogEntriesTable, RefsTable, ManifestsTable, AllManifestsTable, and PartitionsTable build a StaticDataTask whose rows come from in-memory TableMetadata (or from pre-walked manifest data). They wrap a DataFile around metadataFileLocation purely so FileScanTask.file() has a path to return; the bytes are never read. The old StaticDataTask.of(InputFile, ...) entry point built that DataFile via DataFiles.Builder.withInputFile, which calls InputFile.getLength() to populate fileSizeInBytes -- triggering a HEAD/GetObject against the metadata.json on object-store FileIO. That size is never consulted by the DataTask read path in any engine (Spark's RowDataReader.open and Flink's DataTaskReader.open both gate file-size-aware logic on !task.isDataTask()). This replaces StaticDataTask.of(InputFile, ...) with StaticDataTask.of(String location, ...), which builds the DataFile via withPath + withFileSizeInBytes(0L), and migrates the seven metadata tables plus TestDataTaskParser to it. The old InputFile overload and its private constructor (the only remaining getLength call site) are removed, so no construction path performs I/O against the synthetic location. task.length() for these tasks now returns 0 instead of the metadata.json file size. Precedent: AllManifestsTable.ManifestListReadTask.length() already returns a hard-coded 8192 with the comment "return a generic length to avoid looking up the actual length".
|
@stevenzwu @flyrain, this came out of the metadata.json-dependence discussion; would appreciate your eyes when you have a moment. It removes the HEAD/getLength on metadata.json that the synthetic metadata-table tasks were doing. |
|
Thanks for taking a look, @flyrain! Would you be up for a formal review when you have a moment? It's a small, self-contained change. Happy to address anything. If someone else is better placed to review, a pointer would be much appreciated too. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Still relevant and still looking for a reviewer |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
|
@szehon-ho @RussellSpitzer, would either of you mind reopening and reviewing this? GitHub won’t let me reopen it after the stale workflow closed it. I rechecked it against current |
What
HistoryTable,SnapshotsTable,MetadataLogEntriesTable,RefsTable,ManifestsTable,AllManifestsTable, andPartitionsTablebuild aStaticDataTaskwhose rows are materialized from in-memoryTableMetadata(or from pre-walked manifest data). TheDataFilethese tasks wrap exists only soFileScanTask.file()can return a path for identification — the file bytes are never read.The previous
StaticDataTask.of(InputFile, ...)entry point built thatDataFileviaDataFiles.Builder.withInputFile, which callsInputFile.getLength()to populatefileSizeInBytes. On object-storeFileIOimplementations that is aHEAD/GetObjectround-trip against the table'smetadata.json— one extra request on every metadata-table scan, for a value that is never consulted on theDataTaskread path.Change
Replace
StaticDataTask.of(InputFile, ...)withStaticDataTask.of(String location, ...), which builds the syntheticDataFileviawithPath(location)+withFileSizeInBytes(0L). The seven metadata tables andTestDataTaskParserare migrated to it, and the old overload and its private constructor — the only remaininggetLength()call site — are removed, so no construction path performs I/O against the synthetic location.FileScanTask.length()for these tasks now returns0instead of themetadata.jsonsize.Why this is safe
fileSizeInBytesis never read for aDataTask. Engines branch onScanTask.isDataTask()and obtain rows viaDataTask.rows(); any file-size-aware logic (buffer sizing, split planning) lives on the!isDataTask()path — e.g.RowDataReader.openin iceberg-spark andDataTaskReader.openin iceberg-flink.There is existing precedent in the same area:
AllManifestsTable.ManifestListReadTask.length()already returns a hard-coded8192with the comment "return a generic length to avoid looking up the actual length".StaticDataTaskis package-private, so this is not a public API change (no revapi impact).Related discussion
This is a small, concrete step in a direction the community has been discussing — reducing engines' dependence on the root
metadata.jsonand the code paths that read it directly:metadata.jsonoptional: https://lists.apache.org/thread/l1onvv5p5cq2vtql9g8w5fbxc1hhd65lSee also Yufei Gu's doc tracking clients/engines with hard dependencies on the metadata file in storage, shared in that thread: https://docs.google.com/document/d/17PBhJ0IBxHxMKvCW6CstGOp7cZnboMDdpO6BCPO2kmA/edit
The metadata tables never need any bytes from
metadata.json— only a path for identification — yet today they issue aHEADagainst it solely to populate an unused size field. This removes that one unnecessary read; it doesn't attempt the broader changes proposed in those threads.Testing
TestStaticDataTaskpins the contract: a bogus, never-created location is tolerated with no I/O,length()andDataFile.fileSizeInBytes()are0, format isMETADATA, and the supplied path is preserved for identification.TestDataTaskParseris migrated to the string overload; the serialized round-trip is byte-identical (the previousFiles.localInput(...)already reported size0and stripped the URI scheme).