voonhous commented on code in PR #19253: URL: https://github.com/apache/hudi/pull/19253#discussion_r3881293495
########## 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); Review Comment: Moot as of 6ddba4ced5d1: that class is deleted. The sweep now lives in `TestHoodieHadoopIOFactory`, whose methods are package-private (the existing `public` modifiers there were dropped as well). ########## 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: Merged master and dropped the native-log half in 6ddba4ced5d1: `shouldWriteNativeLogs` is format-independent on master (#19283), so there is no per-format dispatch left to sweep, and `TestCommonClientUtils#testShouldWriteNativeLogs` already pins the version gate. The `getLogBlockType` sweep (plus the `@Disabled` VORTEX case for #19252) is folded into `TestCommonClientUtils` and the second `TestFileFormatDispatchCoverage` class is deleted. -- 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]
