SQLProvider icon indicating copy to clipboard operation
SQLProvider copied to clipboard

Invalid SQL produced when using sortBy after nested query.

Open nikonthethird opened this issue 8 years ago • 3 comments
trafficstars

Description

The type provider produces invalid SQL (in my case I used SQLite) when using a sortBy or sortByDescending after a nested query.

Repro steps

I have added a repro here.

  1. Create a SQLite file with the following SQL statements:
create table `Models` (
    `Id` integer not null,
    `Name` text not null,
    primary key (`Id`)
);

create table `Sets` (
    `Id` integer not null,
    `Name` text not null,
    primary key (`Id`)
);

create table `SetModels` (
    `SetId` integer not null,
    `ModelId` integer not null,
    primary key (`SetId`, `ModelId`),
    foreign key (`SetId`) references `Sets` (`Id`),
    foreign key (`ModelId`) references `Models` (`Id`)
);
  1. Run the following query:
query {
    for set in (query {
        for setModel in db.Main.SetModels do // SetModels is the mapping table between Sets and Models.
        for set in setModel.``main.Sets by Id`` do
        select set
    }) do
    sortBy set.Id
}

Expected behavior

The query should work.

Actual behavior

The query fails with an SQLiteException and the message: SQL logic error or missing database.

The generated SQL looks like this:

SELECT [arg2].[Id] as '[arg2].[Id]',[arg2].[Name] as '[arg2].[Name]'
FROM main.SetModels as [arg1]
INNER JOIN  [main].[Sets] as [arg2] on [arg1].[SetId] = [arg2].[Id] 
ORDER BY [set].[Id]

The problem is in the ORDER BY clause. Instead of [set].[Id] it should say [arg2].[Id]. It seems the alias of the inner query is being ignored in the outer query.

Known workarounds

By not using a nested query, the correct SQL is generated:

let setQuery = query {
    for setModel in db.Main.SetModels do // SetModels is the mapping table between Sets and Models.
    for set in setModel.``main.Sets by Id`` do
    sortBy set.Id
    select set
}

Now the query looks like this:

SELECT [arg2].[Id] as '[arg2].[Id]',[arg2].[Name] as '[arg2].[Name]'
FROM main.SetModels as [arg1]
INNER JOIN  [main].[Sets] as [arg2] on [arg1].[SetId] = [arg2].[Id]
ORDER BY [arg2].[Id]

Related information

I have added everything into this Github repo.

  • Windows 10 Creators Update
  • .NET 4 to 4.6

nikonthethird avatar May 07 '17 16:05 nikonthethird

Nice finding. This is not the case if your base-query is selection from one table only, but it happens if you do join like that.

I think the whole concept should be thought more: Should we generate nested SQL-clauses like EntityFramework does: SELECT x FROM (SELECT ... ) and when that is needed and when not. Currently we just merge the query as one SQL-clause and assume that it's enough, when in simple cases it is, but with complex SQL it can produce wrong result (#391).

On the other hand EF is a tool that causes database administrators nightmares with it's complex 10-nested-SQL-clauses structures. So I'm not in favour of nest-every-query. E.g: Could we count the projection / select clauses, and if there are many, then generate a sub query...? Or if join is done to SQLQueryable, then nesting. Sorry, this went bit off-topic.

Thorium avatar May 07 '17 20:05 Thorium

This is actually quite interesting... SqlExp could model a nested tree structure which would be just tuple of nested and rest-of-current-tree plus an nesting alias. Also, generation of current query-sql-clause method in each provider could be made recursive from the fromBuilder to call a sub-query if needed.

But the real question is how could we know when we need a nested query?

-- can be merged, no need to nest:
SELECT A, B FROM ((SELECT A, B, C FROM TBL) AS N)

But then if the inner query it's having any aggregation like distinct, top, group-by, ... and the outer query is having a join, then it would be dangerous to merge queries. But the LINQ-expression-tree can be in a different order than SQL-commands, so how to know which SqlQueryable<T> belongs to where...

Thorium avatar May 12 '17 15:05 Thorium

sortBy nesting is now working in the latest version, but it's still far from perfect and it's better to avoid nesting...

Thorium avatar Nov 07 '17 20:11 Thorium