davidzollo commented on PR #11099: URL: https://github.com/apache/seatunnel/pull/11099#issuecomment-4720349439
Thanks for working on this. The direction makes sense, and the `ALL_MATCH` flow itself looks useful. I still see two things that should be addressed before merge: 1. The default path is no longer backward compatible for duplicate exact `table_path` rules. Before this change, repeated exact `table_path` entries would fail during config resolution because they collapsed into a map with duplicate keys. After this PR, the default `FIRST_MATCH` path silently keeps the first rule and ignores the rest in `AbstractMultiCatalogTransform`. That means an existing invalid config now starts successfully but behaves differently, which conflicts with the compatibility requirement described in #11045. I think we should keep the legacy fail-fast behavior unless the user explicitly opts into repeated exact-table rules with the new mode, instead of silently turning duplicates into "first one wins" by default. 2. The new chained schema-change helpers should preserve the existing `null` short-circuit contract. `TransformFlowLifeCycle` treats `null` from `mapSchemaChangeEvent()` as "this event is filtered, stop propagating it". However, `ChainedMapTransform` and `ChainedFlatMapTransform` continue iterating even if one inner transform returns `null`, so later transforms can still receive a filtered event. It would be safer to short-circuit immediately when `currentEvent == null` and return `null` from the chained helper as well. Non-blocking suggestion: - It would be great to add one focused schema-change test for the chained helper path after the fix, since the current new coverage is strong for row processing but does not yet exercise the chained `mapSchemaChangeEvent()` behavior. Thanks again — the overall idea looks good, but I think the two items above are worth tightening before this goes in. -- 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]
