dongjoon-hyun commented on code in PR #59117:
URL: https://github.com/apache/spark/pull/59117#discussion_r4236798475
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedColumnReader.java:
##########
@@ -381,11 +385,8 @@ private void initDataReader(
this.isCurrentPageDictionaryEncoded = false;
}
- try {
- dataColumn.initFromPage(pageValueCount, in);
- } catch (IOException e) {
- throw new IOException("could not read page in col " + descriptor, e);
- }
+ // readPageV1 and readPageV2 add the page and the column to any error
thrown here.
Review Comment:
Minor: `readPageV1` and `readPageV2` only add the page and column to an
`IOException` or a `ParquetDecodingException`, not to any error. A values
reader's `initFromPage` can still throw other runtime exceptions here. For
example, `VectorizedDeltaBinaryPackedReader.initFromPage` checks the header
with `JavaUtils.checkArgument(miniSize % 8 == 0, ...)`, so a corrupt
DELTA_BINARY_PACKED header with a block size of 128 and 3 mini blocks fails
with an `IllegalArgumentException` that reaches `FAILED_READ_FILE` without the
page or column. The same goes for its `checkArgument(valueCount >= 1, ...)`.
Could the comment name the two types? Turning those checks into
`ParquetDecodingException`s, like the block size checks next to them, could go
with SPARK-59854.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/VectorizedColumnReader.java:
##########
@@ -455,6 +456,9 @@ private int readPageV1(DataPageV1 page) throws IOException {
return pageValueCount;
} catch (IOException e) {
throw new IOException("could not read page " + page + " in col " +
descriptor, e);
Review Comment:
Minor: with the inner catch gone, one `IOException` from `initDataReader`
still gets two "could not read page" layers: the missing dictionary error at
L371-373, whose message still starts with "could not read page in col X". So a
page with RLE_DICTIONARY encoding in a column chunk without a dictionary page
reports "could not read page Page [...] in col X" on top of "could not read
page in col X as the dictionary was missing for encoding RLE_DICTIONARY".
Shortening the inner message to "the dictionary was missing for encoding ..."
would add the page and column once here too, as 3398c18 does for errors from
`initFromPage`.
--
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]
