uros-b commented on code in PR #18304:
URL: https://github.com/apache/iceberg/pull/18304#discussion_r4144042696
##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -198,14 +197,17 @@ public String newDataLocation(String filename) {
}
private static String pathContext(String tableLocation) {
Review Comment:
pathContext() has zero test coverage. Every OBJECT_STORE_ENABLED test in
TestLocationProvider (testObjectStorageWithinTableLocation,
testEncodedFieldNameInPartitionPath, testExcludePartitionInPath,
testHashInjection) uses the default data location, so
storageLocation.startsWith(tableLocation) holds, context is null, and the
reimplemented method never executes; the two tests that set an external
object-store path use deprecated properties that throw first. Green CI does not
exercise the changed code at all.
Please consider adding a TestLocationProvider case with
OBJECT_STORE_ENABLED=true and WRITE_DATA_LOCATION outside the table prefix,
with computed (not hardcoded) expectations across scheme/authority, nested,
single-segment, and trailing-slash inputs.
##########
core/src/main/java/org/apache/iceberg/LocationProviders.java:
##########
@@ -198,14 +197,17 @@ public String newDataLocation(String filename) {
}
private static String pathContext(String tableLocation) {
- Path dataPath = new Path(tableLocation);
- Path parent = dataPath.getParent();
+ String path = LocationUtil.stripTrailingSlash(tableLocation);
+ int lastSlash = path.lastIndexOf('/');
+ String name = lastSlash < 0 ? path : path.substring(lastSlash + 1);
String resolvedContext;
- if (parent != null) {
+ if (lastSlash > 0) {
+ int parentSlash = path.lastIndexOf('/', lastSlash - 1);
// remove the data folder
- resolvedContext = String.format("%s/%s", parent.getName(),
dataPath.getName());
+ String parentName = path.substring(parentSlash + 1, lastSlash);
+ resolvedContext = String.format("%s/%s", parentName, name);
} else {
Review Comment:
The reimplementation is byte-identical to the old Hadoop Path logic for
every location with two or more path segments (the dominant
scheme://auth/db/table case — verified db/table both ways), but silently
diverges for single-path-segment locations. Traced at head: s3://bucket/table
-> old /table vs new bucket/table; /table -> old /table vs new table;
s3://bucket -> old "" vs new /bucket. This branch is reached only in
object-store mode with a write.data.path outside the table prefix, so it
changes the prefix where NEW files land for such tables (no data corruption —
manifests store full paths). The change ships inside a "refactor" with no
mention in the PR description. Either preserve the old output or document the
change, and add the coverage above.
--
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]
