Skip to content

perf(table): flatten orphan URI equivalences - #1810

Merged
zeroshade merged 6 commits into
apache:mainfrom
fallintoplace:perf/flatten-orphan-uri-equivalences
Aug 17, 2026
Merged

perf(table): flatten orphan URI equivalences#1810
zeroshade merged 6 commits into
apache:mainfrom
fallintoplace:perf/flatten-orphan-uri-equivalences

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Closes #1808

What changed

  • Expand comma-separated scheme and authority groups once when options are applied.
  • Use direct map lookups during orphan path normalization and prefix checks.
  • Exact single-key mappings take precedence over comma-separated groups.
  • Overlapping groups are deterministic: the lexicographically last group wins. This intentionally differs from Java’s input-iteration-order behavior because Go map iteration is unordered.
  • Comma-joined group keys are configuration syntax and are not retained as lookup keys.
  • Add tests and a benchmark across path and group counts.

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/op is for one benchmark operation, which processes all paths in that case.

Paths Groups Before After Speedup
100 1 15,965 ns/op 908 ns/op 17.6x
100 100 480,338 ns/op 1,258 ns/op 381.8x
10,000 1 1,102,166 ns/op 72,268 ns/op 15.3x
10,000 100 51,545,935 ns/op 103,446 ns/op 498.3x

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 ./... passed
  • go vet ./table passed
  • GOMAXPROCS=1 go test ./table -run BenchmarkApplyURIEquivalence -benchmem -count=3 -v passed

@fallintoplace
fallintoplace marked this pull request as draft August 13, 2026 12:04

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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's flattenMap, 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.

  2. 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 in TestFlattenURIEquivalences'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.

  3. 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.

  4. Benchmark nit: the result == "" guard at the end can never trigger (applySchemeEquivalence always 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 the b.Fatal implies it's a real assertion.

None of these are blockers; happy to re-review once it's marked ready.

@zeroshade

Copy link
Copy Markdown
Member

Since this is a draft, i'll hold off on further review until it is marked ready

@fallintoplace
fallintoplace force-pushed the perf/flatten-orphan-uri-equivalences branch from 8cd4019 to 6e6bf73 Compare August 17, 2026 19:55
@fallintoplace
fallintoplace marked this pull request as ready for review August 17, 2026 19:56

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the ADLS authority correctness issue described inline. The flattening implementation and existing tests otherwise look good, but the linked issue explicitly requires coverage for ADLS container@host authorities, and that production path currently fails.

Comment thread table/orphan_cleanup.go
return authority
}

if canonical, exists := equalAuthorities[authority]; exists {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: normalizeURLPath and checkPrefixMismatch pass parsedURL.Host here, but Go's net/url separates abfs://container@host/... into User=container and Host=host. As a result, flattened keys such as container@account-a.dfs.core.windows.net never match, and normalizeURLPath also reconstructs the URL without User, dropping the container. A direct probe with two configured container@host authorities normalized them to different URLs (abfs://account-a... vs abfs://account-b...). Please perform authority lookup on the complete authority (User@Host) and preserve/reconstruct user-info when normalizing; the same helper should be used by the prefix-mismatch path.

"s3,s3a": "s3",
}),
WithEqualAuthorities(map[string]string{
"host1,host2": "canonical",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this exercise the required ADLS behavior through normalizeURLPath (and ideally checkPrefixMismatch) using abfs://container@account-*.dfs.core.windows.net/..., rather than only calling applyAuthorityEquivalence directly? That regression test currently fails because net/url removes container@ from URL.Host, which is the gap hidden by these host-only values.

@zeroshade
zeroshade merged commit 9982950 into apache:main Aug 17, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Table: flatten orphan URI equivalences

2 participants