Skip to content
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

Alias CTEs upon initialising #373

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RikvanToor
Copy link

@RikvanToor RikvanToor commented Aug 14, 2023

Hello. :)

This PR fixes #372 by adding a fresh alias whenever a common table expression is used within a query.

As described in the issue, previously this Haskell code

share [mkPersist sqlSettings] [persistLowerCase|
  A
    k Int
    v Int
    Primary k
  
  B
    k Int
    v Int
    Primary k
|]

q :: SqlQuery (SqlExpr (Value Int), SqlExpr (Value Int))
q = do
  bCte <- with $ do
    b <- from $ table @B
    return b
  
  a :& b1 :& b2 <- from $ table @A
    `innerJoin` bCte
    `on` (\(a :& b) -> a.k ==. b.k)
    `innerJoin` bCte
    `on` (\(a :& _ :& b2) -> a.k ==. b2.k)
  return (a.k, a.v +. b1.v +. b2.v)

would generated this SQL query

WITH `cte` AS (
  SELECT `b`.`k` AS `v_k`, `b`.`v` AS `v_v`
  FROM `b`
)

SELECT `a`.`k`,  ((`a`.`v` + `cte`.`v_v`) + `cte`.`v_v`)
FROM `a`
INNER JOIN `cte`
  ON `a`.`k` = `cte`.`v_k`
INNER JOIN `cte`
  ON `a`.`k` = `cte`.`v_k`;

which is invalid. After this PR, it instead generates this SQL query:

WITH `cte` AS (
  SELECT `b`.`k` AS `v_k`, `b`.`v` AS `v_v`
  FROM `b`
)

SELECT `a`.`k`,  ((`a`.`v` + `cte2`.`v_v`) + `cte3`.`v_v`)
FROM `a`
INNER JOIN `cte` AS `cte2`
  ON `a`.`k` = `cte2`.`v_k`
INNER JOIN `cte` AS `cte3`
  ON `a`.`k` = `cte3`.`v_k`

I'm not 100% satisfied with this, as the aliasing is only required when a single CTE is used multiple times in a single query, but my PR applies the aliasing at all times. I was not sure how to check for such a situation from within the context of the with function, so perhaps this PR can serve as a conversation about that. Thanks a lot!


After submitting your PR:

  • Update the Changelog.md file with a link to your PR.
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

@belevy
Copy link
Collaborator

belevy commented Aug 17, 2023

please add a test that would fail to work without this fix. As far as aliasing only on duplicates, how do we handle it with regular table joins?

@jonathanmoregard
Copy link

please add a test that would fail to work without this fix. As far as aliasing only on duplicates, how do we handle it with regular table joins?

See #372.

I'm pretty sure I've run into the same issue in our production app :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joining on a CTE multiple times creates name clashes
3 participants