kbendick commented on a change in pull request #4307:
URL: https://github.com/apache/iceberg/pull/4307#discussion_r828170365



##########
File path: 
spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java
##########
@@ -273,4 +293,20 @@ private static void listDirRecursively(
       return files.iterator();
     };
   }
+
+  @VisibleForTesting
+  static class PartitionAwareHiddenPathFilter implements PathFilter, 
Serializable {

Review comment:
       Could we add a javadoc to this? Some of the variables are named in a way 
that doesn't immediately make sense to me on first look, but I can't think of 
better alternatives so I think a JavaDoc is possibly the best idea.
   
   The JavaDoc for `HiddenPathFilter` is a good place to look for inspiration. 
So, using that, maybe something like
   ```
   /**
    * A {@link PathFilter} that filters out hidden path, but does not filter 
out paths that would be marked
    * as hidden by {@link HiddenPathFilter} due to a partition field that 
starts with one of the characters that
    * indicate a hidden path.
    */
   ```
   
   That said, you can hold off on adding the JavaDoc until we are closer to 
merging this 🙂 




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

Reply via email to