bvarghese1 commented on PR #28308: URL: https://github.com/apache/flink/pull/28308#issuecomment-4633931537
> I agree the file's readability isn't great. We've now split the logic for update, modify, and delete across three files. When working in `FlinkChangelogModeInferenceProgram` I usually jump to one node and want all of its logic in one place, so I'd suggest other paths: > > 1. Convert to Java - definitely +1. > 2. Move each node's logic into a registry (`Map<Class, NodeHandler>` or visitor-per-node). This keeps a node's logic together across traits and drops the `instanceof` ladder entirely, so "what does node X do" becomes a single lookup. > 3. Give repeated branches a name - many nodes just forward or do full-delete-if-updates. > 4. Name specific branches for readability, e.g. `visitWithFallback(child, preferred, fallback)` instead of `Optional.or(() -> visit(child, fallback))`. > > Another option is splitting the `instanceof` ladder into one small private method per node (`visitSink`, `visitJoin`, `visitCalc`), so the top-level `visit()` becomes a short dispatch and each handler reads on its own. We already use this pattern in `StreamNonDeterministicUpdatePlanVisitor`. I lean toward the registry first, but glad to hear other opinions Hi @gustavodemorais , yes that's definitely the plan :-) . But I want to do this refactoring in 2 phases. The first PR simply converts to Java with a few newly introduced classes like in this PR. The second PR will move each node into a registry (this is more involved and I did not want to complicate this initial PR). -- 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]
