dssysolyatin commented on PR #4641: URL: https://github.com/apache/calcite/pull/4641#issuecomment-3557223692
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 https://github.com/apache/calcite/pull/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 -- 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]
