github-actions[bot] commented on code in PR #64768:
URL: https://github.com/apache/doris/pull/64768#discussion_r4033173480
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/util/PathUtils.java:
##########
@@ -85,8 +168,11 @@ private static String normalize(String s) {
if (s == null) {
return "";
}
- // Remove trailing slashes for consistent comparison
- String trimmed = s.replaceAll("/+$", "");
- return trimmed.isEmpty() ? "" : trimmed;
+ // Skip the Matcher allocation when there is no trailing slash (the
overwhelmingly common
+ // case for real object-storage paths), letting the JIT avoid the
regex engine entirely.
+ if (s.isEmpty() || s.charAt(s.length() - 1) != '/') {
+ return s;
Review Comment:
[P1] Preserve repeated trailing slashes in object-store prefixes
This collapses `s3://bucket/data` and `s3://bucket/data//`, but BE does not:
the writer appends `"/" + fileName`, `S3URI::parse` preserves the repeated
slashes, and listing a target ending in `data//` uses `data//` as its prefix. A
valid trigger is an insert planned against `.../data` while an external HMS
update changes the table location to `.../data//` before `finishInsertTable`
re-fetches it. The helper now returns true, so `prepareInsertExistingTable`
completes the MPU at `data/<file>` instead of rejecting/relocating it, and the
current table cannot list that file. Please only normalize the single optional
directory delimiter (or compare prefixes with the BE join/list semantics) and
cover this commit-time location-change case.
##########
fe/fe-foundation/src/main/java/org/apache/doris/foundation/util/PathUtils.java:
##########
@@ -43,40 +62,104 @@ public static boolean equalsIgnoreSchemeIfOneIsS3(String
p1, String p2) {
return p1 == null && p2 == null;
}
+ // Fast path: identical raw strings are equal under every
normalization rule below, so we
+ // can skip the two URI parses (and their Matcher/substring
allocations) on the common
+ // unchanged-location case. This also makes the identity property hold
for inputs that would
+ // otherwise fall back to exact string comparison (opaque,
triple-slash, network-path).
+ if (p1.equals(p2)) {
+ return true;
+ }
+
try {
URI uri1 = new URI(p1);
URI uri2 = new URI(p2);
- String scheme1 = uri1.getScheme();
- String scheme2 = uri2.getScheme();
+ // Opaque URIs (e.g. "s3:bucket/key" with no "//") have null
authority and path, so they
+ // would all normalize to "" and compare equal regardless of
content. Such URIs are
+ // malformed for object-storage purposes; fall back to exact
string comparison.
+ if (uri1.isOpaque() || uri2.isOpaque()) {
+ return p1.equals(p2);
+ }
- // If schemes are equal, compare the full URI strings ignoring case
- if (scheme1 != null && scheme1.equalsIgnoreCase(scheme2)) {
- return p1.equalsIgnoreCase(p2);
+ // Two classes of inputs are malformed for object-storage purposes
and fall back to
+ // exact string comparison so they cannot spuriously match via the
structural path:
+ // 1. A URI with a scheme but a null authority (e.g. the
triple-slash form
+ // "s3:///path" or "file:///path"): its absent authority
would normalize to "" and
+ // could spuriously match another null-authority URI across
schemes.
+ // 2. A network-path reference: a URI with an authority but a
null scheme
+ // (e.g. "//bucket/path"). java.net.URI parses this as
scheme=null,
+ // authority="bucket". It is not a valid object-storage
location, so without this
+ // guard it would fall through to the structural comparison
and spuriously match a
+ // fully-qualified s3 URI ("s3://bucket/path").
+ // Schemeless bare paths (null scheme AND null authority, e.g.
"/path") are intentionally
+ // left to the structural comparison below so identical bare paths
still compare equal.
+ if ((uri1.getScheme() != null && uri1.getRawAuthority() == null)
+ || (uri2.getScheme() != null && uri2.getRawAuthority() ==
null)
+ || (uri1.getScheme() == null && uri1.getRawAuthority() !=
null)
+ || (uri2.getScheme() == null && uri2.getRawAuthority() !=
null)) {
+ return p1.equals(p2);
}
- // If schemes differ but one is "s3", compare only authority and
path ignoring scheme
- if ("s3".equalsIgnoreCase(scheme1) ||
"s3".equalsIgnoreCase(scheme2)) {
- String auth1 = normalize(uri1.getAuthority());
- String auth2 = normalize(uri2.getAuthority());
- String path1 = normalize(uri1.getPath());
- String path2 = normalize(uri2.getPath());
+ // Distinguish the schemeless filesystem root "/" from the
empty/current path "". With no
+ // scheme and no authority, both normalize() to "" (the
trailing-slash strip collapses
+ // "/" to ""), which would make them spuriously equal even though
they are distinct
+ // locations. This must NOT fire when an authority is present
(e.g. "s3://bucket/" vs
+ // "s3://bucket"), where the trailing slash on the bucket root IS
insignificant. The
+ // exact-string fallback returns false for "/" vs "" (the fast
path above already handled
+ // "/" == "/" and "" == "").
+ if (uri1.getScheme() == null && uri1.getRawAuthority() == null) {
+ String rawPath1 = uri1.getRawPath();
+ String rawPath2 = uri2.getRawPath();
+ if (("/".equals(rawPath1) && "".equals(rawPath2))
+ || ("".equals(rawPath1) && "/".equals(rawPath2))) {
+ return p1.equals(p2);
+ }
+ }
+
+ String scheme1 = uri1.getScheme();
+ String scheme2 = uri2.getScheme();
+
+ // Null-safe, case-insensitive scheme equality (RFC 3986 section
3.1). Two null schemes
+ // (e.g. bare paths) are treated as the same scheme so identical
schemeless locations
+ // compare equal. The explicit scheme2 != null guard makes the
null handling obvious
+ // without relying on String.equalsIgnoreCase(null)'s documented
behavior.
+ boolean sameScheme = scheme1 == null
+ ? scheme2 == null
+ : (scheme2 != null && scheme1.equalsIgnoreCase(scheme2));
+ boolean oneIsS3 = "s3".equalsIgnoreCase(scheme1) ||
"s3".equalsIgnoreCase(scheme2);
- return Objects.equals(auth1, auth2) && Objects.equals(path1,
path2);
+ // Different schemes and neither is "s3": treat as different
locations.
+ if (!sameScheme && !oneIsS3) {
+ return false;
}
- // Otherwise, URIs are not equal
- return false;
+ // Same scheme, or cross-scheme where one side is "s3" (object
stores are unified under
+ // the s3 scheme on the BE): the scheme is irrelevant -- compare
only the authority
+ // (bucket/host) and the path. The raw (still percent-encoded)
components are used so the
+ // comparison is byte-for-byte and case-sensitive (e.g. "a%2Fb"
differs from "a/b"), since
+ // object-storage keys are case-sensitive. Both are normalized so
a directory location
+ // compares equal with or without a trailing slash. Query strings
and fragments are also
+ // compared so two locations that differ only there are not
treated as identical.
+ // The authority is compared as-is (null -> ""); java.net.URI
never places trailing
+ // slashes in the authority (the "/" delimiter always belongs to
the path), so the
+ // trailing-slash normalize() is meaningful only for the path.
+ return Objects.equals(Objects.toString(uri1.getRawAuthority(), ""),
+ Objects.toString(uri2.getRawAuthority(), ""))
+ && Objects.equals(normalize(uri1.getRawPath()),
normalize(uri2.getRawPath()))
Review Comment:
[P2] Match query/fragment handling to the BE object identity
These components are not part of the authority-plus-path contract stated in
the PR, and BE's `S3URI::parse` strips everything from `?` or `#` before
constructing the object key. FE normalization currently accepts and preserves
such locations. If an insert is planned against `oss://bucket/data?v=1` and the
live HMS location is represented as `s3://bucket/data?v=2` at commit, both
resolve to the same BE bucket/key, but this comparison returns false.
`prepareInsertExistingTable` then takes rename and skips `objCommit`; the
object is still a pending MPU, so the insert cannot materialize it. Please
either reject query/fragment-bearing storage locations during normalization or
exclude these components here, and make the production commit-decision test pin
the chosen contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
