feat: consolidate JoinType into a top-level enum #795
Conversation
|
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. |
|
@vbarua it is binary compatible. Any reason to hold back this change? This is a good cleanup IMO. |
|
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. |
|
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. |
|
I'm focusing on substrait-io/substrait-go#128, which is actually somewhat related to my ability to check this.
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. |
|
Thanks, @vbarua . I think the gradual migration approach you're suggesting is less disruptive and should help unblock this PR. |
No description provided.