Refactoring join method in PlanMatchPattern in builder pattern

[barunhalderkolkata]

Description

Refactoring join method in PlanMatchPattern in builder pattern

Related issues, pull requests, and links

Fixes #5976

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Barun Halder.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[barunhalderkolkata]

I am raising the PR which is partially complete. I would like to review my approach before making it a too big as there are several places changes need to be done.

[findepi]

@sopel39 this is for #5976. please chime in.

also cc @kasiafi @martint @hashhar @raunaqmorarka @findepi who work with plan tests

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Barun Halder.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[barunhalderkolkata]

@raunaqmorarka, @lukasz-stec can you please review the changes? And also help me out about cla-signed, I have already mailed signed documents as per the process.

[lukasz-stec]

It would be good to find a way to avoid adding new JoinParamUtil.JoinParamBuilder everywhere.

[lukasz-stec]

adding new JoinParamUtil.JoinParamBuilder( and .build() everywhere seems verbose.
Could you come up with some solution that avoids that?

[barunhalderkolkata]

Hi @lukasz-stec The only way I am thinking of having a JoinMatcherBuilder which will have static method with params joinType & equiCriteria to create object of JoinMatcher, and setter for other attributes.
Like below,

join(JoinMatcherBuilder.buildMatcher(joinType, equiCriteria).filter(filter), leftPlanMatchPattern, rightPlanMatchPattern)

But that will require to change JoinMatcher from immutable to mutable.
Otherwise, avoiding method overloading to use builder pattern will bring code verbosity. Please guide what can be done?

[lukasz-stec]

to remove new JoinParamUtil.JoinParamBuilder you could have a single class, JoinMatcherBuilder

class JoinMatcherBuilder
{
    public static JoinMatcherBuilder join(
      JoinNode.Type joinType,
      List<ExpectedValueProvider<JoinNode.EquiJoinClause>> expectedEquiCriteria,
      PlanMatchPattern left,
      PlanMatchPattern right) {..}

  ...

    PlanMatchPattern build() {...}
}

That still requires build for every current PlanMatchPattern.join method call.
One way to keep it out of tests is to add duplicated method to every method in PlanMatchPattern that takes PlanMatchPattern as param to instead take PlanMatchPatternBuilder (new interface) but that seems too much just for joins.
@sopel39 you're the original issue author. Did you have some ideas on what this should look like when you created the ticket?

[lukasz-stec]

instead of having JoinParamUtil and JoinParamBuilder you could have a single JoinMatcherBuilder that either builds JoinMatcher or PlanMatchPattern

[kasiafi]

@barunhalderkolkata we already have a few examples in PlanMatchPattern where builders are used. See window() method which uses WindowMatcher.Builder for example. It would be good to follow the same approach when adding new builders.

[sopel39]

Fixed in #14542