fix(table): normalize orphan paths independently of host OS

[fallintoplace]

Summary

Use portable, volume-aware path normalization for orphan-cleanup comparisons.

Why

filepath.Clean follows the host OS separator rules. On Unix it does not clean Windows-style dot-dot segments, and on Windows it can introduce backslashes into URL paths. Treating Windows volumes as ordinary path components also allowed paths to escape drive and UNC roots.

What changed

Normalization now preserves rooted drives, drive-relative paths, and complete UNC shares while clamping dot-dot segments at the volume root. Local file URIs use the same portable cleaner.

Tests

• Added drive-root and drive-relative cases
• Added UNC share-root cases
• Added Windows and UNC file-URI cases
• Added orphan matching regressions
• go test ./table
• go vet ./table

[zeroshade]

Thanks for making path normalization host-independent. The most important structural property is correct here: listed paths normalize at table/orphan_cleanup.go:571, and referenced paths go through the same configured normalizer at table/orphan_cleanup.go:609-614. That symmetry should remain intact.

The Windows-local case handling is still a blocking deletion-safety issue, detailed inline.

Two follow-ups are outside the changed lines:

• table/orphan_cleanup.go:741 recognizes only lowercase file:. Suggested fix: detect the scheme case-insensitively.
• table/orphan_cleanup.go:773 falls back to filepath.Join after url.JoinPath fails for a URL-shaped location. Suggested fix: propagate that error instead of treating malformed URL input as a local path.

Please add Windows drive and UNC case-only deletion-safety tests, retain an S3 case-distinction test, and cover encoded separators, duplicate slashes, uppercase FILE:, authority cases, and an end-to-end dry run.

[zeroshade]

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.

[zeroshade]

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.

[laskoviymishka]

Thanks for the follow-up — making normalization host-independent is the right call, and the symmetry zeroshade flagged (listed and referenced paths going through the same configured normalizer) is intact. Both of his out-of-diff follow-ups landed too: the case-insensitive file: detection, and the versionHintLocation error propagation with an invalid-URL test to pin it.

I'd still hold this before merging though, because I don't think his blocking Windows-local concern is fully closed. isWindowsLocalPath treats any //server/share path as Windows-local on every OS, so on a Linux NFS/Samba mount we lowercase a case-sensitive path and can silently skip a real orphan — the same misclassification he raised, now living in the UNC branch rather than the drive-letter one. It's safe in that we don't delete anything we shouldn't, but it quietly defeats cleanup, and there's no test pinning UNC-on-Linux behavior.

While reviewing the diff I also hit two normalization paths that can send a live file to deletion, independent of the Windows question — the escaped-vs-decoded asymmetry in filePathKey, and the full *parsedURL copy that now drags query strings (presigned/SAS URLs) and user info into the comparison key. Details are inline.

Things I'd want to settle before merge:

• gate the UNC (and drive-letter) lowercasing on runtime.GOOS == "windows", or document the conservative behavior and add a UNC-on-Linux test
• make filePathKey symmetric — decode or escape both branches, not one each
• construct normalizedURL explicitly so query/user/fragment don't leak into the key
• an end-to-end pass through DeleteOrphanFiles with a mixed-case reference vs. a differently-cased scanned path, so a one-sided-normalization regression gets caught

On the opaque-key direction (dropping ../. resolution): I think it's defensible and it lines up with PyIceberg, but it does diverge from the Java reference the comment cites, so I'd at least note the divergence there.

Once those are settled, happy to take another pass and approve.

[laskoviymishka]

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?

[laskoviymishka]

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:

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.

[fallintoplace]

I don't think dropping RawQuery and Fragment is safe here though.

iceberg-go's object FileIO intentionally treats the portion after the authority as an opaque object key. In splitObjectLocation, ? and # are preserved as part of the raw key rather than interpreted as URL query/fragment components.

So s3://bucket/key, s3://bucket/key?version=1, and s3://bucket/key#metadata can represent distinct object names for the FileIO.

[laskoviymishka]

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.

[laskoviymishka]

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?

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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:

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.

[zeroshade]

The alias-set design resolves the deletion-safety question structurally: the exact original spelling is always the primary comparison identity — so case-sensitive POSIX paths (including Linux NFS/SMB mounts spelled //server/share) are never folded away and real orphans still match — while the Windows-folded and UNC-collapsed spellings participate only as conservative extra aliases, which can only cause a file to be kept. Ambiguity now has exactly one failure direction, and it's the safe one. The coverage matrix (case-only Windows/UNC safety, object-store key-case preservation, double-slash ambiguity, dot-segment clamping at drive and share roots, FILE: case-insensitivity with propagated URI errors) pins all the edges from both review rounds. Thorough work on a genuinely tricky problem.

Deferring thread closure to laskoviymishka since his change request predates the UNC-case commits.

---

This review was drafted with an AI-assisted tool and may contain mistakes; an Apache Iceberg Go maintainer has reviewed and confirmed the submission. See the contributing docs for what the project considers a maintainer review.