voonhous commented on code in PR #19295: URL: https://github.com/apache/hudi/pull/19295#discussion_r3680148921
########## hudi-trino/src/test/java/io/trino/plugin/hudi/TestHudiMorPayloadSemantics.java: ########## @@ -0,0 +1,109 @@ +/* + * 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; + +import io.trino.plugin.hudi.testing.CompositeHudiTablesInitializer; +import io.trino.plugin.hudi.testing.DmsPayloadHudiTablesInitializer; +import io.trino.plugin.hudi.testing.OverwriteNonDefaultsPayloadHudiTablesInitializer; +import io.trino.plugin.hudi.testing.SummingPayloadHudiTablesInitializer; +import io.trino.plugin.hudi.testing.SummingTestPayload; +import io.trino.testing.AbstractTestQueryFramework; +import io.trino.testing.QueryRunner; +import org.junit.jupiter.api.Test; + +import static io.trino.plugin.hudi.testing.DmsPayloadHudiTablesInitializer.RT_TABLE_NAME; Review Comment: Done, dropped the static imports and qualified all three. `TestHudiMorMergeModeSemantics` had the same mixed pattern, so fixed it there too. ########## hudi-trino/src/test/java/io/trino/plugin/hudi/testing/EventTimeDeletesHudiTablesInitializer.java: ########## @@ -0,0 +1,160 @@ +/* + * 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.testing; + +import com.google.common.collect.ImmutableList; +import io.trino.metastore.Column; +import org.apache.avro.JsonProperties; +import org.apache.avro.Schema; +import org.apache.avro.generic.GenericData; +import org.apache.avro.generic.GenericRecord; +import org.apache.hudi.client.HoodieJavaWriteClient; +import org.apache.hudi.client.WriteStatus; +import org.apache.hudi.common.config.RecordMergeMode; +import org.apache.hudi.common.model.HoodieAvroPayload; +import org.apache.hudi.common.model.HoodieKey; +import org.apache.hudi.common.model.HoodieRecord; +import org.apache.hudi.common.table.HoodieTableMetaClient; +import org.apache.hudi.config.HoodieWriteConfig; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import static io.trino.metastore.HiveType.HIVE_BOOLEAN; +import static io.trino.metastore.HiveType.HIVE_LONG; +import static io.trino.metastore.HiveType.HIVE_STRING; +import static org.apache.hudi.common.model.HoodieRecord.HOODIE_IS_DELETED_FIELD; + +/** + * Creates a non-partitioned Merge-On-Read table in {@link RecordMergeMode#EVENT_TIME_ORDERING} that + * exercises the read-side merge-mode dispatch with deletes (issue apache/hudi#18898). ONLY a record merge + * mode is set (no payload class), so table creation persists the mode as-is, which is exactly the dispatch + * input {@code HudiTrinoReaderContext.getRecordMerger} switches on. + * <p> + * A base commit is followed by a log commit carrying an update, a soft delete + * ({@code _hoodie_is_deleted=true}), an OBSOLETE soft delete and an OBSOLETE update (both with an ordering + * value LOWER than the base row's, so event-time merging must keep the base row), and then a hard-delete + * commit ({@code writeClient.delete}) that produces a native delete log file read back through the + * connector's {@code getFileRecordIterator}. + * <p> + * Records are wrapped in {@link HoodieAvroPayload}, which implements {@code HoodieRecordPayload} directly + * (NOT {@code BaseAvroPayload}), so rows with {@code _hoodie_is_deleted=true} are written as DATA records + * and delete semantics are evaluated at READ time. See {@code TestHudiMorMergeModeSemantics}. + */ +public class EventTimeDeletesHudiTablesInitializer + extends AbstractMergerHudiTablesInitializer +{ + public static final String TABLE_NAME = "mor_deletes"; + public static final String RT_TABLE_NAME = TABLE_NAME + "_rt"; + + /** The single unnamed partition this fixture writes into, needed to address hard-deleted keys. */ + private static final String PARTITION_PATH = ""; Review Comment: Done, went with the `hoodieKey(String)` helper on the base class, and `avroRecord` now uses it too. Both `PARTITION_PATH` redeclarations are gone. ########## hudi-trino/src/main/java/io/trino/plugin/hudi/reader/HudiTrinoReaderContext.java: ########## @@ -243,9 +267,8 @@ protected Option<HoodieRecordMerger> getRecordMerger(RecordMergeMode mergeMode, // Dispatch on the table's merge mode, mirroring HoodieAvroReaderContext. The Trino reader // operates on IndexedRecord, so the Avro mergers apply directly. Using the read-time merger // (combineAndGetUpdateValue) rather than a fixed preCombine merger keeps COMMIT_TIME_ORDERING - // and custom-payload tables correct on MoR reads. - // TODO(apache/hudi#18898): add MoR read tests for delete markers and custom payloads to - // exercise the EVENT_TIME_ORDERING (combineAndGetUpdateValue) and CUSTOM branches below. + // and custom-payload tables correct on MoR reads; TestHudiMorMergeModeSemantics and + // TestHudiMorPayloadSemantics cover these branches end-to-end (apache/hudi#18898). Review Comment: You are right, `partialMerge` does make those arms reachable. Reworded the comment to say they are just not covered by these suites, and restored the TODO. -- 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]
