Fix: Don't emit multiple parens for multiple union alls

kbrock commented 6 years ago

This emits SQL without adding extra parens to multiple UNION ALL statements in a row.

Fixes #58 Fixes #158 Fixes #299 Fixes #341 Fixes #404 Fixes #418

Before:

The MySQL adapter overrides the Union visitor but not the UnionAll one. What will come as no surprise, Arel generates the correct UNION statement for Mysql. Unfortunately, UNION ALL on MySql and UNION for the other databases are incorrect.

After:

Arel generates the correct UNION ALL and UNION statements for all databases. (Correct meaning without extra parentheses.)

In order to know that parens have already been generated, it calls the visitor method directly if the child class is the same. (This will be the Union or UnionAll nodes). It passes in a parameter to omit the unwanted parenthesis.

I like not changing the method signature of the visit_* method but do not like skipping the call into the visitor infrastructure. But since this node is the same class, it is known to support this method.

UPDATE: The spacing around the parenthesis could be removed, and produces prettier SQL. Let me know if you want me to do this here or submit a followup. It changes a dozen other tests.

NOTE:

WARNING: I played with the tests, but not run in the DB itself.
@bughit could you test this PR to see if it fixes your issue? Thnx
@matthewd Hi :)

rails-bot commented 6 years ago

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

kbrock commented 6 years ago

@tenderlove I don't think rails uses union.

I added a print "Y" into the modified method and ran tests for active_record postgres. Got no output.
grep didn't show me much either.

Just let me know how to proceed. Thnx for review

matthewd commented 6 years ago

Per #523, Arel development is moving to rails/rails.

If this PR is still relevant, please consider reopening it over there.

rails / arel