nsivabalan commented on PR #19205: URL: https://github.com/apache/hudi/pull/19205#issuecomment-5082488817
Pushed [`8c98ec2`](https://github.com/apache/hudi/pull/19205/commits/8c98ec2e9f90) — correctness fixes found by auditing all 103 `populateMetaFields()` / `POPULATE_META_FIELDS` references while scoping the deprecation follow-up. The audit turned up sites that read the deprecated boolean directly instead of resolving `hoodie.meta.fields.mode`. With a selective mode set and the boolean left at its `true` default, these resolve the wrong way — and several are reachable today on the Spark CoW path this PR enables. Fixing them here rather than in the follow-up, since they are defects in the feature being shipped. **Record-key reads → NPE.** `HoodieSparkRecord` and `HoodieAvroIndexedRecord` take the meta-column branch under a selective mode, then dereference the null `_hoodie_record_key` ordinal. **Writer factories bypassing the mode.** The Lance and Vortex paths in `HoodieSparkFileWriterFactory`, and the HFile path in `HoodieAvroFileWriterFactory`, would stamp all five meta columns and enable the bloom filter — the opposite of what the mode requests. (The HFile site also used `getBoolean` rather than `getBooleanOrDefault`, which can NPE on unbox when neither property is set.) **Schema reconstruction asking the wrong question.** `SparkValidatorUtils`, `PartitionStatsIndexer` and `SparkMetadataWriterUtils` ask "are meta columns on disk?" using an is-record-key-populated predicate. Selective modes write the meta columns as physical *nullable* columns, so the correct test is `mode != NONE` — the same predicate `TableSchemaResolver` already uses. Otherwise the rebuilt schema omits columns the file actually has, and in the validator case that schema goes straight into `spark.read().schema(...).parquet(...)`. **Guards bypassable by setting only the mode.** `AutoRecordKeyGenerationUtils` (auto-generated keys computed then discarded), the `ENABLE_ROW_WRITER` infer function in `DataSourceOptions` (row writer stayed on, silently ignoring `COMBINE_BEFORE_INSERT` — the exact case its own comment warns about), and `BaseDatasetBulkInsertCommitActionExecutor` (picked a partitioner sorting on null `_hoodie_record_key`). **Bootstrap dropped the mode entirely.** `BootstrapExecutorUtils` and `BootstrapExecutor` called `setPopulateMetaFields(...)` without the paired `setMetaFieldsModeFromString(...)` that `HoodieSparkSqlWriter` and `StreamSync` have. **Irreversibility guard was all-or-nothing.** In `BaseHoodieWriteClient#validateAgainstTableProperties`, both sides of `!tableConfig.populateMetaFields() && writeConfig.populateMetaFields()` collapse to `mode == ALL`, so `NONE → COMMIT_TIME_ONLY` and `FILE_NAME_ONLY → COMMIT_TIME_AND_FILE_NAME` slipped through silently. Replaced with `MetaFieldsMode#isWiderThan`, which rejects any transition that adds a populated column (narrowing is still allowed, since later commits simply leave the column null). **Deliberately not changed:** the MoR incremental-relation guards at `MergeOnReadIncrementalRelationV1/V2`. They are stricter than `isCommitTimePopulated()` on purpose — `HoodieAppendHandle` does not yet honor the mode, so relaxing them would convert a loud failure into silent data loss. The remaining ~40 references are semantically identical today (`populateMetaFields() == mode == ALL`) and only misname intent; those go in the deprecation follow-up as `isRecordKeyPopulated()` renames. -- 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]
