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

Nested CTE is not rendered in top level query #50

Closed
matt-psaltis opened this issue Feb 11, 2018 · 9 comments
Closed

Nested CTE is not rendered in top level query #50

matt-psaltis opened this issue Feb 11, 2018 · 9 comments
Milestone

Comments

@matt-psaltis
Copy link

Amazing work on the library, been really enjoying using it for my use case.

My scenario has two CTEs. Due to program flow, I am attaching the first CTE to the second CTE query, expecting that both CTEs would be rendered in the top level query.

Given the following code:

var cte1 = new Query("Table1");
cte1.Select("Column1", "Column2");
cte1.Where("Column2", 1);

var cte2 = new Query("Table2");
cte2.With("cte1", cte1);
cte2.Select("Column3", "Column4");
cte2.Join("cte1", join => join.On("Column1", "Column3"));
cte2.Where("Column4", 2);

var mainQuery = new Query("Table3");
mainQuery.With("cte2", cte2);
mainQuery.Select("*");
mainQuery.From("cte2");

var compiler = new SqlServerCompiler();
var sql = compiler.Compile(mainQuery);

Current behaviour:
Rendered SQL:

WITH [cte2] AS (SELECT [Column3], [Column4] FROM [Table2] INNER JOIN [cte1] ON ([Column1] = [Column3]) WHERE [Column4] = @p0) SELECT * FROM [cte2]

Bindings: [@p0: 1, @p1: 2]

As you can see above, whilst CTE1 is not rendered in the SQL, the bindings do filter up which has the side effect of throwing off the correct parameter values for the query (the query doesn't execute so its a mute point) If I use AddComponent to reattach CTE1 manually to the mainQuery, I end up with 3 bindings as the CTE1 parameter binding is promoted a second time.

Expected behaviour:

WITH [cte1] AS (SELECT [Column1], [Column2] FROM [Table1] WHERE [Column2] = @p0), [cte2] AS (SELECT [Column3], [Column4] FROM [Table2] INNER JOIN [cte1] ON ([Column1] = [Column3]) WHERE [Column4] = @p1) SELECT * FROM [cte2]

Bindings: [@p0: 1, @p1: 2]
@ahmad-moussawi
Copy link
Contributor

Hi thanks for your feedback, actually all CTEs should be attached to the main query and not to any other CTE or subquery.

so you should rewrite your programs as the following:

var cte1 = new Query("Countries").Where("Population", ">", 100);
var cte2 = new Query("Cities").Join("Cte1", j => j.On("Cte1.Id", "Cities.CountryId"));

var main = new Query("Users").With("Cte1", cte1).With("Cte2", cte2).From("Cte2")

let me know if this work.

@matt-psaltis
Copy link
Author

Yeah that method does work. With the way I've structured my solution, each query only has access to things they are directly related to. CTE2 has a reference to CTE1. Main only has a reference to CTE2.

Would you take a pull request for having the CTEs pulled from sub queries up to the main query during compilation?

@ahmad-moussawi
Copy link
Contributor

I think pulling the CTEs to the top level automatically may lead to confusion in certain cases, for example when a CTE is referenced by more than one query, this may lead to adding the same CTE more than once in the main query.

one example would be:

var cte1 = new Query("table1");
var cte2 = new Query("table2").Join("Cte1", "Cte1.Col", "table2.col");
var cte3 = new Query("table3").Join("Cte1", "Cte1.Col", "table3.col");
var main = new Query().With("Cte2", cte2).With("Cte3", cte3); // adds cte1 twice

Maybe leaving this to the developer is more appropriate, anyway we can keep the discussion open and get more feedback.

for the moment we can enhance the usage examples in the docs, if you find so please open an issue there.

@matt-psaltis
Copy link
Author

I understand where you're coming from. However, the current behaviour has already led to developer confusion because I added a CTE in the query where the CTE is actually referenced and this wasn't rendered in the subsequent SQL.

Maybe this needs to throw an exception when SqlKata detects a nested CTE? In terms of the example above, that's not quite what I had in mind and I agree that example would lead to developer confusion.

Maybe it was just an omission but cte1 was not added to cte2 & cte3 using the With syntax.

What this might look like:

var cte1 = new Query("table1");
var cte2 = new Query("table2").With("Cte1", cte1).Join("Cte1", "Cte1.Col", "table2.col");
var cte3 = new Query("table3").With("Cte1", cte1).Join("Cte1", "Cte1.Col", "table3.col");
var main = new Query().With("Cte2", cte2).With("Cte3", cte3);

To avoid the duplication aspects, Cte1 would be added once due to an Object reference equality check.

As you say, maybe it is more appropriate to leave it to the developer, the current behaviour allows SqlKata to produce unexpected and broken SQL.

Anyway, just a suggestion, thanks for taking the time to reply.

@ahmad-moussawi
Copy link
Contributor

@SaltyDH, I will reconsider this, I am convinced with your approach more honestly, do you have a PR for this feature ?

@ahmad-moussawi ahmad-moussawi added this to the version-1.1 milestone Jul 15, 2018
@IssueHuntBot
Copy link

@BoostIO funded this issue with $20. Visit this issue on Issuehunt

@IssueHuntBot
Copy link

@edokan has started working. Visit this issue on Issuehunt

@IssueHuntBot
Copy link

@edokan has submitted output. Visit this issue on Issuehunt

ahmad-moussawi added a commit that referenced this issue Sep 27, 2018
Fixes #50 Nested CTE is not rendered in top level query
@IssueHuntBot
Copy link

@ahmad-moussawi has rewarded. Visit this issue on Issuehunt

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

No branches or pull requests

3 participants