fix(catalog): encode default table locations with UTF-8#4975
Open
anxkhn wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the derived default table storage location (when object-storage prefixing is enabled) deterministic across JVMs by encoding namespace/table path segments using UTF-8 instead of the platform default charset. This aligns LocalIcebergCatalog with other URL encoding/decoding call sites and the existing namespace codec, and adds a regression test plus a changelog note.
Changes:
- Switch
LocalIcebergCatalog.buildPrefixedLocationtoURLEncoder.encode(..., StandardCharsets.UTF_8)for namespace levels and table name. - Add a regression test covering non-ASCII namespace/table identifiers to ensure the stored location uses UTF-8 percent-encoding.
- Document the behavior change in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CHANGELOG.md | Notes the switch to UTF-8 percent-encoding for derived default table locations under object-storage prefixing. |
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java | Uses StandardCharsets.UTF_8 for percent-encoding namespace/table segments to make derived locations charset-independent. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergOverlappingTableTest.java | Adds a regression test asserting UTF-8 encoding for non-ASCII identifiers and that raw identifiers don’t appear in the durable location. |
dimas-b
reviewed
Jul 7, 2026
|
|
||
| @Test | ||
| @DisplayName("Hashed default location encodes non-ASCII identifiers with UTF-8") | ||
| void testHashedTableLocationsEncodeNonAsciiIdentifiersAsUtf8(@TempDir Path tempDir) { |
Contributor
There was a problem hiding this comment.
This is a good test, but it probably does not belong in the "overlapping table" test class. Why not put it into LocalIcebergCatalogTest?
buildPrefixedLocation derived a table's default storage location by percent-encoding the namespace levels and table name with URLEncoder.encode(..., Charset.defaultCharset()). For non-ASCII identifiers the persisted location then depended on the JVM's default charset, so the same table could resolve to a different (or lossy) location on a non-UTF-8 JVM, breaking portability and round-trip expectations. Encode both segments with StandardCharsets.UTF_8, matching PolarisEntityUtils.encodeNamespace and every other URLEncoder call site in the service. Add a regression test that creates a table under non-ASCII namespace and table identifiers with object-storage prefixing enabled and asserts the derived location uses the UTF-8 percent-encoded form regardless of the platform default charset. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
c79aa1e to
989b96c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
buildPrefixedLocationinLocalIcebergCatalogderives a table's durable defaultstorage location by percent-encoding each namespace level and the table name with
URLEncoder.encode(..., Charset.defaultCharset()). Using the JVM's default charsetmeans that, for a namespace or table whose name contains non-ASCII characters, the
persisted location depends on the platform default charset of whatever JVM created the
table. On a non-UTF-8 JVM the same logical table resolves to a different (or lossy)
location than on a UTF-8 JVM, which breaks portability and the round-trip expectation
that a table's location is stable and deterministic.
This encodes both segments with
StandardCharsets.UTF_8. That matches the canonicalnamespace codec
PolarisEntityUtils.encodeNamespace(which already usesStandardCharsets.UTF_8) and every otherURLEncoder/URLDecodercall site in theservice, so the derived location is now deterministic and charset-independent. These
two call sites were the only ones still using the platform default charset.
Current behavior: with object-storage prefixing enabled
(
DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED), the hashed default location for atable under a non-ASCII namespace/table name is percent-encoded using the JVM default
charset, so it is not stable across deployments with different default charsets.
Expected behavior after this change: the derived location is always the UTF-8
percent-encoded form, independent of the JVM default charset.
Added a regression test (
IcebergOverlappingTableTest,testHashedTableLocationsEncodeNonAsciiIdentifiersAsUtf8) that creates a catalog,namespace, and table using non-ASCII identifiers with object-storage prefixing enabled
and asserts the derived location ends with the UTF-8 percent-encoded
namespace/tablesuffix and never contains the raw identifiers. The two small private helper overloads it
uses delegate from the existing helper signatures, so all existing callers are
untouched.
Checklist
security issue.
above (no matching issue; self-found correctness bug, so no
Fixes #line).new regression test in
IcebergOverlappingTableTest.an explanatory comment).
CHANGELOG.md- added a### Fixesentry under[Unreleased].site/content/in-dev/unreleased- N/A (no user-facingconfig/API surface changes; only the encoding of a derived internal path).
Optional AI-assistance disclosure (Anas's call)
Polaris CONTRIBUTING.md ("Guidelines for AI-assisted Contributions") encourages
(does not require) disclosing significant AI assistance, and is explicit that the human
submitter is the author and must understand and be able to justify the change end-to-end
in review. This diff is small and the rationale is fully explained above, so a one-line
disclosure is low-cost and honest. Suggested line to append to the PR body if you want
it:
Include it or not at your discretion - just be ready to explain the encoding fix and the
test if a reviewer asks.
Commit
Single commit, Conventional Commits style matching repo history, no DCO sign-off
(correct: ASF uses the ICLA; the history has zero
Signed-off-by). Message body alreadyexplains why + what. No
Fixes #/Related to #line because there is no matching issue.Files changed (3)