Skip to content

fix(catalog): encode default table locations with UTF-8#4975

Open
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:fix/table-location-utf8-encoding
Open

fix(catalog): encode default table locations with UTF-8#4975
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:fix/table-location-utf8-encoding

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

buildPrefixedLocation in LocalIcebergCatalog derives a table's durable default
storage location by percent-encoding each namespace level and the table name with
URLEncoder.encode(..., Charset.defaultCharset()). Using the JVM's default charset
means 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 canonical
namespace codec PolarisEntityUtils.encodeNamespace (which already uses
StandardCharsets.UTF_8) and every other URLEncoder/URLDecoder call site in the
service, 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 a
table 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/table
suffix 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

  • Don't disclose security issues! (contact security@apache.org) - N/A, not a
    security issue.
  • Clearly explained why the changes are needed, or linked related issues - see
    above (no matching issue; self-found correctness bug, so no Fixes # line).
  • Added/updated tests with good coverage, or manually tested (and explained how) -
    new regression test in IcebergOverlappingTableTest.
  • Added comments for complex logic - N/A (one-line encoder change; the test carries
    an explanatory comment).
  • Updated CHANGELOG.md - added a ### Fixes entry under [Unreleased].
  • Updated documentation in site/content/in-dev/unreleased - N/A (no user-facing
    config/API surface changes; only the encoding of a derived internal path).

Note when actually filling the template on GitHub: check the boxes that apply and
leave the N/A ones per the notes above. Do not paste these parenthetical notes into
the PR verbatim - they are guidance for Anas.


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:

Prepared with AI assistance; I have reviewed the change and take responsibility for 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 already
explains why + what. No Fixes #/Related to # line because there is no matching issue.

fix(catalog): encode default table locations with UTF-8

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.

Files changed (3)

CHANGELOG.md                                                                    | 1 +
runtime/service/.../catalog/iceberg/LocalIcebergCatalog.java                    | 6 +-  (import swap + 2 encoder args)
runtime/service/src/test/.../catalog/iceberg/IcebergOverlappingTableTest.java   | 69 ++  (new test + 2 helper overloads)

Copilot AI review requested due to automatic review settings July 5, 2026 18:35
@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.buildPrefixedLocation to URLEncoder.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 dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch 👍 Thanks for your contribution, @anxkhn !


@Test
@DisplayName("Hashed default location encodes non-ASCII identifiers with UTF-8")
void testHashedTableLocationsEncodeNonAsciiIdentifiersAsUtf8(@TempDir Path tempDir) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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>
@anxkhn anxkhn force-pushed the fix/table-location-utf8-encoding branch from c79aa1e to 989b96c Compare July 12, 2026 11:52
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.

3 participants