search

substrait-io / substrait-validator

Apache License 2.0
8 stars 8 forks source link

feat: Add validate function for a `substrait::Plan` message #281

Closed wackywendell closed 4 weeks ago

wackywendell commented 1 month ago

Also refactors the parse / validation layer a bit to separate them a bit more.

Addresses #280.

I'm still fairly new to this code / repo, so I took a bit of time to see if I could address #280 in the "naive" way. Naive solutions are often as good as any... but sometimes, they are naive, so let me know if there's something I'm missing or if this is something we should avoid exposing!

wackywendell commented 1 month ago

Sorry for the force-push - I usually try not to do that, but as no one had reviewed yet and the CI was failing on commit messages, I figured I'd fix those.

Next time I'll plan to make a Draft PR first, get it passing CI, then mark it for review.

wackywendell commented 4 weeks ago

@mbrobbel - thanks! Is this ready to merge, then? If so, can you do that - I don't have permissions?

mbrobbel commented 4 weeks ago

@mbrobbel - thanks! Is this ready to merge, then? If so, can you do that - I don't have permissions?

Let's get #282 in first, then rebase/merge here to fix CI.

wackywendell commented 4 weeks ago

@mbrobbel - thanks for approving and merging #282!

I can either rebase this on main (post #282), then force-push, which will be "cleaner" git history with the downside of making reviews harder to follow, or I can merge in main. Any preferences?

mbrobbel commented 4 weeks ago

@mbrobbel - thanks for approving and merging #282!

I can either rebase this on main (post #282), then force-push, which will be "cleaner" git history with the downside of making reviews harder to follow, or I can merge in main. Any preferences?

It doesn't really matter because we squash merge in the substrait-io org. This will be one commit on main.

wackywendell commented 4 weeks ago

OK, main merged in, let's see how CI goes... ⏳

wackywendell commented 4 weeks ago

And CI passed! 🎉