Skip to content

Fix file location in case of '/' root #17808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
assaf2 wants to merge 2 commits into from
Closed

Fix file location in case of '/' root #17808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5eb05e
Fix file location in case of '/' root
assaf2 Jun 8, 2023
652f2e2
fix - another suggestion
assaf2 Jun 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,18 +207,23 @@ public Location parentDirectory()
* Creates a new location by appending the given path element to the current path.
* A slash will be added between the current path and the new path element if needed.
*
* @throws IllegalArgumentException if the new path element is empty or starts with a slash
* @throws IllegalArgumentException if the new path element is empty
Copy link
Copy Markdown
Member

@findepi findepi Jun 12, 2023

Choose a reason for hiding this comment

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

I don't think we should be changing Location.appendPath API.
This can unwanted consequences. More conservative approach would be to keep disallowing leading / here and fix listing instead.

*/
public Location appendPath(String newPathElement)
{
checkArgument(!newPathElement.isEmpty(), "newPathElement is empty");
checkArgument(!newPathElement.startsWith("/"), "newPathElement starts with a slash: %s", newPathElement);

if (path.isEmpty()) {
if (newPathElement.startsWith("/")) {
newPathElement = newPathElement.substring(1);
}
return appendToEmptyPath(newPathElement);
}

if (!path.endsWith("/")) {
if (path.endsWith("/") && newPathElement.startsWith("/")) {
newPathElement = newPathElement.substring(1);
}
else if (!path.endsWith("/") && !newPathElement.startsWith("/")) {
newPathElement = "/" + newPathElement;
}
return withPath(location + newPathElement, path + newPathElement);
Expand Down
4 changes: 4 additions & 0 deletions lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,11 @@ void testAppendPath()
assertAppendPath("scheme:///path/", "name", Location.of("scheme:///path/name"));

assertAppendPath("/", "name", Location.of("/name"));
assertAppendPath("/", "/name", Location.of("/name"));
assertAppendPath("/path", "name", Location.of("/path/name"));
assertAppendPath("/path", "/name", Location.of("/path/name"));
assertAppendPath("/path/", "/name", Location.of("/path/name"));
assertAppendPath("/path/", "name", Location.of("/path/name"));
}

private static void assertAppendPath(String locationString, String newPathElement, Location expected)
Expand Down
2 changes: 1 addition & 1 deletion lib/trino-hdfs/src/main/java/io/trino/filesystem/hdfs/HdfsFileIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public FileEntry next()

verify(path.startsWith(root), "iterator path [%s] not a child of listing path [%s] for location [%s]", path, root, listingLocation);

Location location = listingLocation.appendPath(path.substring(root.length() + 1));
Location location = listingLocation.appendPath(path.substring(root.length()));

List<Block> blocks = Stream.of(status.getBlockLocations())
.map(HdfsFileIterator::toTrinoBlock)
Expand Down