marijnh / Postmodern

A Common Lisp PostgreSQL programming interface
http://marijnhaverbeke.nl/postmodern
Other
400 stars 90 forks source link

Updated :union & :union-all to wrap their subqueries in parens. #337

Closed peoplestom closed 9 months ago

peoplestom commented 10 months ago

When doing a union over queries that use a with clause the with clauses aren't inside parens when they need to be for valid syntax.

e.g.

(:union
   (:with (:as 'cte1 (:parens (:select '* :from 'demo)))
      (:select '*
        :from 'base
        :inner-join 'cte1
        :on t))
   (:with (:as 'cte2 (:parens (:select '* :from 'example)))
      (:select '*
        :from 'level
        :left-join 'cte2
        :on t))))

without this PR produces:

(WITH cte1 AS  ((SELECT * FROM demo))
    (SELECT *
     FROM base 
     INNER JOIN cte1 
     ON true) 
union 
WITH cte2 AS  ((SELECT * FROM example))
    (SELECT * 
     FROM level 
     LEFT JOIN cte2
     ON true))

which fails.

with this PR it produces:

(( WITH cte1 AS  ((SELECT * FROM demo))
    (SELECT *
     FROM base
     INNER JOIN cte1
     ON true) ) 
union
( WITH cte2 AS  ((SELECT * FROM example))
    (SELECT *
     FROM level
     LEFT JOIN cte2
     ON true) ))
peoplestom commented 10 months ago

I initially wrapped all :with queries in an extra set of parens, but that gave me errors in insert & delete queries. This time I've only updated the way we expand union and union-all subqueries so they are always wrapped in parens.

sabracrolleton commented 10 months ago

Fails on:

(:union-all
 (:with (:as 't1 (:select (:as (:random) 'x)
                          :from (:generate-series 1 3)))
        (:select '* :from 't1))
 (:select '* :from 't1))

with an undefined table error.

peoplestom commented 10 months ago

That does fail, but I thought that would be expected behavior.

If a CTE was intended to be available to all union sub-queries I would expect it needs to be written like this:

(:with (:as 't1 (:select (:as (:random) 'x)
               :from (:generate-series 1 3)))
        (:as 't2 (:select (:as (:random) 'x)
               :from (:generate-series 10 30)))
        (:union-all
         (:select '* :from 't1)
         (:select '* :from 't2)))

That works with and with out this PR.

Currently if we have a :with clause any where but the first position in a union the query is invalid. For example both of the following currently produce invalid queries without this PR.

(:union-all
 (:with (:as 't1 (:select (:as (:random) 'x)
                          :from (:generate-series 1 3)))
        (:select '* :from 't1))
 (:with (:as 't2 (:select (:as (:random) 'x)
                          :from (:generate-series 10 30)))
        (:select '* :from 't2)))

(:union-all
 (:select (:as (:random) 'x)
                          :from (:generate-series 1 3))
 (:with (:as 't2 (:select (:as (:random) 'x)
                          :from (:generate-series 10 30)))
        (:select '* :from 't2)))

I'm not sure how to leave existing code written as in your example working, while allowing each union subquery to have it's own CTE aswell?

sabracrolleton commented 10 months ago

Your rewrite is not the same query as my example. My example is pulled from https://www.postgresql.org/docs/current/sql-select.html

WITH t AS (
    SELECT random() as x FROM generate_series(1, 3)
  )
SELECT * FROM t
UNION ALL
SELECT * FROM t

How would you rewrite my query to get this?

peoplestom commented 9 months ago
(:with (:as 't1 (:select (:as (:random) 'x)
               :from (:generate-series 1 3)))
        (:union-all
         (:select '* :from 't1)
         (:select '* :from 't1)))

This would be my s-sql rewrite of that SQL. It produces a valid query with and without this PR.

sabracrolleton commented 9 months ago

I will adjust the tests.