voonhous commented on code in PR #19470:
URL: https://github.com/apache/hudi/pull/19470#discussion_r3706421528
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/hadoop/fs/HadoopFSUtils.java:
##########
@@ -277,15 +277,36 @@ private static FSDataInputStream
getFSDataInputStreamForGCS(FSDataInputStream fs
* @return true if the inputstream or the wrapped one is of type
GoogleHadoopFSInputStream
*/
public static boolean isGCSFileSystem(FileSystem fs) {
- return fs.getScheme().equals(StorageSchemes.GCS.getScheme());
+ return StorageSchemes.GCS.getScheme().equals(getScheme(fs));
}
/**
* Chdfs will throw {@code IOException} instead of {@code EOFException}. It
will cause error in isBlockCorrupted().
* Wrapped by {@code BoundedFsDataInputStream}, to check whether the desired
offset is out of the file size in advance.
*/
public static boolean isCHDFileSystem(FileSystem fs) {
- return StorageSchemes.CHDFS.getScheme().equals(fs.getScheme());
+ return StorageSchemes.CHDFS.getScheme().equals(getScheme(fs));
+ }
+
+ /**
+ * Resolves the scheme of {@code fs} without depending on {@link
FileSystem#getScheme()}.
+ *
+ * <p>{@code getScheme()} is optional in Hadoop: {@link FileSystem}'s own
implementation throws
+ * {@link UnsupportedOperationException}, and proxy implementations such as
Presto's
+ * {@code PrestoS3FileSystem} do not override it, so calling it unguarded
turns an unrelated read into
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
(HUDI-4602).
+ * {@link FileSystem#getUri()} is abstract, so every implementation supplies
it, and its scheme is what
+ * {@code getScheme()} returns wherever both are present.
+ *
+ * @param fs instance of {@link FileSystem} in use.
+ * @return the scheme of {@code fs}, or null if its URI carries none.
+ */
+ public static String getScheme(FileSystem fs) {
+ try {
+ return fs.getScheme();
+ } catch (UnsupportedOperationException e) {
+ return fs.getUri().getScheme();
+ }
Review Comment:
`HoodieException` is the right call, keep it. I checked: `HoodieIOException`
only declares `(String)` and `(String, IOException)`
(`hudi-io/src/main/java/org/apache/hudi/exception/HoodieIOException.java:32,37`),
so my snippet would not have compiled as written. Wrapping the
`UnsupportedOperationException` in an `IOException` just to fit that signature
is exactly the invented layer you were right to avoid, and it would bury the
cause one frame deeper for no gain.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestHadoopFSUtils.java:
##########
@@ -22,22 +22,80 @@
import org.apache.hudi.storage.StoragePath;
import org.apache.hudi.storage.StoragePathInfo;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.FilterFileSystem;
import org.apache.hadoop.fs.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
import static
org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopFileStatus;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToHadoopPath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePath;
import static org.apache.hudi.hadoop.fs.HadoopFSUtils.convertToStoragePathInfo;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
/**
* Tests {@link HadoopFSUtils}
*/
public class TestHadoopFSUtils {
+ /**
+ * HUDI-4602: {@link FileSystem#getScheme()} is optional in Hadoop -- the
base implementation throws
+ * {@link UnsupportedOperationException} -- and proxy implementations such
as Presto's
+ * {@code PrestoS3FileSystem} do not override it. Opening a log file went
straight through
+ * {@code isGCSFileSystem}, so a MOR {@code _rt} query on Presto failed with
+ * "Not implemented by the PrestoS3FileSystem FileSystem implementation"
rather than reading anything.
+ *
+ * <p>{@link FilterFileSystem} has the same shape: it leaves {@code
getScheme()} to the throwing base
+ * implementation while overriding {@code getUri()}.
+ */
+ @Test
+ public void testGetFSDataInputStreamWhenGetSchemeIsUnimplemented(@TempDir
File tempDir) throws IOException {
Review Comment:
Qualified is fine, no need to do the unqualified version. Converting the two
pre-existing conversion tests to `org.apache.hadoop.fs.Path` would widen the
diff well past what this PR is about, and the nit was only ever about the
`@TempDir File` plus `file.toPath()` round-trip, which is gone.
--
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]
