voonhous commented on code in PR #19882: URL: https://github.com/apache/hudi/pull/19882#discussion_r4011728619
########## hudi-trino/src/test/java/io/trino/plugin/hudi/storage/TestHudiTrinoStorageExtensionPoint.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hudi.storage; + +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.memory.MemoryFileSystem; +import org.apache.hudi.common.util.HoodieStorageUtils; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StorageConfiguration; +import org.apache.hudi.storage.StoragePath; +import org.junit.jupiter.api.Test; + +import java.io.FileNotFoundException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class TestHudiTrinoStorageExtensionPoint +{ + private static final StoragePath PATH = new StoragePath("memory:///warehouse/table"); + + @Test + void testStorageResolvesFromConfiguration() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + StorageConfiguration<?> conf = new TrinoStorageConfiguration(fileSystem); + + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, conf); + + assertThat(storage).isInstanceOf(HudiTrinoStorage.class); + assertThat(storage.getConf()).isSameAs(conf); + } + + @Test + void testResolvedStorageIsUsable() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, new TrinoStorageConfiguration(fileSystem)); + + assertThatThrownBy(() -> storage.getPathInfo(new StoragePath(PATH, "absent"))) + .isInstanceOf(FileNotFoundException.class); Review Comment: **major:** This passes whichever filesystem the resolved storage ends up with: `getPathInfo` throws `FileNotFoundException` on `!inputFile.exists()` alone (`HudiTrinoStorage.java:189-190`), so any empty `MemoryFileSystem` satisfies it. A constructor that swapped in a fresh `MemoryFileSystem` would keep all four tests green. Could we assert the carried handle is the one passed in (or write a file through `fileSystem` and read its length back)? ```suggestion assertThat(((HudiTrinoStorage) storage).getFileSystem()).isSameAs(fileSystem); ``` ########## hudi-trino/src/test/java/io/trino/plugin/hudi/storage/TestHudiTrinoStorageExtensionPoint.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hudi.storage; + +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.memory.MemoryFileSystem; +import org.apache.hudi.common.util.HoodieStorageUtils; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StorageConfiguration; +import org.apache.hudi.storage.StoragePath; +import org.junit.jupiter.api.Test; + +import java.io.FileNotFoundException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class TestHudiTrinoStorageExtensionPoint +{ + private static final StoragePath PATH = new StoragePath("memory:///warehouse/table"); + + @Test + void testStorageResolvesFromConfiguration() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + StorageConfiguration<?> conf = new TrinoStorageConfiguration(fileSystem); + + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, conf); + + assertThat(storage).isInstanceOf(HudiTrinoStorage.class); + assertThat(storage.getConf()).isSameAs(conf); + } + + @Test + void testResolvedStorageIsUsable() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, new TrinoStorageConfiguration(fileSystem)); + + assertThatThrownBy(() -> storage.getPathInfo(new StoragePath(PATH, "absent"))) + .isInstanceOf(FileNotFoundException.class); + } + + @Test + void testConfigurationWithoutFileSystemFailsClearly() + { + assertThatThrownBy(() -> HoodieStorageUtils.getStorage(PATH, new TrinoStorageConfiguration())) + .rootCause() + .hasMessageContaining("carries no file system"); + } + + @Test + void testDerivedConfigurationsKeepTheFileSystem() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + TrinoStorageConfiguration conf = new TrinoStorageConfiguration(fileSystem); + + assertThat(HoodieStorageUtils.getStorage(PATH, conf.newInstance())) + .isInstanceOf(HudiTrinoStorage.class); + assertThat(HoodieStorageUtils.getStorage(PATH, conf.getInline())) + .isInstanceOf(HudiTrinoStorage.class); Review Comment: **minor:** Both assertions only check the resolved type, so a `newInstance()` that carried a *different* filesystem would still pass, though the test name promises the filesystem is kept. Not blocking. Could we assert identity on the derived configurations too? ```suggestion assertThat(HoodieStorageUtils.getStorage(PATH, conf.newInstance())) .isInstanceOf(HudiTrinoStorage.class); assertThat(HoodieStorageUtils.getStorage(PATH, conf.getInline())) .isInstanceOf(HudiTrinoStorage.class); assertThat(((TrinoStorageConfiguration) conf.newInstance()).getFileSystem()).containsSame(fileSystem); assertThat(((TrinoStorageConfiguration) conf.getInline()).getFileSystem()).containsSame(fileSystem); ``` ########## hudi-trino/src/main/java/io/trino/plugin/hudi/storage/HudiTrinoStorage.java: ########## @@ -54,6 +54,25 @@ public HudiTrinoStorage(TrinoFileSystem fileSystem, TrinoStorageConfiguration st this.fileSystem = fileSystem; } + public HudiTrinoStorage(StoragePath path, StorageConfiguration<?> storageConf) + { + this(requireTrinoStorageConfiguration(storageConf, path), path); + } + + private HudiTrinoStorage(TrinoStorageConfiguration storageConf, StoragePath path) + { + this(storageConf.getFileSystem().orElseThrow(() -> new IllegalArgumentException( + "Storage configuration for " + path + " carries no file system")), storageConf); + } + + private static TrinoStorageConfiguration requireTrinoStorageConfiguration(StorageConfiguration<?> storageConf, StoragePath path) + { + if (storageConf instanceof TrinoStorageConfiguration trinoConf) { + return trinoConf; + } + throw new IllegalArgumentException("Storage configuration for " + path + " is not a TrinoStorageConfiguration"); Review Comment: **minor:** The "is not a TrinoStorageConfiguration" branch is never reached by a test (every case passes a `TrinoStorageConfiguration`), so dropping the `instanceof` guard would go unnoticed. Not blocking. Could we add a case that sets `HOODIE_STORAGE_CLASS` to `HudiTrinoStorage` on a `HadoopStorageConfiguration` (already on the test classpath, see `UncompactedMetadataHudiTablesInitializer:231`) and asserts the root cause is an `IllegalArgumentException` with this message? ########## hudi-trino/src/test/java/io/trino/plugin/hudi/storage/TestHudiTrinoStorageExtensionPoint.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hudi.storage; + +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.memory.MemoryFileSystem; +import org.apache.hudi.common.util.HoodieStorageUtils; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StorageConfiguration; +import org.apache.hudi.storage.StoragePath; +import org.junit.jupiter.api.Test; + +import java.io.FileNotFoundException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class TestHudiTrinoStorageExtensionPoint +{ + private static final StoragePath PATH = new StoragePath("memory:///warehouse/table"); + + @Test + void testStorageResolvesFromConfiguration() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + StorageConfiguration<?> conf = new TrinoStorageConfiguration(fileSystem); + + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, conf); + + assertThat(storage).isInstanceOf(HudiTrinoStorage.class); + assertThat(storage.getConf()).isSameAs(conf); + } + + @Test + void testResolvedStorageIsUsable() + { + TrinoFileSystem fileSystem = new MemoryFileSystem(); + HoodieStorage storage = HoodieStorageUtils.getStorage(PATH, new TrinoStorageConfiguration(fileSystem)); + + assertThatThrownBy(() -> storage.getPathInfo(new StoragePath(PATH, "absent"))) + .isInstanceOf(FileNotFoundException.class); + } + + @Test + void testConfigurationWithoutFileSystemFailsClearly() + { + assertThatThrownBy(() -> HoodieStorageUtils.getStorage(PATH, new TrinoStorageConfiguration())) + .rootCause() + .hasMessageContaining("carries no file system"); Review Comment: **nit:** Only the message ties this to the intended branch; asserting the root-cause type too would make it explicit. Feel free to ignore. ```suggestion .rootCause() .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("carries no file system"); ``` ########## hudi-trino/src/test/java/io/trino/plugin/hudi/storage/TestHudiTrinoStorageExtensionPoint.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hudi.storage; + +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.memory.MemoryFileSystem; +import org.apache.hudi.common.util.HoodieStorageUtils; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StorageConfiguration; +import org.apache.hudi.storage.StoragePath; +import org.junit.jupiter.api.Test; + +import java.io.FileNotFoundException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class TestHudiTrinoStorageExtensionPoint +{ + private static final StoragePath PATH = new StoragePath("memory:///warehouse/table"); + + @Test + void testStorageResolvesFromConfiguration() Review Comment: **minor:** All four tests call `HoodieStorageUtils.getStorage` directly, but the motivating flow is `initTable`, which resolves twice (`createTableLayoutOnStorage` at `HoodieTableMetaClient.java:643`, then `setConf(...).build()` at `:919`) and later `ActiveTimelineV2.createFileInPath` via `getStorage(StoragePath)`. A local probe of that flow on `MemoryFileSystem` passes on this head. Not blocking. Could we add one test that runs `initTable` with `new TrinoStorageConfiguration(fileSystem)`, creates a requested instant, and asserts `hoodie.properties` and the instant file exist in `fileSystem`? ########## hudi-trino/src/test/java/io/trino/plugin/hudi/storage/TestHudiTrinoStorageExtensionPoint.java: ########## @@ -0,0 +1,74 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hudi.storage; + +import io.trino.filesystem.TrinoFileSystem; +import io.trino.filesystem.memory.MemoryFileSystem; +import org.apache.hudi.common.util.HoodieStorageUtils; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StorageConfiguration; +import org.apache.hudi.storage.StoragePath; +import org.junit.jupiter.api.Test; + +import java.io.FileNotFoundException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class TestHudiTrinoStorageExtensionPoint Review Comment: **minor:** These four cases exercise the same two classes as `TestHudiTrinoStorage` in this package, which already uses `MemoryFileSystem` and has the `writeFile` helper the identity check on `testResolvedStorageIsUsable` could use. Not blocking. Could we move them (and `PATH`) into `TestHudiTrinoStorage` and drop this file? -- 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]
