nsivabalan commented on PR #19205: URL: https://github.com/apache/hudi/pull/19205#issuecomment-5081737819
Thanks for the review @danny0405 — addressed both points below. ### 1. Deprecate `populate.meta.fields`, use `meta.fields.mode` alone Agreed, and done in [`1656733`](https://github.com/apache/hudi/pull/19205/commits/16567335f5d1). `hoodie.meta.fields.mode` is now the single control. The genuinely confusing part was the resolution itself: `MetaFieldsMode.fromConfig(populateMetaFields, rawMode)` took two independent inputs and short-circuited on the boolean, so `populate.meta.fields=true` silently discarded an explicit mode. That's replaced by `resolve(rawMode, legacyPopulateMetaFields)`: ``` if mode is set -> that mode, whichever way the boolean points else populate=false -> NONE (legacy fallback) else -> ALL ``` The mode always wins; the boolean is consulted only for tables written before the property existed, so backward compat is preserved without the two properties composing. Concretely: - `POPULATE_META_FIELDS` is marked `@Deprecated` / `deprecatedAfter("1.2.0")`, along with `HoodieWriteConfig.Builder#withPopulateMetaFields` and `TableBuilder#setPopulateMetaFields`. - The `populate=true` + non-empty-mode mutual-exclusion check in `HoodieWriteConfig.validate()` is **gone** — the combination is no longer ambiguous, so there is nothing left to reject. - `populateMetaFields()` now derives from the mode (`toLegacyPopulateMetaFields()`, true only for `ALL`), so the ~55 existing call sites observe an answer consistent with the enum. On scope: I deliberately did **not** rewrite those 55 call sites here. That sweep reaches into the Flink and Java clients, well outside this PR's surface, and would bury the behavior change in mechanical churn. Tracking it as a follow-up so this PR stays reviewable. Two latent bugs fell out of removing the two-input resolution: - A selective mode passed to `TableBuilder` *without* an explicit `populateMetaFields` boolean matched neither `Boolean.TRUE.equals(null)` nor `Boolean.FALSE.equals(null)`, so both cross-validation guards were skipped — the mode was persisted and then silently ignored at read time. Gone now that the mode is persisted verbatim and read back directly. - `setMetaFieldsModeFromString` used raw `valueOf`, so a typo in a properties file produced a bare `No enum constant ...`. It now routes through `MetaFieldsMode.parse`, which lists the allowed values. ### 2. Do we need `FILE_NAME_ONLY` / `COMMIT_TIME_AND_FILE_NAME`? These are intentional and I'd like to keep them. Query-side inference works where the engine exposes it — Spark has `input_file_name()` — but the column isn't only for query-side projection. It's for file-level pruning and investigation lookups: going from a record to the file holding it without a full scan, including from engines and tooling that surface no file-name pseudo-column. `HoodieMetadataTableValidator` reads `_hoodie_file_name` directly for exactly this. `COMMIT_TIME_AND_FILE_NAME` is what a table wants when it needs incremental queries *and* that lookup path — the common case for the users who asked for this (#18383). Happy to keep discussing if you think the pruning case is better served another way. Worth noting the asymmetry in when we can change our minds: since this is an open enum, dropping a mode later is a doc change, whereas adding one back post-release is a config compatibility problem. -- 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]
