zeroshade commented on code in PR #1644:
URL: https://github.com/apache/iceberg-go/pull/1644#discussion_r3732106789
##########
table/orphan_cleanup.go:
##########
@@ -805,7 +806,7 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedURL := &url.URL{
Scheme: normalizedScheme,
Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
+ Path: pathpkg.Clean(parsedURL.Path),
Review Comment:
Cleaning the decoded URL path conflates distinct object keys: escaped
separators such as `%2F`, duplicate slashes, and literal dot segments can
collapse to the same comparison key. Unlike the Windows issue, this can retain
a real orphan rather than delete a live file, but it still makes cleanup
incorrect.
Suggested fix: preserve opaque object-key spelling for remote URLs while
normalizing only the scheme/authority equivalences that are explicitly
configured.
##########
table/orphan_cleanup.go:
##########
@@ -814,19 +815,66 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
// normalizeNonURLPath provides basic path normalization for non-URL paths.
//
// Handles file system paths by:
-// 1. Applying filepath.Clean() to resolve "..", ".", and redundant separators
-// 2. Converting Windows-style backslashes to forward slashes for consistency
+// 1. Converting Windows-style backslashes to forward slashes for consistency
+// 2. Applying slash-based path cleaning to resolve "..", ".", and redundant
separators
//
// This ensures that paths like "dir/./file", "dir//file", and "dir\file" (on
Windows)
// all normalize to "dir/file" for consistent comparison.
-//
-// Uses filepath.ToSlash() equivalent logic to match Go's standard library
approach.
func normalizeNonURLPath(path string) string {
- normalized := filepath.Clean(path)
- // We use this because to handle Windows paths
- // on all platforms.filepath.ToSlash() only convert the current OS
separator, and
- // we need cross-platform support.
- return strings.ReplaceAll(normalized, "\\", "/")
+ normalized := strings.ReplaceAll(path, "\\", "/")
Review Comment:
Windows drive and UNC paths remain case-sensitive after this normalization.
A referenced `C:\Warehouse\Data.parquet` and listed `c:\warehouse\data.parquet`
miss the comparison, the listed path is classified as orphan, and orphan
cleanup then **deletes a live file**.
Suggested fix: case-fold comparison keys only for paths positively
identified as Windows-local (drive or UNC). Never case-fold object-store keys:
S3 keys are legitimately case-sensitive. Preserve the existing symmetry by
applying the same revised normalizer to both the referenced and listed sets.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
