fix(table): preserve version hint URIs in orphan cleanup#1331
fix(table): preserve version hint URIs in orphan cleanup#1331fallintoplace wants to merge 2 commits into
Conversation
tanmayrauth
left a comment
There was a problem hiding this comment.
Clean fix and the write→read round-trip holds for s3/gs/file URIs and local paths alike. Just one small test-coverage nit. Details inline.
| } | ||
|
|
||
| func versionHintLocation(tableLocation string) string { | ||
| if strings.Contains(tableLocation, "://") || strings.HasPrefix(tableLocation, "file:") { |
There was a problem hiding this comment.
Nice catch adding || strings.HasPrefix(tableLocation, "file:") — since file:///x and file://host/x already contain ://, this branch is really only here to cover the single-slash file:/x opaque form. Just confirming that's the intent.
| } | ||
| } | ||
|
|
||
| func TestVersionHintLocation(t *testing.T) { |
There was a problem hiding this comment.
The three cases here all either contain :// (s3, file:///) or fall through to filepath.Join (local), so none of them actually exercise the file:-prefix-only branch above. A file:/tmp/table → file:/tmp/table/metadata/version-hint.text case would lock in the one input that branch uniquely guards.
Summary
build the orphan-cleanup version-hint path with a URI-aware join for URL locations
keep the existing filepath join fallback for plain local filesystem paths
add regression coverage for URI and local-path version-hint construction, plus the referenced-file set
Why
Table.getReferencedFilesaddsmetadata/version-hint.textto the referenced set so orphan cleanup will not delete it. That path was being built withfilepath.Join(metadata.Location(), ...), which is fine for plain local paths but corrupts object-store URIs likes3://bucket/tableintos3:/bucket/table/....Once that malformed path hit orphan cleanup's normalizer, it no longer looked like a URL and could diverge from the real
s3://.../version-hint.textcandidate path. That made the real version-hint file eligible to be treated as an orphan.Testing
go test ./table -run 'Test(VersionHintLocation|GetReferencedFiles_IncludesStatisticsFiles|.Orphan.)$' -count=1
go test ./table -count=1
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./table/...
git diff --check