[GitHub] [iceberg] openinx commented on a change in pull request #2881: Api#2880: Close the underlying iterator in ClosingIterator in hasNext() call

[GitBox <gi...@apache.org>]

openinx commented on a change in pull request #2881:
URL: https://github.com/apache/iceberg/pull/2881#discussion_r678048412



##########
File path: api/src/main/java/org/apache/iceberg/io/ClosingIterator.java
##########
@@ -25,33 +25,30 @@
 
 public class ClosingIterator<T> implements Iterator<T> {
   private final CloseableIterator<T> iterator;
-  private boolean shouldClose = false;
 
   public ClosingIterator(CloseableIterator<T> iterator) {
     this.iterator = iterator;
-
   }
 
   @Override
   public boolean hasNext() {
     boolean hasNext = iterator.hasNext();
-    this.shouldClose = !hasNext;
+    if (!hasNext) {
+      close();

Review comment:
       I think you mean in the read pattern (for the original `ClosingIterator`)
   
   ```java
      while (hasNext()) {
         T val = next();
       }
   ```
   
   When the hasNext() return false, then we won't call the `next()` to close the internal `CloseableIterator`.   Yes, I think you are correct,  but I think we will need to guarantee that we will only close the `CloseableIterator` once even if we called the `ClosingIterator#hasNext` many times, otherwise once we've called the first `hasNext`, all the following `hasNext` check will be thrown an exception, that's not the expected behavior.




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