rahil-c commented on PR #19110: URL: https://github.com/apache/hudi/pull/19110#issuecomment-4997994878
Thanks for the thorough review, @nsivabalan! Summary of the changes addressing the feedback (latest commit `58f39b5`). ### 🚨 `nestedDebeziumMetadataEnabled` boolean / new `HoodieTableConfig` overloads Removed. There's no boolean and no new overloads — `inferMergingConfigsForWrites` / `inferMergingConfigsForV9TableCreation` are back to their original signatures (unchanged vs. master) and `maybeNestColumn` is gone. One deviation from the literal suggestion (resolve entirely in `HoodieStreamer`) that I'd like your read on: not every Debezium write path goes through the streamer. The Spark DataFrame/SQL and Flink **table-creation** paths reach `HoodieTableConfig` through `HoodieTableMetaClient.init` directly and rely on `handlePayloadAdhocConfigs` auto-correcting the ordering field to the payload's canonical column(s) — `TestPayloadDeprecationFlow#testMergerBuiltinPayloadFromTableCreationPath` exercises exactly that create path, and merge correctness depends on it. Resolving only in `HoodieStreamer` regresses those paths. So instead of a layout flag, `handlePayloadAdhocConfigs` now reconciles using the **existing** `orderingFieldName` argument it already receives: it forces the flat canonical ordering (`_event_lsn`, or `_event_bin_file,_event_pos`) unless the caller already resolved the nested form — which only the Streamer does — in which case the nested form is preserved. The column knowledge lives in `DebeziumConstants.resolveOrderingFields(payloadClass, nested)`, so the generic inference stays layout-agnostic. `HoodieStreamer` resolves the (flat or nested) ordering into `cfg.sourceOrderingFields`, so a nested table persists the nested path at creation. This also fixes a gap where the nested ordering field wasn't actually reaching table creation. Happy to iterate if you'd prefer a different split. ### ⚠️ Postgres nested-metadata default → `false` Flipped for cross-source consistency with MySQL + backward compatibility. Removed the "defaults to true" wording from `DebeziumTransformerConfig` and the `PostgresDebeziumTransformer` Javadoc. ### ⚠️ Visibility of the new overloads Moot — the overloads were removed. ### 💬 Suggestions - `SNAPSHOT_OP = "r"` moved into `DebeziumConstants`. - Class Javadoc on `AbstractDebeziumTransformer` now references `DebeziumTransformerConfig`. - `apply()` split into named phases (`selectBeforeOrAfterImage` / `buildOutputColumns` / `applyErrorTablePassthrough` / `applyNullabilityRules`) — pure private-method extraction, no abstract methods added. - `hasSchemaField` simplified via a `getSourceStruct` helper. - Added the resolution-order note on the constructor. - Kept the `col.toString()` LSN detection as-is, per your note. ### 💅 Nits - Fixed the `.s` typo in `HoodieTableConfig`. - The Javadoc link was already `https://`. - Left the `ERROR_TABLE_CURRUPT_RECORD_COL_NAME` typo for a separate cleanup (it lives in `BaseErrorTableWriter`). ### Tests - Added: Postgres snapshot-LSN defaulting under the nested layout; MySQL `_event_seq` when the binlog `file`/`pos` are null (propagates null rather than fabricating a sequence). - Reworked `TestHoodieTableConfigDebeziumOrdering` to cover the reconcile: flat by default, nested preserved when the caller resolved it, Postgres always `_event_lsn`. ### Oracle / SQLServer Oracle transformer and the chain/type-conversion transformers are planned as separate follow-up patches; SQLServer is not being upstreamed. -- 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]
