kevinjqliu commented on code in PR #17284:
URL: https://github.com/apache/iceberg/pull/17284#discussion_r3865355222
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -1352,7 +1356,12 @@ public ReaderFunction withSchema(Schema expectedSchema) {
}
private ReadBuilder(InputFile file) {
- this.file = file;
+ long fileLength = file.getLength();
+ this.file = canEagerFetch(fileLength) ? EagerInputFile.of(file,
fileLength) : file;
Review Comment:
Codex helped me flag this issue. In summary, S3/GCS/ADLS already accepts and
use the (potentially staled) length. Hadoop was using `getStat()` to get the
length directly. So this is a new regression for Hadoop.
For small Hadoop-backed Parquet reads, `file.getLength()` may come from the
manifest-list entry and can be stale. Previously, Hadoop reads obtained the
physical size through:
```java
HadoopInputFile.fromStatus(hfile.getStat(), hfile.getConf());
```
See
[`ParquetIO.file()`](https://github.com/apache/iceberg/blob/2b8d9be9044d8d557a0aa54fba45166cbc1270be/parquet/src/main/java/org/apache/iceberg/parquet/ParquetIO.java#L53-L66).
This PR wraps files up to 1 MiB using the reported length:
```java
long fileLength = file.getLength();
this.file = canEagerFetch(fileLength) ? EagerInputFile.of(file, fileLength)
: file;
```
The eager reader then reads exactly that many bytes:
```java
byte[] bytes = new byte[(int) length];
IOUtil.readFully(src, bytes, 0, bytes.length);
```
If the recorded length is smaller than the physical file, the buffer
excludes the real Parquet footer and trailing `PAR1` magic:
```text
java.lang.RuntimeException: ... is not a Parquet file.
Expected magic number at tail, but found [0, 80, 65, 82]
at
org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:622)
at org.apache.iceberg.parquet.ReadConf.newReader(ReadConf.java:194)
at org.apache.iceberg.parquet.ParquetReader.init(ParquetReader.java:74)
```
If the recorded length is larger, `readFully` attempts to read past EOF.
[#16910](https://github.com/apache/iceberg/pull/16910) corrects new complete
rewrites, but it does not repair existing affected manifest-list entries and
still preserves source lengths for manifests carried over by incremental
rewrites.
This PR therefore removes Hadoop's existing protection against stale lengths
by wrapping `HadoopInputFile` before `ParquetIO` can call `getStat()`. Other
input implementations already trusted their reported lengths and are not newly
broken.
Please preserve the physical Hadoop `FileStatus` length before eager
wrapping and add a stale-length regression test.
--
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]
