Skip to content

Core: Treat path prefixes as literals in RewriteTablePathUtil.replacePaths - #17521

Merged
szehon-ho merged 2 commits into
apache:mainfrom
uros-b:core-rewritetablepath-literal-prefix
Aug 18, 2026
Merged

Core: Treat path prefixes as literals in RewriteTablePathUtil.replacePaths#17521
szehon-ho merged 2 commits into
apache:mainfrom
uros-b:core-rewritetablepath-literal-prefix

Conversation

@uros-b

@uros-b uros-b commented Aug 4, 2026

Copy link
Copy Markdown
Member

RewriteTablePathUtil.replacePaths rewrote the table location with
String.replaceFirst(sourcePrefix, targetPrefix), which interprets the user-supplied
sourcePrefix as a regular expression and targetPrefix as a replacement string.
Both are file paths, so this misbehaves whenever a path contains regex metacharacters:

  • sourcePrefix = s3://bucket/warehouse.db/table also matches
    s3://bucket/warehouseXdb/table, rewriting a location that is not under the prefix.
  • A sourcePrefix with a trailing separator (e.g. s3://bucket/warehouse/table/) does not
    match at all, so the location is silently left pointing at the source while every
    other path is rewritten to the target.
  • A targetPrefix containing $ fails with IndexOutOfBoundsException: No group 1, and a
    path containing an unbalanced [ fails with PatternSyntaxException.

Every other path rewrite in this class already goes through newPath() -> relativize(),
which compares prefixes literally (startsWith + substring), normalizes trailing
separators, and raises a clear IllegalArgumentException when a path is not under the
source prefix. In RewriteTablePathSparkAction#rewriteVersionFile the immediately
preceding stagingPath(...) call already applies that literal check, so this change makes
the location rewrite consistent with the validation already performed one line earlier.

Both prefixes are user-supplied through the public
RewriteTablePath.rewriteLocationPrefix(sourcePrefix, targetPrefix) API.

Tested with TestRewriteTablePathUtil (core) and TestRewriteTablePathsAction on Spark
3.5, 4.0 and 4.1.

Note: #14355 makes the same one-line change as a side effect of adding multiple
source/destination prefixes. This is the minimal standalone fix plus regression tests for
the metacharacter and trailing-separator cases, so it can land independently; whichever
merges first, the other should rebase cleanly on this method.

@github-actions github-actions Bot added the core label Aug 4, 2026

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I came across this while working on #17484, cc @krisnaru who's currently working on #14355

@uros-b

uros-b commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

cc @szehon-ho

@uros-b

uros-b commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

cc @nastra

@szehon-ho szehon-ho 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.

LGTM, thanks for the clean fix.

metadata.location() was the last path in this class still rewritten via String.replaceFirst, meaning the user-supplied sourcePrefix was compiled as a regex Pattern and targetPrefix was interpreted as a replacement template. Every other path already went through newPath() -> relativize(), which compares prefixes literally and normalizes trailing separators.

A full example of the quietest failure mode. A user copying s3://src/db/table to s3://dst/db/table runs:

CALL sys.rewrite_table_path(
  table => 'db.table',
  source_prefix => 's3://src/db/table/',   -- trailing separator
  target_prefix => 's3://dst/db/table/'
)

For the location field, the old code evaluated:

"s3://src/db/table".replaceFirst("s3://src/db/table/", "s3://dst/db/table/")

The pattern is the source prefix itself, and its final / is a required character. The location stored in metadata has no trailing slash, so the pattern matches nowhere and replaceFirst returns the input unchanged. Meanwhile the manifest list, statistics files, metadata log entries and location properties all go through relativize(), which appends a separator to both sides before comparing, so those rewrite correctly. The staged metadata comes out as:

{
  "location": "s3://src/db/table",
  "snapshots": [
    {"manifest-list": "s3://dst/db/table/metadata/snap-123-1-abc.avro"}
  ]
}

The copy is staged under the target prefix but still declares the source location, and because every other path is correct the breakage is silent. With the fix, relativize() normalizes both sides to s3://src/db/table/ and combinePaths() yields s3://dst/db/table/.

One suggestion before merge: the description names three more failure modes that this change fixes but no test covers.

  • targetPrefix containing $, e.g. s3://bucket/cost$1/table - previously IndexOutOfBoundsException: No group 1, since $1 is a group reference in a replacement template.
  • sourcePrefix containing an unbalanced [, e.g. s3://bucket/db/table[0 - previously PatternSyntaxException: Unclosed character class.
  • sourcePrefix containing a balanced bracket, e.g. s3://bucket/db/table[0] - arguably the worst of the three, since [0] is a valid character class, so it silently failed to match the real directory table[0] while matching an unrelated table0.

Each is one assertion, in the shape of:

assertThat(RewriteTablePathUtil.replacePaths(metadata, sourcePrefix, "s3://bucket/cost$1/table").location())
    .isEqualTo("s3://bucket/cost$1/table");

Comment thread core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java

@szehon-ho szehon-ho 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.

LGTM, thanks for adding the extra cases.

One thing worth a line in the description before merge: this also changes what happens when the table location isn't under sourcePrefix. Before, replaceFirst silently returned the location unchanged; now relativize throws IllegalArgumentException. That's the right behavior — the old result was a staged metadata.json still pointing at the source — but it does turn a silently-wrong run into a hard failure, so it's worth calling out.

In practice very little reaches it. rewriteVersionFile calls stagingPath(versionFilePath, sourcePrefix, stagingDir) one line earlier, which relativizes the metadata file path against the same prefix, so an unrelated prefix already fails there today. You'd only hit the new exception with write.metadata.path pointing inside the source prefix while the table location sits outside it.

@szehon-ho
szehon-ho merged commit a561c8b into apache:main Aug 18, 2026
37 checks passed
@szehon-ho

Copy link
Copy Markdown
Member

Merged, thanks @uros-b !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants