RelNodeVisitCoverageTest for test RelShuttle visit coverage

[xiedeyantu]

This test is just a general approach and not fully implemented. It can help detect whether classes inheriting from RelNode have corresponding visit methods in RelShuttle. However, it introduces a dependency, and I haven't thought of a cleaner implementation approach yet.

[mihaibudiu]

verifies whether

[mihaibudiu]

so what's the result of the test?
The JavaDoc of the base shuttle should probably list the rules for a class to be visited; otherwise we will be arguing forever.

[xiedeyantu]

This is a work in progress. The current logic can obtain the set of all classes that inherit from RelNode, as well as the set of all classes visited in RelShuttle. The current test only compares whether these two sets are equal. We might also need a criterion to determine which classes should be included in the visit and which classes should be excluded.

[xiedeyantu]

This is a minor suggestion for adding tests to #4620. I don't currently plan to implement it myself.

[dssysolyatin]

This test only checks that visit(<RelNodeType> rel) exists, which is a good start. But it doesn’t check whether visit(<RelNodeType> rel) actually works. To test that, each <RelNodeType> object would need to be created manually since they don’t share a common constructor and accept would need to be called on each one to verify that the correct method is invoked

[xiedeyantu]

@dssysolyatin Yes, it does seem challenging to write this test. As an alternative, could we first perform a manual audit to check if all necessary RelNode classes have implemented the  visit  method? If the audit confirms they do, would it be acceptable to merge that PR #4620 first? Because it is indeed a defect.

[xuzifu666]

@dssysolyatin Yes, it does seem challenging to write this test. As an alternative, could we first perform a manual audit to check if all necessary RelNode classes have implemented the  visit  method? If the audit confirms they do, would it be acceptable to merge that PR #4620 first? Because it is indeed a defect.

@dssysolyatin This is indeed a problem. Do you agree with this suggestion?

[dssysolyatin]

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

[xiedeyantu]

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.