substrait feat: consolidate JoinType into a top-level enum

Mar 19 '25 20:03 vbarua

I noticed that we had 4 different definitions of the same enum, and thought it would be better to factor it out. I believe this is binary compatible, though I want to confirm that.

Mar 19 '25 20:03 vbarua

@vbarua it is binary compatible. Any reason to hold back this change? This is a good cleanup IMO.

Apr 13 '25 21:04 yongchul

Thanks, @vbarua . Is there any remaining work or anything we can help with to get this PR merged? Any concerns about merging it? While this is a backwards-incompatible change, it'd be great to get this consolidation in place soon.

Apr 13 '25 21:04 jcamachor

Oh... I didn't realize that the enum values actually don't match! :( As @jcamachor said, I still think this is a necessary breaking change to keep things in consistent state.

Apr 13 '25 23:04 yongchul

I'm focusing on https://github.com/substrait-io/substrait-go/issues/128, which is actually somewhat related to my ability to check this.

didn't realize that the enum values actually don't match!

The other way to do this would to via slow migration, adding second field joint type field alongside the original ones (which we deprecate), have them co-exist for a number of versions, and then remove the old fields.

Apr 14 '25 20:04 vbarua

Thanks, @vbarua . I think the gradual migration approach you're suggesting is less disruptive and should help unblock this PR.

Apr 15 '25 02:04 jcamachor