feat: Implement UNION ALL BY NAME #14538
Conversation
github-actions
bot
added
sql
| NULL 3 | ||
| 5 NULL | ||
|
|
||
| # TODO: This should pass, but the sanity checker isn't allowing it. |
There was a problem hiding this comment.
A few of the UNION ALL BY NAME tests are failing during the sanity checker. Not completely sure why atm 🤔
| @@ -0,0 +1,264 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Most tests here were taken from DuckDB's SLT suite!
There was a problem hiding this comment.
DuckDB to the best of my knowledge is NOT Apache licensed.. Any code or tests taken from them should be licensed appropriately.
There was a problem hiding this comment.
Thank you for catching that @Omega359. Can I include the notice there in conjunction with this header?
There was a problem hiding this comment.
If there is anything to be done about this, let's do it before the merge
There was a problem hiding this comment.
I think having both should be ok @rkrishn7
|
FYI @berkaysynnada and @berkaysynnada |
|
I ran the sqlite sqllogictests against your branch and it passed so none of those files covered union (all) by name |
|
I'll take a look ASAP, but any additional reviewers are welcomed |
There was a problem hiding this comment.
Thank you @rkrishn7, I really like this PR. I'm just curios about why the SanityChecker does not allow those queries. I guess it should be related with equivalence reset during UnionExec's (perhaps we should sustain some info under some conditions). However, it's not a problem coming along with this PR, and I think it shouldn't be a blocker.
sorry @Omega359, I couldn't quite get what you mean. Do we expect a failure? |
Thanks for the review @berkaysynnada! I was thinking I can open up an issue to fix the failing tests due to the sanity checker, and leave that as a follow-up once this is merged. Let me know if that sounds good to you. |
No, it just means that this union isn't covered at all with sqlite tests. |
Sounds great, let's do that |
Which issue does this PR close?
What changes are included in this PR?
UNION [ALL | DISTINCT] BY NAME implementation and associated tests
Are these changes tested?
Yes
Are there any user-facing changes?
Yes, no breaking changes, but new functionality.