-
Notifications
You must be signed in to change notification settings - Fork 608
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
perf(api): rewrite union and intersection construction to support more operands #9194
Conversation
So currently both the ibis and sqlglot representations are deeply nested using pairwise union operations. We could change it on the ibis side to be variadic but we would still hit the recursion error in the sqlglot implementation, but at least the error wouldn't come from our side. |
The updated implementation generates SQL in a manner that reduces the depth of nesting, thereby delaying the hitting of the recursion error in the SQLglot implementation. See the generated sql query fabove. |
my current unit test uses |
3e6d064
to
cfbd7e5
Compare
Ok, so the new compile benchmark simply cannot be run on After this PR, the benchmarks look like this:
Merging! |
Description of changes
When I am working on the #9139 (comment), I had a
RecursionError: maximum recursion depth exceeded while calling a Python object
when union large number of tables, I found we had this similar issues before, #7124 and tobymao/sqlglot#2961.I am trying to rewrite the
union
in our codebase, It recursively union the tables using a queue, the number of unions is still similar to the original implementation, but it allow us to union more tables than before (It could unblock #9139 and added a test for larger number of tables where the original implementation will fail). Not sure if there is other side effect or not. If it is not a good solution, I could close this PR. Thanks for your review.I was thinking to open an issue for discussion first, since I had some code ready, just open this for discussion, if it is not the best practice to file a PR, please let me know.
Update from the below comment
sql query: Union 7 tables in the new implementation
SQL query for the same union from the original implementation
Issues closed