nsivabalan commented on code in PR #19205: URL: https://github.com/apache/hudi/pull/19205#discussion_r3781957780
########## hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/functional/TestMetaFieldsModeE2E.java: ########## @@ -0,0 +1,830 @@ +/* + * 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.functional; + +import org.apache.hudi.DataSourceReadOptions; +import org.apache.hudi.DataSourceWriteOptions; +import org.apache.hudi.SparkAdapterSupport$; +import org.apache.hudi.common.config.HoodieMetadataConfig; +import org.apache.hudi.common.model.HoodieRecord; +import org.apache.hudi.common.model.MetaFieldsMode; +import org.apache.hudi.common.table.HoodieTableConfig; +import org.apache.hudi.common.table.HoodieTableMetaClient; +import org.apache.hudi.common.table.timeline.HoodieInstant; +import org.apache.hudi.testutils.SparkClientFunctionalTestHarness; + +import org.apache.spark.sql.Dataset; +import org.apache.spark.sql.Row; +import org.apache.spark.sql.RowFactory; +import org.apache.spark.sql.SaveMode; +import org.apache.spark.sql.functions; +import org.apache.spark.sql.types.DataTypes; +import org.apache.spark.sql.types.StructField; +import org.apache.spark.sql.types.StructType; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +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.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Spark-datasource end-to-end tests for the {@code hoodie.meta.fields.mode} property on CoW tables. + * Every {@link MetaFieldsMode} value is exercised via a write / re-read round trip; on-disk column + * population is verified by reading the parquet files back and inspecting the meta-column values. + */ +class TestMetaFieldsModeE2E extends SparkClientFunctionalTestHarness { + + private static StructType simpleSchema() { + return DataTypes.createStructType(new StructField[]{ + DataTypes.createStructField("column1", DataTypes.StringType, true), + DataTypes.createStructField("column2", DataTypes.StringType, true), + DataTypes.createStructField("column3", DataTypes.StringType, true) + }).asNullable(); + } + + private Map<String, String> baseOptions() { + Map<String, String> opts = new HashMap<>(); + opts.put(DataSourceWriteOptions.RECORDKEY_FIELD().key(), "column1"); Review Comment: Agreed the matrix is missing, and I'll add it. But I probed the premise first, and one part of it does not hold — worth stating before we encode it in tests. ### The read-side limitation you cite is stale `HoodieBaseRelation:117-128` does assert `checkState(keyFields.length == 1)`, with a comment saying `SimpleKeyGenerator` is "the only `KeyGenerator` permitted for virtual-keys payloads". Two things about it: 1. **It is a field-count check, not a keygen-class check.** A single-field `ComplexKeyGenerator` or a `NonpartitionedKeyGenerator` passes it. So the allowlist admitting COMPLEX and NON_PARTITION is not by itself in conflict with the read side. 2. **That path is no longer reached for ordinary reads.** `DefaultSource:344-352` routes CoW snapshot and read-optimized queries to `HoodieCopyOnWriteSnapshotHadoopFsRelationFactory`; `resolveBaseFileOnlyRelation` — and with it `HoodieBaseRelation` — is reached only when `isNotMetadataTable` is false, i.e. for the metadata table. I wrote a throwaway test to settle it rather than argue from the code: a **two-field** `ComplexKeyGenerator` table under `COMMIT_TIME_ONLY`, written and then read back through `format("hudi")`. It serves both rows, via `HoodieCopyOnWriteSnapshotHadoopFsRelationFactory`. So the combination you flagged as reachable-and-broken is reachable and *works*; the comment describes a limitation the current reader does not have. ### What I am doing, and what I am not **Adding** the matrix for the combinations that are genuinely exercised by this feature — {simple, single-field complex, non-partitioned} x {`hive_style_partitioning` on/off} under `COMMIT_TIME_ONLY`, write and read back. Your two prior-bug citations (#5664 hive-style + virtual keys, #5747 non-partitioned + virtual keys) are the reason this is worth the cells rather than one representative case. **Not** touching the write/read gap in this PR. There is a genuine one — the writer never checks the record-key field count, so nothing stops a multi-field virtual-key table being written even where a reader would reject it — but: - it predates this PR, and - selective modes do not widen it. `COMMIT_TIME_ONLY` yields the same `populateMetaFields() == false` on the read path that `populate.meta.fields=false` already did, so every combination reachable now was reachable before. What selective modes change is *plausibility*, not reachability: `populate.meta.fields=false` is documented as being for append-only data, so nobody paired it with complex keys and mutation; `COMMIT_TIME_ONLY` targets tables that keep taking writes. That is a good argument for the tests. It is not an argument for narrowing `NO_METAFIELDS_KEYGEN_ALLOWLIST` inside this PR, which would be a user-visible compatibility break landing where no reviewer would look for one. I would rather raise the count check as its own issue. Happy to file it, and happy to be argued out of the split if you think the break is small enough to fold in here. Separately: I will also correct the `HoodieBaseRelation` comment, since it now misdescribes the constraint and is what sent both of us down this path. -- 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]
