feat(core): add context to RelVisitor, ExpressionVisitor and FuncArgVisitor

[nielspardon]

BREAKING CHANGE: adds a new context argument to RelVisitor, ExpressionVisitor and FuncArgVisitor

Preparation for #426

Adds the new context argument as a io.substrait.util.EmptyVisitationContext with value null everywhere.

[vbarua]

Had a chance to take a look, and have some high-level notes.

I'm generally a fan of this, but I do worry that it could introduce a lot of noise for visitations that don't require context.

Your usage in a2f0c8e (#426) doesn't strictly require a context parameter like you're introducing, as you could achieve a similar result by just pushing the context before calls to accept, and popping it after. However, that is more error prone and having it as parameter makes it easier to understand the dataflow and makes it harder for users to not propagate the context because they're forced to.

I'm going to ping the Slack to get additional eyes for this, because it does end up being an fairly intrusive API change and I'd like to hear what others think.

[nielspardon]

I'm generally a fan of this, but I do worry that it could introduce a lot of noise for visitations that don't require context.

I also had this concern while working on this. I first had a new set of visitors that I was shipping alongside the existing once. I then decided to just replace the existing once. There are definitely trade-offs for both options.

[nielspardon]

I'm going to ping the Slack to get additional eyes for this, because it does end up being an fairly intrusive API change and I'd like to hear what others think.

make sense. that's why I tagged it as a breaking change since users of substrait-java might be implementing the current visitor and it might be nice to get informed in the release notes that such an intrusive API change has happened.

[vbarua]

We've had this open for a bit and haven't had any other feedback, so I'm prepared to merge it this week.

My original plan had been to wait for #426 to be ready as well to merge both of them at the same time and minimize API churn, however I think that might open us up to merge conflicts.

Something I think we could do to minimize API churn would be to bring the SubstraitRelNodeConverter.Context class into this PR, just like

public static class Context {
    public static Context newContext() {
      return new Context();
    }
}

and update the SubstraitRelNodeConverter to use it:

public class SubstraitRelNodeConverter
    extends AbstractRelVisitor<RelNode, SubstraitRelNodeConverter.Context, RuntimeException>

This way, we only break the API once with this merge, and then #426 will add the functionality to the Context.

What do you think @nielspardon? If you make a new commit switching that over, I can review it easily and merge it on Friday.

[nielspardon]

What do you think @nielspardon? If you make a new commit switching that over, I can review it easily and merge it on Friday.

Updated the PR accordingly.

[nielspardon]

I pushed a few more of the API changes from including the SubstraitRelNodeConverter.Context in this PR, especially for the unit test class accept() calls which means #426 is now super clean.

[vbarua]

There's still a couple of usages of null where we could use the EmptyVisitationContext instead, but that can be handled as a follow-up change.

For the future, if you could avoid squashing and force pushing PR feedback, it would help with reviewing. Specifically, if you make make changes based on PR feedback their own commit, I can just look at the commit during review. We squash merge into main, so they won't show up in the final history anyway.

[vbarua]

We should update all the usage of null to use the EmptyVisitationContext.INSTANCE you introduced, but we can do that as a follow-up.

[nielspardon]

I created #434 for this

[nielspardon]

For the future, if you could avoid squashing and force pushing PR feedback, it would help with reviewing. Specifically, if you make make changes based on PR feedback their own commit, I can just look at the commit during review. We squash merge into main, so they won't show up in the final history anyway.

sure