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

Add with_cte_expression #224

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

macobo
Copy link

@macobo macobo commented Feb 10, 2025

ClickHouse supports two different sorts of CTEs:

  1. Normal, subquery based CTEs supported by most DBMS
  2. Expression-based CTEs

This PR adds support for (2) by adding a new adapter-specific method. Note the SQL syntax is flipped for (2).

Implementation is a hack - let me know if there are better solutions!

ClickHouse supports two different sorts of CTEs:
1. Normal, subquery based CTEs supported by most DBMS
2. Expression-based CTEs

This PR adds support for (2) by adding a new adapter-specific method.
Note the SQL syntax is flipped for (2).
@macobo macobo force-pushed the with-cte-expression branch from 286bbdc to f82412a Compare February 10, 2025 08:20
@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 10, 2025

👋 @macobo

If it's time-sensitive, I am happy to merge it but maybe as undocumented function/macro to avoid anyone outside of Plausible using it until we verify there is no other way to support this type of CTEs.

@macobo
Copy link
Author

macobo commented Feb 10, 2025

Not in a rush right now - I thought this might simplify a task I'm working on, but I might be taking another approach.

@macobo
Copy link
Author

macobo commented Feb 10, 2025

Small note for if you do end up reaching out to ecto developers: Keeping opts passed to with_cte in the same format, not validating for extra keys and making sure they get passed to connection.ex#cte would be ideal from an extensibility point of view. This would allow something like:

q
|> with_cte(fragment("123"), as: "foo", expression: true)

And the adapter can take care of the rest

@ruslandoga
Copy link
Contributor

ruslandoga commented Feb 11, 2025

Definitely! But I don't think this alone would be enough for this example to work, since Ecto would try to ensure that "foo" is a valid CTE and fragment("123") is a valid CTE name and right now it fails at compile time before the adapter receives anything. with_cte("foo", as: fragment("123"), expression: true) would probably work however.

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.

2 participants