[GitHub] [iceberg] islamismailov commented on a diff in pull request #6353: Make sure S3 stream opened by ReadConf ctor is closed

[GitBox]

islamismailov commented on code in PR #6353:
URL: https://github.com/apache/iceberg/pull/6353#discussion_r1054796838


##########
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();

Review Comment:
   You call `this.conf = readConf.copy();`. `copy()` calls a private 
constructor that does not copy the reader itself: 
   
   ```
   private ReadConf(ReadConf<T> toCopy) {
       this.reader = null;
   ```
   
   so the reader is created to just fill all the other fields and then 
immediately discarded without getting closed.
   
   then, inside the iterator you call `conf.reader()` which now creates a 
reader because it's null:
   
   ```
     ParquetFileReader reader() {
       if (reader != null) {
         reader.setRequestedSchema(projection);
         return reader;
       }
   ```
   
   i suppose the other way to "fix" this would be to copy the reader as part of 
the `copy` but the fact that was done looked very deliberate, perhaps you have 
a context why.



-- 
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