DanielLeens commented on issue #11052: URL: https://github.com/apache/seatunnel/issues/11052#issuecomment-4671704787
Thanks for writing this up so concretely. I agree this is a larger API-cleanup / compatibility topic, not just a local refactor. I would not remove `Expression` directly in the first PR. My preference is Option 1: deprecate first, migrate in-repo usages, and keep compatibility shims for one release cycle. The main reason is that `Expression` is still a public `seatunnel-api` type today, and it is not only an internal wrapper. `RequiredOption.ConditionalRequiredOptions` still stores an `Expression`, and `ConfigValidator` still validates through `Expression`. Even if all in-repo call sites are migrated in one PR, deleting it immediately would still be a source-level breaking change for downstream extensions that may already import it. My suggested scope is: 1. keep `Condition<?>` as the long-term single model 2. migrate the internal evaluation / rendering path toward `Condition<?>` 3. keep deprecated `Expression` entrypoints / constructors / accessors as forwarding compatibility shims for now 4. keep behavior and error-message rendering unchanged in this step It is fine to update the `seatunnel-engine` and `seatunnel-core` call paths in the same PR if the observable behavior stays identical. What I would avoid in the first change is removing all `Expression`-typed APIs from `ConfigValidator` and `RequiredOption` at once. So from my side: yes to the cleanup direction, but deprecate-first is the safer path here. -- 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]
