xiedeyantu commented on PR #4641: URL: https://github.com/apache/calcite/pull/4641#issuecomment-3557277755
> It’s fine with me if we just check that `visit(<RelNodeType> relNode)` exists in `RelShuttle` using this test, and then manually check the `accept` method. But ideally for each of these checks, we still need to write a test manually, just like you did for `LogicalSnapshot`. > > But before checking, it’s still necessary to figure out which node types `RelShuttle` should support. > > > If the audit confirms they do, would it be acceptable to merge that PR #4620 first? > > I would prefer to avoid breaking changes across multiple releases. If we merge `LogicalSnapshot` first, there’s a good chance that future releases will introduce more breaking changes on this topic, since the discussion and implementation around `RelShuttle` could take another five years. Personally, I would go with an “all or nothing” approach rather than leaving it in an intermediate state @dssysolyatin Thank you for your reply and explanation. I will temporarily close this test PR for now, as this test doesn't seem very practical to implement. Even if we were to create a comprehensive test, its sole purpose would be to remind contributors not to forget the visit method, which seems like a disproportionate effort for the benefit gained. So I'll close this PR for now and wait to see if someone can propose a better solution in the future. -- 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]
