Repository navigation
Fix bug with unions. - #964
guzman-raphael merged 15 commits into
Conversation
Sorry, something went wrong.
|
Seems the test coverage api we use is experiencing a major outage, tests all pass locally. We can run the github actions when they are back up (outage information). |
Sorry, something went wrong.
guzman-raphael
left a comment
There was a problem hiding this comment.
@jverswijver Nice work on this one man! 🚀
Only small suggestions included but overall fix looks great.
Sorry, something went wrong.
| x = set(zip(*q1.fetch('i', 'j'))) | ||
| y = set(zip(*q2.fetch('i', 'j'))) | ||
| assert_set_equal(x, y) |
There was a problem hiding this comment.
Based on the lessons I learned earlier from the recursive comparison within lists discussion, we can simplify this now to:
| x = set(zip(*q1.fetch('i', 'j'))) | |
| y = set(zip(*q2.fetch('i', 'j'))) | |
| assert_set_equal(x, y) | |
| assert q1.fetch() == q2.fetch() |
Sorry, something went wrong.
There was a problem hiding this comment.
This seems to break the test, I get this error when running it:
(1064, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'UNION (SELECT DISTINCT `i`,`j` FROM `djtest_relational`.`#i_j` WHERE((`j`=2) AND' at line 1")
The other changes work fine.
Sorry, something went wrong.
There was a problem hiding this comment.
@jverswijver Just tagged up on this point and looks like this test should have passed. Must still be something missing with this fix. Let me know once you've pinpointed the problem/solution. Thanks 🤝
Sorry, something went wrong.
Co-authored-by: Raphael Guzman <38401847+guzman-raphael@users.noreply.github.com>
| x = set(zip(*q1.fetch('i', 'j'))) | ||
| y = set(zip(*q2.fetch('i', 'j'))) | ||
| assert_set_equal(x, y) |
There was a problem hiding this comment.
@jverswijver Just tagged up on this point and looks like this test should have passed. Must still be something missing with this fix. Let me know once you've pinpointed the problem/solution. Thanks 🤝
Sorry, something went wrong.
unified string formatting
|
It seems mysql does not support nested unions in 5.7. In 8.0 the issue is semi fixed where it allows left side nested unions, but throws a specific error for unsupported right side nested unions. However, there is a workaround for both versions where wrapping the query and all sub queries in a SELECT * FROM () and then adding an alias allows the query to execute properly. |
Sorry, something went wrong.
|
|
||
| def where_clause(self): | ||
| return '' if not self.restriction else ' WHERE(%s)' % ')AND('.join( | ||
| return '' if not self.restriction else ' WHERE (%s)' % ')AND('.join( |
There was a problem hiding this comment.
we don't really need the extra space but no big deal
Sorry, something went wrong.
guzman-raphael
left a comment
There was a problem hiding this comment.
@jverswijver @A-Baji 🚀 Excellent job guys and great collab! 🚀
It is too bad that MySQL has such nuances in working with unions but glad we were able to workaround them to achieve full union nesting support.
Sorry, something went wrong.
|
Hi there, I am using datajoint version 0.13.7, and so I think this should be fixed based on what I've seen online for the different versions, but I am still getting this error when I try to do a union on more than 2 tables. Any advice? Thank you so much! |
Sorry, something went wrong.
Added handling for when union gets a union object inside its make_sql(), this allows you to do multiple unions.
Took the test from @guzman-raphael comments on the issue linked.
fix #926