-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix to unparse the plan with multiple UNION statements into an SQL string #12605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @goldmedal! One minor comment on the error message, but otherwise looks good to me! I should have tested more than 2 UNIONs initially 😅
Co-authored-by: Phillip LeBlanc <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- thank you @goldmedal and @phillipleblanc for the review
Thanks @alamb @phillipleblanc ! |
…ring (apache#12605) * fix unparse multiple UNION statement * enhance the error message Co-authored-by: Phillip LeBlanc <[email protected]> * cargo fmt --------- Co-authored-by: Phillip LeBlanc <[email protected]>
…ring (apache#12605) * fix unparse multiple UNION statement * enhance the error message Co-authored-by: Phillip LeBlanc <[email protected]> * cargo fmt --------- Co-authored-by: Phillip LeBlanc <[email protected]>
…ring (apache#12605) (#37) * fix unparse multiple UNION statement * enhance the error message * cargo fmt --------- Co-authored-by: Jax Liu <[email protected]> Co-authored-by: Phillip LeBlanc <[email protected]>
Which issue does this PR close?
No corrsponding issue.
Rationale for this change
Consider the following SQL:
If a plan contains more than two UNION statements, it cannot be properly unparsed back into an SQL string.
What changes are included in this PR?
Fix the unparsing behavior for UNION.
Are these changes tested?
yes
Are there any user-facing changes?
no