wombatu-kun commented on code in PR #19253: URL: https://github.com/apache/hudi/pull/19253#discussion_r3695431619
########## hudi-client/hudi-client-common/src/test/java/org/apache/hudi/utils/TestFileFormatDispatchCoverage.java: ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.hudi.utils; + +import org.apache.hudi.common.model.HoodieFileFormat; +import org.apache.hudi.common.table.HoodieTableConfig; +import org.apache.hudi.common.table.HoodieTableVersion; +import org.apache.hudi.common.table.log.block.HoodieLogBlock; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.util.CommonClientUtils; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Exhaustiveness tests for the per-{@link HoodieFileFormat} dispatch sites on the log write + * path in {@link CommonClientUtils}. + * + * <p>Per-format dispatch is duplicated across the codebase (reader/writer factories in + * hudi-common/hudi-hadoop-common, log block type selection here). When a new + * {@link HoodieFileFormat} value is added as a base file format, it is easy to wire the + * reader/writer factories but miss {@link CommonClientUtils#getLogBlockType}, in which case + * MOR log writes throw at runtime; that is exactly what happened with VORTEX (missing case + * threw HoodieException until apache/hudi#19252). These tests iterate + * {@code HoodieFileFormat.values()} (never a hardcoded list, excluding only HOODIE_LOG which + * is the log format itself and can never be a base file format) so they fail mechanically + * when a future format value misses this dispatch point. + */ +class TestFileFormatDispatchCoverage { + + /** + * Every format the write path can select as a base file format must map to a log block + * type; a missing case in the getLogBlockType switch fails MOR upserts at runtime. + */ + // TODO: include VORTEX here (remove the exclusion) once apache/hudi#19252 is merged; on + // current master getLogBlockType has no VORTEX case and throws HoodieException, which is + // exactly the gap this test exists to catch (see testGetLogBlockTypeForVortex). + @ParameterizedTest + @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE, names = {"HOODIE_LOG", "VORTEX"}) + void testGetLogBlockTypeMapsEveryBaseFileFormat(HoodieFileFormat format) { + assertNotNull( + CommonClientUtils.getLogBlockType(writeConfigWithoutExplicitLogFormat(), tableConfigWithBaseFormat(format)), + () -> "getLogBlockType must return a log block type for base file format " + format + + "; add the missing case to the switch in CommonClientUtils#getLogBlockType"); + } + + /** + * Same check as {@link #testGetLogBlockTypeMapsEveryBaseFileFormat} for VORTEX, asserting + * the post-fix mapping of apache/hudi#19252 (VORTEX -> AVRO_DATA_BLOCK). + */ + @Disabled("Depends on apache/hudi#19252: on current master getLogBlockType has no VORTEX case and throws " + + "HoodieException. Enable once #19252 is merged.") + @Test + void testGetLogBlockTypeForVortex() { + assertEquals(HoodieLogBlock.HoodieLogBlockType.AVRO_DATA_BLOCK, + CommonClientUtils.getLogBlockType( + writeConfigWithoutExplicitLogFormat(), tableConfigWithBaseFormat(HoodieFileFormat.VORTEX))); + } + + /** + * shouldWriteNativeLogs must produce a decision (never throw) for every base file format, + * and must never select native logs below writer version TEN regardless of format. + * (Which formats opt out of native logs at version TEN and above is format-specific policy, + * asserted case-by-case in TestCommonClientUtils.) + */ + @ParameterizedTest + @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE, names = {"HOODIE_LOG"}) + void testShouldWriteNativeLogsHandlesEveryBaseFileFormat(HoodieFileFormat format) { + HoodieWriteConfig writeConfig = writeConfigWithoutExplicitLogFormat(); + HoodieTableConfig tableConfig = tableConfigWithBaseFormat(format); + + when(writeConfig.getWriteVersion()).thenReturn(HoodieTableVersion.SIX); + assertFalse(CommonClientUtils.shouldWriteNativeLogs(writeConfig, tableConfig), Review Comment: `CommonClientUtils.shouldWriteNativeLogs` on master takes only the write config and no longer consults the base file format, so these two-arg calls do not compile after a rebase and the disabled VORTEX case asserts the opposite of current behavior. Worth rebasing and dropping the native-log half here, since `TestCommonClientUtils` already pins the version gate. ########## hudi-hadoop-common/src/test/java/org/apache/hudi/io/storage/hadoop/TestFileFormatDispatchCoverage.java: ########## @@ -0,0 +1,242 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.hudi.io.storage.hadoop; + +import org.apache.hudi.common.model.HoodieFileFormat; +import org.apache.hudi.common.testutils.HoodieTestUtils; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.core.io.storage.HoodieFileReader; +import org.apache.hudi.core.io.storage.HoodieFileReaderFactory; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StoragePath; +import org.apache.hudi.storage.StoragePathInfo; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.io.IOException; +import java.lang.reflect.Method; +import java.util.EnumSet; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.apache.hudi.common.util.ConfigUtils.DEFAULT_HUDI_CONFIG_FOR_READER; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +/** + * Exhaustiveness tests for the per-{@link HoodieFileFormat} dispatch sites on the read path. + * + * <p>The per-format {@code switch} statements are duplicated across factory overloads + * ({@link HoodieFileReaderFactory#getFileReader} has one switch per overload, and + * {@link HoodieHadoopIOFactory#getFileFormatUtils} has another). When a new + * {@link HoodieFileFormat} value is added, it is easy to add the new case to one dispatch + * point but miss another; e.g. the VORTEX case was added to the + * {@code getFileReader(HoodieConfig, StoragePath, ...)} overload but initially missed in the + * {@code getFileReader(HoodieConfig, StoragePathInfo, ...)} overload (fixed by + * apache/hudi#19252). These tests iterate {@code HoodieFileFormat.values()} (never a + * hardcoded list) and assert that all dispatch points treat every format consistently, so + * they fail mechanically when a future format value covers some dispatch points but not + * others. + */ +class TestFileFormatDispatchCoverage { + + private static final StoragePath TEST_PATH = new StoragePath("/partition/path/f1_1-0-1_000.parquet"); + + /** + * Outcome of pushing a format through one {@code getFileReader} dispatch point. + */ + private enum Dispatch { + // The switch has a case for the format and routed to the per-format factory method. + DISPATCHED, + // The switch fell through to the default case and threw UnsupportedOperationException. + UNSUPPORTED + } + + // TODO: include VORTEX here (remove the exclusion) once apache/hudi#19252 is merged; on + // current master the StoragePathInfo overload is missing the VORTEX case, which is exactly + // the asymmetry this test exists to catch (see testVortexReaderOverloadDispatchConsistency). + @ParameterizedTest + @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE, names = {"VORTEX"}) + public void testStoragePathAndStoragePathInfoOverloadsDispatchConsistently(HoodieFileFormat format) Review Comment: `TestHoodieFileReaderFactory` in #19255 runs the same enum sweep over both `getFileReader` overloads and the extension entry, sits in hudi-common next to the class under test, and additionally records which hook fired rather than a shared marker. Is the reader-overload half here still needed once that lands, or should this class keep only the `getFileFormatUtils` checks? ########## hudi-hadoop-common/src/test/java/org/apache/hudi/io/storage/hadoop/TestFileFormatDispatchCoverage.java: ########## @@ -0,0 +1,242 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.hudi.io.storage.hadoop; + +import org.apache.hudi.common.model.HoodieFileFormat; +import org.apache.hudi.common.testutils.HoodieTestUtils; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.core.io.storage.HoodieFileReader; +import org.apache.hudi.core.io.storage.HoodieFileReaderFactory; +import org.apache.hudi.storage.HoodieStorage; +import org.apache.hudi.storage.StoragePath; +import org.apache.hudi.storage.StoragePathInfo; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.io.IOException; +import java.lang.reflect.Method; +import java.util.EnumSet; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.apache.hudi.common.util.ConfigUtils.DEFAULT_HUDI_CONFIG_FOR_READER; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +/** + * Exhaustiveness tests for the per-{@link HoodieFileFormat} dispatch sites on the read path. + * + * <p>The per-format {@code switch} statements are duplicated across factory overloads + * ({@link HoodieFileReaderFactory#getFileReader} has one switch per overload, and + * {@link HoodieHadoopIOFactory#getFileFormatUtils} has another). When a new + * {@link HoodieFileFormat} value is added, it is easy to add the new case to one dispatch + * point but miss another; e.g. the VORTEX case was added to the + * {@code getFileReader(HoodieConfig, StoragePath, ...)} overload but initially missed in the + * {@code getFileReader(HoodieConfig, StoragePathInfo, ...)} overload (fixed by + * apache/hudi#19252). These tests iterate {@code HoodieFileFormat.values()} (never a + * hardcoded list) and assert that all dispatch points treat every format consistently, so + * they fail mechanically when a future format value covers some dispatch points but not + * others. + */ +class TestFileFormatDispatchCoverage { + + private static final StoragePath TEST_PATH = new StoragePath("/partition/path/f1_1-0-1_000.parquet"); + + /** + * Outcome of pushing a format through one {@code getFileReader} dispatch point. + */ + private enum Dispatch { + // The switch has a case for the format and routed to the per-format factory method. + DISPATCHED, + // The switch fell through to the default case and threw UnsupportedOperationException. + UNSUPPORTED + } + + // TODO: include VORTEX here (remove the exclusion) once apache/hudi#19252 is merged; on + // current master the StoragePathInfo overload is missing the VORTEX case, which is exactly + // the asymmetry this test exists to catch (see testVortexReaderOverloadDispatchConsistency). + @ParameterizedTest + @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE, names = {"VORTEX"}) + public void testStoragePathAndStoragePathInfoOverloadsDispatchConsistently(HoodieFileFormat format) + throws IOException { + assertReaderOverloadsDispatchConsistently(format); + } + + /** + * Same check as {@link #testStoragePathAndStoragePathInfoOverloadsDispatchConsistently} for + * VORTEX, asserting the post-fix behavior of apache/hudi#19252 (both overloads dispatch). + */ + @Disabled("Depends on apache/hudi#19252: on current master getFileReader(HoodieConfig, StoragePathInfo, ...) " + + "is missing the VORTEX case while the StoragePath overload has it. Enable once #19252 is merged.") + @Test + public void testVortexReaderOverloadDispatchConsistency() throws IOException { + HoodieFileReader marker = mock(HoodieFileReader.class); + HoodieFileReaderFactory factory = factoryWithMarkerReaders(marker); + assertEquals(Dispatch.DISPATCHED, dispatchByStoragePath(factory, marker, HoodieFileFormat.VORTEX)); + assertEquals(Dispatch.DISPATCHED, dispatchByStoragePathInfo(factory, marker, HoodieFileFormat.VORTEX)); + } + + @ParameterizedTest + @EnumSource(HoodieFileFormat.class) + public void testExtensionDispatchConsistentWithFormatDispatch(HoodieFileFormat format) throws IOException { + HoodieFileReader marker = mock(HoodieFileReader.class); + HoodieFileReaderFactory factory = factoryWithMarkerReaders(marker); + Dispatch byExtension = dispatchByExtension(factory, marker, format); + Dispatch byFormat = dispatchByStoragePath(factory, marker, format); + assertEquals(byFormat, byExtension, () -> String.format( + "Dispatch asymmetry for %s: getFileReader(HoodieConfig, StoragePath, %s) -> %s but the extension-based " + + "getFileReader(HoodieConfig, StoragePath) -> %s. A format handled by the format switch must also be " + + "recognized by the extension if-chain in HoodieFileReaderFactory, and vice versa.", + format, format, byFormat, byExtension)); + } + + @ParameterizedTest + @EnumSource(HoodieFileFormat.class) + public void testGetFileFormatUtilsThrowsOnlyUnsupportedOperationException(HoodieFileFormat format) + throws IOException { + try (HoodieStorage storage = HoodieTestUtils.getDefaultStorage()) { + HoodieHadoopIOFactory ioFactory = new HoodieHadoopIOFactory(storage); + try { + assertNotNull(ioFactory.getFileFormatUtils(format), + () -> "getFileFormatUtils(" + format + ") returned null instead of a FileFormatUtils"); + } catch (UnsupportedOperationException e) { + // Acceptable: the format has no FileFormatUtils implementation yet. Any other exception + // type (e.g. HoodieException, NPE) propagates and fails the test, flagging a dispatch + // point that handles a new format inconsistently with the rest. + } + } + } + + @Test + public void testFileFormatUtilsCoverAllReaderDispatchableFormats() throws IOException { Review Comment: The sweep only drives the `HoodieFileFormat` overload - `HoodieIOFactory#getFileFormatUtils(StoragePath)` is a second extension if-chain whose only coverage is the hardcoded list in `TestHoodieHadoopIOFactory`, which never got a VORTEX row. Driving the parameterized test through the path overload too would put it on the same enum sweep. -- 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]
