laskoviymishka commented on code in PR #1644:
URL: https://github.com/apache/iceberg-go/pull/1644#discussion_r3740301545
##########
table/orphan_cleanup.go:
##########
@@ -802,31 +804,99 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedScheme := applySchemeEquivalence(parsedURL.Scheme,
equalSchemes)
normalizedAuthority := applyAuthorityEquivalence(parsedURL.Host,
equalAuthorities)
- normalizedURL := &url.URL{
- Scheme: normalizedScheme,
- Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
- }
+
+ // Object-store paths are opaque keys. Keep their spelling exactly as
+ // supplied: escaped separators, duplicate slashes, and dot segments can
+ // all be meaningful parts of a key. Only the explicitly configured
scheme
+ // and authority equivalences are normalized here.
+ normalizedURL := *parsedURL
+ normalizedURL.Scheme = normalizedScheme
+ normalizedURL.Host = normalizedAuthority
return normalizedURL.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, "\\", "/")
+ volume, remainder, rooted := splitPortableVolume(normalized)
+ if volume == "" {
+ return normalizeWindowsLocalPathCase(pathpkg.Clean(normalized))
+ }
+
+ if rooted {
+ cleaned := pathpkg.Clean("/" + remainder)
+
+ return normalizeWindowsLocalPathCase(volume + "/" +
strings.TrimPrefix(cleaned, "/"))
+ }
+
+ if remainder == "" {
+ return normalizeWindowsLocalPathCase(volume)
+ }
+
+ return normalizeWindowsLocalPathCase(volume + pathpkg.Clean(remainder))
+}
+
+func normalizeWindowsLocalPathCase(path string) string {
+ if isWindowsLocalPath(path) {
+ return strings.ToLower(path)
+ }
+
+ return path
+}
+
+func isWindowsLocalPath(path string) bool {
+ if len(path) >= 2 && isDriveLetter(path[0]) && path[1] == ':' {
+ return true
+ }
+
+ volume, _, rooted := splitPortableVolume(path)
+
+ return rooted && strings.HasPrefix(volume, "//")
+}
+
+func splitPortableVolume(path string) (volume, remainder string, rooted bool) {
+ if len(path) >= 2 && isDriveLetter(path[0]) && path[1] == ':' {
+ if len(path) >= 3 && path[2] == '/' {
+ return path[:2], path[3:], true
+ }
+
+ return path[:2], path[2:], false
+ }
+
+ if !strings.HasPrefix(path, "//") {
+ return "", path, false
+ }
+
+ unc := strings.TrimPrefix(path, "//")
+ serverEnd := strings.IndexByte(unc, '/')
+ if serverEnd <= 0 || serverEnd+1 >= len(unc) {
+ return "", path, false
+ }
+
+ shareStart := serverEnd + 1
+ shareEnd := strings.IndexByte(unc[shareStart:], '/')
+ if shareEnd < 0 {
+ return "//" + unc, "", true
+ }
+ if shareEnd == 0 {
+ return "", path, false
+ }
+
+ shareEnd += shareStart
+
+ return "//" + unc[:shareEnd], unc[shareEnd+1:], true
+}
+
+func isDriveLetter(r byte) bool {
Review Comment:
Tiny thing: the param is `r` but typed `byte`. `r` reads as "rune" in Go —
since drive letters are ASCII, `byte` is the right choice, I'd just rename it
to `b` or `c` so the name matches the type.
##########
table/orphan_cleanup.go:
##########
@@ -802,31 +804,99 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedScheme := applySchemeEquivalence(parsedURL.Scheme,
equalSchemes)
normalizedAuthority := applyAuthorityEquivalence(parsedURL.Host,
equalAuthorities)
- normalizedURL := &url.URL{
- Scheme: normalizedScheme,
- Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
- }
+
+ // Object-store paths are opaque keys. Keep their spelling exactly as
+ // supplied: escaped separators, duplicate slashes, and dot segments can
+ // all be meaningful parts of a key. Only the explicitly configured
scheme
+ // and authority equivalences are normalized here.
+ normalizedURL := *parsedURL
+ normalizedURL.Scheme = normalizedScheme
+ normalizedURL.Host = normalizedAuthority
return normalizedURL.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, "\\", "/")
+ volume, remainder, rooted := splitPortableVolume(normalized)
+ if volume == "" {
+ return normalizeWindowsLocalPathCase(pathpkg.Clean(normalized))
+ }
+
+ if rooted {
Review Comment:
A bare UNC share root like `//server/share` (no path component) comes back
as `//server/share/`, because it hits this `rooted` branch first and
`pathpkg.Clean("/" + "")` yields `/` which we then append. So
`normalizeNonURLPath` isn't idempotent across the first application for that
input, and a reference that ends exactly at the share root without the trailing
slash won't match.
Handling the empty-remainder case before the `if rooted` branch — returning
`normalizeWindowsLocalPathCase(volume)` directly, mirroring the non-rooted
empty case just below — fixes it.
##########
table/orphan_cleanup.go:
##########
@@ -802,31 +804,99 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedScheme := applySchemeEquivalence(parsedURL.Scheme,
equalSchemes)
normalizedAuthority := applyAuthorityEquivalence(parsedURL.Host,
equalAuthorities)
- normalizedURL := &url.URL{
- Scheme: normalizedScheme,
- Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
- }
+
+ // Object-store paths are opaque keys. Keep their spelling exactly as
+ // supplied: escaped separators, duplicate slashes, and dot segments can
+ // all be meaningful parts of a key. Only the explicitly configured
scheme
+ // and authority equivalences are normalized here.
+ normalizedURL := *parsedURL
+ normalizedURL.Scheme = normalizedScheme
+ normalizedURL.Host = normalizedAuthority
return normalizedURL.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, "\\", "/")
+ volume, remainder, rooted := splitPortableVolume(normalized)
+ if volume == "" {
+ return normalizeWindowsLocalPathCase(pathpkg.Clean(normalized))
+ }
+
+ if rooted {
+ cleaned := pathpkg.Clean("/" + remainder)
+
+ return normalizeWindowsLocalPathCase(volume + "/" +
strings.TrimPrefix(cleaned, "/"))
+ }
+
+ if remainder == "" {
+ return normalizeWindowsLocalPathCase(volume)
+ }
+
+ return normalizeWindowsLocalPathCase(volume + pathpkg.Clean(remainder))
+}
+
+func normalizeWindowsLocalPathCase(path string) string {
+ if isWindowsLocalPath(path) {
+ return strings.ToLower(path)
+ }
+
+ return path
+}
+
+func isWindowsLocalPath(path string) bool {
+ if len(path) >= 2 && isDriveLetter(path[0]) && path[1] == ':' {
+ return true
+ }
+
+ volume, _, rooted := splitPortableVolume(path)
+
+ return rooted && strings.HasPrefix(volume, "//")
Review Comment:
I think this is the crux of zeroshade's Windows-local concern, and it's
still not fully closed. `isWindowsLocalPath` returns true for any `//`-prefixed
volume regardless of host OS, so on a Linux box with an NFS/Samba mount like
`//nas/share/Data/file.parquet` we lowercase the whole path.
On a case-sensitive mount `//nas/share/Data/file` and
`//nas/share/data/file` are two different files, and folding them means a real
orphan silently survives. The direction is safe — we don't delete anything we
shouldn't — but it quietly defeats cleanup, and there's no test pinning
UNC-on-Linux behavior.
I'd gate the UNC lowercasing on `runtime.GOOS == "windows"` (the
drive-letter branch too), or if the conservative behavior is intentional, say
so in a comment and add a case-sensitive-mount test that locks it in. wdyt?
##########
table/orphan_cleanup.go:
##########
@@ -837,6 +907,12 @@ func filePathKey(file string) string {
// remain part of the comparison key.
if strings.Contains(file, "://") ||
strings.HasPrefix(strings.ToLower(file), "file:") {
if parsedURL, err := url.Parse(file); err == nil {
+ if parsedURL.Scheme != "" &&
!strings.EqualFold(parsedURL.Scheme, "file") {
+ // Keep remote object-key spelling, including
escaped separators,
+ // when grouping candidates for prefix-mismatch
checks.
+ return parsedURL.EscapedPath()
Review Comment:
There's an encoding asymmetry here that could send a live file to deletion.
For a remote scheme we return `parsedURL.EscapedPath()` (percent-encoded), but
for `file:` we fall through to `normalizeNonURLPath(parsedURL.Path)` (decoded).
So a local `file:///path%20to/file.parquet` keys as `/path to/file.parquet`
on one side and `/path%20to/file.parquet` on the other — the referenced and
listed forms don't match, and the file can land on the orphan list. Java's
`FileURI.path` uses `uri.getPath()` (decoded) uniformly for both.
I'd make the two branches symmetric — decode both (use `parsedURL.Path`) or
escape both — so indexing and lookup can't disagree.
##########
table/orphan_cleanup.go:
##########
@@ -802,31 +804,99 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedScheme := applySchemeEquivalence(parsedURL.Scheme,
equalSchemes)
normalizedAuthority := applyAuthorityEquivalence(parsedURL.Host,
equalAuthorities)
- normalizedURL := &url.URL{
- Scheme: normalizedScheme,
- Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
- }
+
+ // Object-store paths are opaque keys. Keep their spelling exactly as
Review Comment:
I get the intent here — treating object keys as opaque so `%2F`, duplicate
slashes, and dot segments are preserved, and this now lines up with PyIceberg's
raw-string compare. But it does diverge from the Java reference this file
cites: Hadoop `Path` resolves `..`/`.` at construction, and the PR flips
`complex_path_cleaning` to keep `s3://bucket/path/../other/./file.txt`
unresolved.
The practical risk is narrow — no conforming writer stores `..` in a
metadata path — but dropping the resolution also removes a last-line safety net
for a table historically maintained by Java cleanup. Since the comment already
cites Java, I'd at least call out that we're intentionally diverging from
Hadoop Path normalization, so the next reader doesn't "fix" it back. Are we
comfortable with that, or would a guard that skips (rather than deletes)
entries whose resolved form differs be worth it?
##########
table/orphan_cleanup.go:
##########
@@ -802,31 +804,99 @@ func normalizeURLPath(path string, cfg
*orphanCleanupConfig) string {
normalizedScheme := applySchemeEquivalence(parsedURL.Scheme,
equalSchemes)
normalizedAuthority := applyAuthorityEquivalence(parsedURL.Host,
equalAuthorities)
- normalizedURL := &url.URL{
- Scheme: normalizedScheme,
- Host: normalizedAuthority,
- Path: filepath.Clean(parsedURL.Path),
- }
+
+ // Object-store paths are opaque keys. Keep their spelling exactly as
+ // supplied: escaped separators, duplicate slashes, and dot segments can
+ // all be meaningful parts of a key. Only the explicitly configured
scheme
+ // and authority equivalences are normalized here.
+ normalizedURL := *parsedURL
Review Comment:
Copying the whole `*parsedURL` keeps `User`, `RawQuery`, and `Fragment` in
the comparison key, which the old explicit-struct construction dropped. That's
a false-orphan risk: a presigned S3 URL or an ABFS SAS token carries a query
string, so `s3://bucket/key?X-Amz-...` and `s3://bucket/key` now normalize
differently and one side can get labeled an orphan and deleted.
I'd construct it explicitly and only carry the fields we actually compare:
```go
normalizedURL := &url.URL{
Scheme: normalizedScheme,
Host: normalizedAuthority,
Path: parsedURL.Path,
RawPath: parsedURL.RawPath,
}
```
That keeps the opaque-key intent (RawPath preserves `%2F`) without dragging
query/user/fragment into the key.
##########
table/orphan_cleanup_test.go:
##########
@@ -254,6 +304,23 @@ func TestNormalizeNonURLPath(t *testing.T) {
}
}
+func TestNormalizeNonURLPathIsIdempotent(t *testing.T) {
+ tests := map[string]string{
+ "windows_drive": `C:\warehouse\data\..\file.parquet`,
+ "windows_drive_relative": `C:foo\..\bar`,
+ "unc": `\\server\share\data\..\file.parquet`,
+ "forward_slash_unc": `//server/share/../file.parquet`,
+ "unix": "/warehouse/data/../file.parquet",
+ }
+
+ for name, input := range tests {
+ t.Run(name, func(t *testing.T) {
+ normalized := normalizeNonURLPath(input)
+ assert.Equal(t, normalized,
normalizeNonURLPath(normalized))
Review Comment:
Two small things while we're here. The `assert.Equal` args are reversed —
testify is `Equal(t, expected, actual)`, so with `assert.Equal(t, normalized,
normalizeNonURLPath(normalized))` the labels come out backwards on failure. I'd
pull the second pass into a variable:
```go
secondPass := normalizeNonURLPath(normalized)
assert.Equal(t, normalized, secondPass, "normalizeNonURLPath must be
idempotent")
```
Also every other table test in this file uses a `[]struct{name, input,
expected}` slice; the `map[string]string` here iterates in random order, so
`-v` output ordering is nondeterministic. I'd switch it to a slice to match,
and add a bare `//server/share` case while doing so — that's the input that
exposes the trailing-slash edge above.
##########
table/orphan_cleanup_test.go:
##########
@@ -213,6 +213,16 @@ func TestNormalizeFilePath(t *testing.T) {
input: "s3a://bucket/path/file.txt",
expected: "s3://bucket/path/file.txt",
},
+ {
+ name: "windows_file_uri",
Review Comment:
The uppercase `FILE:` coverage landed nicely for `versionHintLocation`. The
one case-insensitive site this suite still doesn't pin is
`normalizeFilePathWithConfig` (and the matching check in `filePathKey`) —
`TestNormalizeFilePath` only feeds lowercase `file:`, so those
`strings.ToLower(...)` calls could be dropped and this suite would still pass.
An uppercase sibling right next to `windows_file_uri` — `input:
"FILE:///C:/warehouse/data/../file.parquet"`, `expected:
"c:/warehouse/file.parquet"`, plus a `filePathKey` case for `FILE:` — would
close the loop.
--
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]
