nastra commented on code in PR #6353:
URL: https://github.com/apache/iceberg/pull/6353#discussion_r1042500106
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -68,7 +72,7 @@ public ParquetReader(
private ReadConf<T> init() {
if (conf == null) {
- ReadConf<T> readConf =
+ try (ReadConf<T> readConf =
Review Comment:
```suggestion
try (ReadConf<T> readConf =
new ReadConf<>(
input,
options,
expectedSchema,
filter,
readerFunc,
null,
nameMapping,
reuseContainers,
caseSensitive,
null)) {
this.conf = readConf.copy();
}
```
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetReader.java:
##########
@@ -79,9 +83,11 @@ private ReadConf<T> init() {
nameMapping,
reuseContainers,
caseSensitive,
- null);
- this.conf = readConf.copy();
- return readConf;
+ null)) {
+ this.conf = readConf.copy();
+ } catch (IOException e) {
+ LOG.warn("Failed to close ReadConf", e);
Review Comment:
The contract of `AutoCloseable` is slightly different. It says
> While this interface method is declared to throw Exception, implementers
are strongly encouraged to declare concrete implementations of the close method
to throw more specific exceptions, or to throw no exception at all if the close
operation cannot fail.
So we don't have to throw any exception in `ReadConf.close()`. We can add
the try-catch + logging to `ReadConf.close()` directly (because I'm not sure
there's value to propagate that exception in this particular case). See the
suggestions I've made. I think this is cleaner, because then you don't have to
use a `try-with-resources` + `catch` to mention that the underlying
`ParquetFileReader` failed to be closed.
##########
parquet/src/main/java/org/apache/iceberg/parquet/ReadConf.java:
##########
@@ -257,4 +257,11 @@ private List<Map<ColumnPath, ColumnChunkMetaData>>
getColumnChunkMetadataForRowG
}
return listBuilder.build();
}
+
+ @Override
Review Comment:
```suggestion
@Override
public void close() {
if (reader != null) {
try {
reader.close();
} catch (IOException e) {
LOG.warn("Failed to close ReadConf", e);
}
}
}
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
