From 7ca048c2b30a3a167cc9f2899331569614c90b2e Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sun, 29 Sep 2024 07:14:09 +0900 Subject: [PATCH 1/2] Don't return null in Alluxio convertToLocation The subsequent logic doesn't expect receiving null. --- .../filesystem/alluxio/AlluxioFileIterator.java | 2 +- .../filesystem/alluxio/AlluxioFileSystem.java | 2 +- .../trino/filesystem/alluxio/AlluxioUtils.java | 16 ++++++---------- .../filesystem/alluxio/TestAlluxioUtils.java | 14 ++++++-------- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileIterator.java b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileIterator.java index ba16de33df26..e67a51f16e90 100644 --- a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileIterator.java +++ b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileIterator.java @@ -54,7 +54,7 @@ public FileEntry next() return null; } URIStatus fileStatus = files.next(); - Location location = convertToLocation(fileStatus, mountRoot); + Location location = convertToLocation(fileStatus.getPath(), mountRoot); return new FileEntry( location, fileStatus.getLength(), diff --git a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileSystem.java b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileSystem.java index 371e521baecb..b6a949ee44d6 100644 --- a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileSystem.java +++ b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioFileSystem.java @@ -291,7 +291,7 @@ public Set listDirectories(Location location) List filesStatus = alluxioClient.listStatus(convertToAlluxioURI(location, mountRoot)); return filesStatus.stream() .filter(URIStatus::isFolder) - .map((URIStatus fileStatus) -> AlluxioUtils.convertToLocation(fileStatus, mountRoot)) + .map((URIStatus fileStatus) -> AlluxioUtils.convertToLocation(fileStatus.getPath(), mountRoot)) .map(loc -> { if (!loc.toString().endsWith("/")) { return Location.of(loc + "/"); diff --git a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioUtils.java b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioUtils.java index 493e808fb35e..49ece36a8b93 100644 --- a/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioUtils.java +++ b/lib/trino-filesystem-alluxio/src/main/java/io/trino/filesystem/alluxio/AlluxioUtils.java @@ -14,31 +14,27 @@ package io.trino.filesystem.alluxio; import alluxio.AlluxioURI; -import alluxio.client.file.URIStatus; import io.trino.filesystem.Location; import java.util.ArrayDeque; import java.util.Deque; import java.util.Optional; +import static java.util.Objects.requireNonNull; + public class AlluxioUtils { private AlluxioUtils() {} - public static Location convertToLocation(URIStatus fileStatus, String mountRoot) + public static Location convertToLocation(String path, String mountRoot) { - if (fileStatus == null) { - return null; - } - String path = fileStatus.getPath(); - if (path == null) { - return null; - } + requireNonNull(path, "path is null"); + if (path.isEmpty()) { return Location.of(""); } if (path.startsWith("alluxio://")) { - return Location.of(fileStatus.getPath()); + return Location.of(path); } String schema = "alluxio://"; diff --git a/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java b/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java index f38ec2f6db49..f344e185a86c 100644 --- a/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java +++ b/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java @@ -14,8 +14,6 @@ package io.trino.filesystem.alluxio; import alluxio.AlluxioURI; -import alluxio.client.file.URIStatus; -import alluxio.wire.FileInfo; import io.trino.filesystem.Location; import org.junit.jupiter.api.Test; @@ -36,12 +34,12 @@ public void testSimplifyPath() public void convertToLocation() { String mountRoot = "/"; - URIStatus fileStatus = new URIStatus(new FileInfo().setPath("/mnt/test/level0-file0")); - assertThat(Location.of("alluxio:///mnt/test/level0-file0")).isEqualTo(AlluxioUtils.convertToLocation(fileStatus, mountRoot)); - fileStatus = new URIStatus(new FileInfo().setPath("/mnt/test/level0/level1-file0")); - assertThat(Location.of("alluxio:///mnt/test/level0/level1-file0")).isEqualTo(AlluxioUtils.convertToLocation(fileStatus, mountRoot)); - fileStatus = new URIStatus(new FileInfo().setPath("/mnt/test2/level0/level1/level2-file0")); - assertThat(Location.of("alluxio:///mnt/test2/level0/level1/level2-file0")).isEqualTo(AlluxioUtils.convertToLocation(fileStatus, mountRoot)); + String path = "/mnt/test/level0-file0"; + assertThat(Location.of("alluxio:///mnt/test/level0-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); + path = "/mnt/test/level0/level1-file0"; + assertThat(Location.of("alluxio:///mnt/test/level0/level1-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); + path = "/mnt/test2/level0/level1/level2-file0"; + assertThat(Location.of("alluxio:///mnt/test2/level0/level1/level2-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); } @Test From 3dfdb2e3efd9bcebd219e44de54fc4e7a9b90118 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sun, 29 Sep 2024 07:16:09 +0900 Subject: [PATCH 2/2] Minor cleanup in TestAlluxioUtils - Change the class to package-private and final - Change the method to package-private - Add missing test prefix - Replace actual and expected argument of assertions --- .../filesystem/alluxio/TestAlluxioUtils.java | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java b/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java index f344e185a86c..46f0e3215442 100644 --- a/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java +++ b/lib/trino-filesystem-alluxio/src/test/java/io/trino/filesystem/alluxio/TestAlluxioUtils.java @@ -17,40 +17,39 @@ import io.trino.filesystem.Location; import org.junit.jupiter.api.Test; +import static io.trino.filesystem.alluxio.AlluxioUtils.convertToAlluxioURI; +import static io.trino.filesystem.alluxio.AlluxioUtils.convertToLocation; +import static io.trino.filesystem.alluxio.AlluxioUtils.simplifyPath; import static org.assertj.core.api.Assertions.assertThat; -public class TestAlluxioUtils +final class TestAlluxioUtils { @Test - public void testSimplifyPath() + void testSimplifyPath() { - String path = "test/level0-file0"; - assertThat(path).isEqualTo(AlluxioUtils.simplifyPath(path)); - path = "a/./b/../../c/"; - assertThat("c/").isEqualTo(AlluxioUtils.simplifyPath(path)); + assertThat(simplifyPath("test/level0-file0")).isEqualTo("test/level0-file0"); + assertThat(simplifyPath("a/./b/../../c/")).isEqualTo("c/"); } @Test - public void convertToLocation() + void testConvertToLocation() { - String mountRoot = "/"; - String path = "/mnt/test/level0-file0"; - assertThat(Location.of("alluxio:///mnt/test/level0-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); - path = "/mnt/test/level0/level1-file0"; - assertThat(Location.of("alluxio:///mnt/test/level0/level1-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); - path = "/mnt/test2/level0/level1/level2-file0"; - assertThat(Location.of("alluxio:///mnt/test2/level0/level1/level2-file0")).isEqualTo(AlluxioUtils.convertToLocation(path, mountRoot)); + assertThat(convertToLocation("/mnt/test/level0-file0", "/")) + .isEqualTo(Location.of("alluxio:///mnt/test/level0-file0")); + assertThat(convertToLocation("/mnt/test/level0/level1-file0", "/")) + .isEqualTo(Location.of("alluxio:///mnt/test/level0/level1-file0")); + assertThat(convertToLocation("/mnt/test2/level0/level1/level2-file0", "/")) + .isEqualTo(Location.of("alluxio:///mnt/test2/level0/level1/level2-file0")); } @Test - public void testConvertToAlluxioURI() + void testConvertToAlluxioURI() { - Location location = Location.of("alluxio:///mnt/test/level0-file0"); - String mountRoot = "/"; - assertThat(new AlluxioURI("/mnt/test/level0-file0")).isEqualTo(AlluxioUtils.convertToAlluxioURI(location, mountRoot)); - location = Location.of("alluxio:///mnt/test/level0/level1-file0"); - assertThat(new AlluxioURI("/mnt/test/level0/level1-file0")).isEqualTo(AlluxioUtils.convertToAlluxioURI(location, mountRoot)); - location = Location.of("alluxio:///mnt/test2/level0/level1/level2-file0"); - assertThat(new AlluxioURI("/mnt/test2/level0/level1/level2-file0")).isEqualTo(AlluxioUtils.convertToAlluxioURI(location, mountRoot)); + assertThat(convertToAlluxioURI(Location.of("alluxio:///mnt/test/level0-file0"), "/")) + .isEqualTo(new AlluxioURI("/mnt/test/level0-file0")); + assertThat(convertToAlluxioURI(Location.of("alluxio:///mnt/test/level0/level1-file0"), "/")) + .isEqualTo(new AlluxioURI("/mnt/test/level0/level1-file0")); + assertThat(convertToAlluxioURI(Location.of("alluxio:///mnt/test2/level0/level1/level2-file0"), "/")) + .isEqualTo(new AlluxioURI("/mnt/test2/level0/level1/level2-file0")); } }