RussellSpitzer commented on code in PR #12066:
URL: https://github.com/apache/iceberg/pull/12066#discussion_r1930808394


##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -51,6 +50,8 @@
 public class RewriteTablePathUtil {
 
   private static final Logger LOG = 
LoggerFactory.getLogger(RewriteTablePathUtil.class);
+  // iceberg client file separator, independent of that of the underlying file 
system

Review Comment:
   I'm not sure this comment is helpful? We don't have an actual defined 
"Iceberg Client File Separator". This is a POSIX path separator that makes 
sense to use because AWS, GCP, AZURE, HDFS etc ... all use it. Comments 
shouldn't beg the question, they should ask and answer it if needed. 
   
   In this case something like
   
   "Use the POSIX separator instead of File.separator because File.separator is 
dependent on the client environment and not the target filesystem. POSIX is 
compatible with S3, GCS, HDFS, etc ..."
   
   So we describe what we are doing and why we are doing that instead of 
something else.



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