perf(table): flatten orphan URI equivalences - #1810
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The flattening approach is correct and the precedence tests are thorough. I verified all production paths go through newOrphanCleanupConfigWithMode, so the flatten step is applied consistently, and the raw-option test (TestOrphanCleanupOptions) still exercises the pre-flatten maps as intended. Tests pass on the PR head.
A few points I'd like your take on before this leaves draft:
-
flattenURIEquivalences(table/orphan_cleanup.go:176) introduces a defined precedence contract — exact keys beat groups, and among overlapping groups the lexicographically last wins. The old code was nondeterministic (map iteration order), so determinism is strictly better, but note this also diverges from Java'sflattenMap, which is last-put-wins over input iteration order with no exact-key precedence. I think your choice is the more sensible one; just confirm it's intentional and consider stating the divergence in the function comment alongside the existing Java references, since the other helpers in this file cite Java line numbers as the behavioral source of truth. -
The flattened map retains the original comma-joined group keys (
"s3,s3a,s3n"stays as a key via the overlay loop). Harmless since a URI scheme/authority can't contain a comma, but it's slightly surprising and shows up inTestFlattenURIEquivalences's expected map. If it's only there to make the overlay loop simpler, a short comment (or skipping comma keys in the overlay) would help future readers. -
Nit: the license header in table/orphan_cleanup_bench_test.go has a blank line after the license URL, splitting it into two comment blocks — the other files in the package use one contiguous block. Worth fixing so RAT/header tooling doesn't trip on it.
-
Benchmark nit: the
result == ""guard at the end can never trigger (applySchemeEquivalencealways returns a non-empty string for non-empty input), so it's dead code — fine to keep as a sink to prevent dead-code elimination, but theb.Fatalimplies it's a real assertion.
None of these are blockers; happy to re-review once it's marked ready.
Closes #1808
What changed
Benchmark
I ran the same benchmark before and after this change on an Apple M1 Pro with
GOMAXPROCS=1. These are the medians from 3 runs.ns/opis for one benchmark operation, which processes all paths in that case.The old implementation also allocated 100, 5,050, 10,000, and 505,611 objects per operation for those four cases. The new implementation reported 0 allocations per operation in every case.
Tests run
go test ./...passedgo test ./tablepassedgo vet ./tablepassedGOMAXPROCS=1 go test ./table -run '^$' -bench '^BenchmarkApplyURIEquivalence$' -benchmem -count=3 -vpassed