You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dhruv-pratap (via GitHub)" <gi...@apache.org> on 2025/05/20 21:15:37 UTC
[PR] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
dhruv-pratap opened a new pull request, #13108:
URL: https://github.com/apache/iceberg/pull/13108
We should intercept the `ParquetDecodingException` and log the corrupted parquet file to make this easily discoverable. Knowing the exact parquet file that the executor failed on is essential for identifying bad nodes in a cluster that could be producing corrupt data, and eventually take them out.
```
23/12/10 13:28:04 WARN task-result-getter-0 TaskSetManager: Lost task 55.0 in stage 0.0 (TID 55) (ip-100-74-15-132.ec2.internal executor 28): org.apache.iceberg.bdp.shaded.org.apache.parquet.io.ParquetDecodingException: could not decompress page
```
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100556682
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
Review Comment:
nit: could we please add an extra line to conform the current coding guide?
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100557187
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
+ }
Review Comment:
nit: newline
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100531383
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
`org.apache.iceberg.io.InputFile` doesn't but all its implementations do. For example see `S3InputFile` below:
<img width="1137" alt="image" src="https://github.com/user-attachments/assets/f49dcc5e-63c9-4c46-bfef-6956c2dbfc65" />
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#issuecomment-2895871030
LGTM, though looks like you need to run `spotlessApply`.
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100587778
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
Review Comment:
Added in the latest commit.
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#issuecomment-2900909351
Merged to main.
Thanks for the PR @dhruv-pratap, @ebyhr, @bryanck for the review!
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2098952765
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
Review Comment:
Is `reader` really nullable? This condition looks redundant.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
ParquetInputFile doesn't implement `toString` method, right? Does this log print human-readable path?
```java
public String getFile() {
return file.toString();
}
```
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100594414
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
`location` is a method on the `org.apache.iceberg.io.OutputFile` interface, and not `org.apache.iceberg.io.InputFile`.
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #13108:
URL: https://github.com/apache/iceberg/pull/13108
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100539842
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
Review Comment:
It does seem plausible since `org.apache.parquet.hadoop.ParquetFileReader`, from where the value of `reader` is being inherited in `ParquetReader`, has nullable checks for `reader` as well.
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100588274
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
+ }
Review Comment:
Added in the latest commit.
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100560896
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
Could we just print the `location`?
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100622628
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
Is it?
https://github.com/apache/iceberg/blob/6550486f0c640e15bb7c4d6c16b087397656213e/api/src/main/java/org/apache/iceberg/io/InputFile.java#L53
--
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] CORE: [PARQUET] Log corrupted parquet filenames to trace bad nodes that may have written them. [iceberg]
Posted by "dhruv-pratap (via GitHub)" <gi...@apache.org>.
dhruv-pratap commented on code in PR #13108:
URL: https://github.com/apache/iceberg/pull/13108#discussion_r2100663158
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -120,18 +125,27 @@ public boolean hasNext() {
@Override
public T next() {
- if (valuesRead >= nextRowGroupStart) {
- advance();
- }
-
- if (reuseContainers) {
- this.last = model.read(last);
- } else {
- this.last = model.read(null);
+ try {
+ if (valuesRead >= nextRowGroupStart) {
+ advance();
+ }
+
+ if (reuseContainers) {
+ this.last = model.read(last);
+ } else {
+ this.last = model.read(null);
+ }
+ valuesRead += 1;
+
+ return last;
+ } catch (ParquetDecodingException e) {
+ if (reader != null) {
+ // Knowing the exact parquet file is essential for tracing bad nodes
+ // that produced the corrupt file, parquet lib doesn't do this today.
+ LOG.error("Error decoding Parquet file {}", reader.getFile(), e);
Review Comment:
My bad, it indeed does. Unfortunately though `org.apache.parquet.hadoop.ParquetFileReader` encapsulates the `InputFile`, and only exposes `getFile()` to get the parquet file location. There is also `getPath()` but that has been marked as deprecated.
--
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