Distinct() does not insert DISTINCT keyword when generating SQL. (SubSonic 2.2, Oracle 9.2)

[FrostyZoob]

I switched from using the SubSonic.dll included with SubSonic2.2.zip found on http://www.subsonicproject.com to using my own copy generated from compiling the source from GitHub. Since I switched over, none of my queries calling the Distinct() method were returning the expected results. Example:

SqlQuery sql = new Select(Widgets.Columns.WidgetType)
                   .From(Widgets.Schema)
                   .OrderAsc(Widgets.Columns.WidgetType)
                   .Distinct();

I started debugging and I found the following

From ANSISqlGenerator.GenerateCommandLine(): public virtual string GenerateCommandLine() { StringBuilder sb = new StringBuilder();

        //start with the SqlCommand - SELECT, UPDATE, INSERT, DELETE
        sb.Append(query.SQLCommand);
        string columnList;
        if(query.Aggregates.Count > 0)
            columnList = BuildAggregateCommands();
        else
        {
            if (query.IsDistinct)
                sb.Append(query.DistinctSpec);
            //set "TOP"
            sb.Append(query.TopSpec);

            //decide the columns

Stepping through the code shows that query.DistinctSpec returns an empty string.

Now, keep in mind: This used to work. I was under the impression that the source found on here (GitHub) was from the 2.2 build of SubSonic (despite being labeled "2.0"). Is that not the case?

NOTE: Maybe I should make this another issue, but I'll mention it here: I noticed that the DLL generated from GitHub source is ~8k smaller than the SubSonic.dll bundled with SubSonic2.2.zip

Doing some more digging, I managed to find the source for ANSISqlGenerator.cs on Google Code (Here: http://subsonicproject.googlecode.com/svn/branches/Release_2.2/SubSonic/SqlQuery/SqlGenerators/). (This time being explicitly labeled as belonging to the 2.2 branch.) This is what I found:

    public virtual string GenerateCommandLine()
    {
        StringBuilder sb = new StringBuilder();

        //start with the SqlCommand - SELECT, UPDATE, INSERT, DELETE
        sb.Append(query.SQLCommand);
        string columnList;
        if(query.Aggregates.Count > 0)
            columnList = BuildAggregateCommands();
        else
        {
            if (query.IsDistinct)
                sb.Append(SqlFragment.DISTINCT);
            //set "TOP"
            sb.Append(query.TopSpec);

            // DISTINCT"
            sb.Append(query.DistinctSpec); <<Specific to 2.2 codebase

            //decide the columns

So it appears that the 2.2 code works because it uses the SqlFragment.DISTINCT constant. Whats very interesting is that a few lines later, query.DistinctSpec is appended! Looks like query.DistinctSpec has always been broken! (If it had worked, every SELECT query generated would always have had a DISTINCT keyword injected! Ack!)

Anyone have thoughts on a fix? Personally, since Select.Distinct() assigns a value to query.DistinctSpec, I'm inclined to have it override it's base class implementation (which is what currently gets called.)

WORKAROUND:

Casting a SqlQuery back to Select works:

SqlQuery sql = new Select(Widgets.Columns.WidgetType)
                   .From(Widgets.Schema)
                   .OrderAsc(Widgets.Columns.WidgetType);

((Select)sql).Distinct();

[6pac]

Hi Mike

I found exactly this problem with one of the unit tests which has exactly the form you speak of. I also traced the spaghetti, and found that it's because the .Select method returns a 'Select' object whereas the subsequent .From and .OrderAsc methods return an SqlQuery object. The overriding .Distinct method in the Select class works correctly, but not the one in SqlQuery. (I hope I have all this right, I'm going from memory and it was a few weeks ago !) My workaround was to say: SqlQuery sql = new Select(Widgets.Columns.WidgetType) .Distinct() .From(Widgets.Schema) .OrderAsc(Widgets.Columns.WidgetType); This (a little unfortunately) works, for the reasons given above. Looking at the spaghetti, I realised that there are some times when a DISTINCT might need to be suppressed (when using aggregates for example) so we'd need to work out all the possibilities here. The best start would be to put forward all test cases where DISTINCT should or should not appear in the SQL given the presence of .Distinct() I wasn't willing to go there, but if you are, I'd probably be willing to sit down for an hour or so and make up some test cases.

BTW, I've delegated myself to pull your latest, but I'm in a bit of a busy spot the next few days. I hope to do it sometime over the weekend.

Ben McIntyre

[FrostyZoob]

Ben,

Your workaround makes more sense, especially since from a SQL standpoint the DISTINCT keyword immediately follows the SELECT keyword anyway.

I realised that there are some times when a DISTINCT might need to be suppressed (when using aggregates for example) so we'd need to work out all the possibilities here.

True, but I'm not sure it's an issue in the current source. If any aggregates exist, a different code path is taken and injecting DISTINCT won't occur:

    if(query.Aggregates.Count > 0)
        columnList = BuildAggregateCommands();
    else
    {
        if (query.IsDistinct)
            sb.Append(query.DistinctSpec);

BTW, I've delegated myself to pull your latest, but I'm in a bit of a busy spot the next few days. I hope to do it sometime over the weekend.

Thanks! Hopefully this will give me enough time to submit a fix for another bug I've found. (I will submit an issue shortly.)

Given what I've found, do you have any thoughts as to weather or not the subsonic / SubSonic-2.0 repository here contains the latest 2.2 source? I have my doubts. I hope to do a more thorough comparison between the source here and the 2.2 source from Google Code.

[6pac]

@#$ !! You mean there's ANOTHER copy of the source ? I'm only just getting over the move from SubVersion to GIT ! Don't tell me it's moving to Mercurial !

Re fix: if you're confident that the above code is the only point where this decision is made. I found overloads duplicating a lot of the logic, wasn't confident to make the call without sitting down and nutting it all out for a few hours. Plus there's already some duplication of code paths for SQL generation in 2.x. I suppose in the end, I should put together some more unit tests to test the cases I'm worried about, then just be happy if they pass. This code is end-of-lifecycle.

[6pac]

OK, looking around, it looks like Google Code is a dead branch of the SV code. Given that GIT has repos only for 2.0 and 3.0, I'm making the assumption that this really means 2.x and 3.x. In other words, I'd expect the code to be the latest 2.2 I've used WinMerge in the past to check source trees outside of source control. Shall I check it out ?

[6pac]

I note that in the GitHub version, the SubSonic and SubSonic.Tests assemblies are marked 2.2, Migrations, SubStage and SubCommander are marked 2.1 So I think the assumption holds.

[FrostyZoob]

Re fix: if you're confident that the above code is the only point where this decision is made. I found overloads duplicating a lot of the logic, wasn't confident to make the call without sitting down and nutting it all out for a few hours. Plus there's already some duplication of code paths for SQL generation in 2.x.

I did a global search for "GenerateCommandLine" and couldn't find any overloads for it. Also, I did a search for "DistinctSpec" and the only time the getter is even called is in GenerateCommandLine(). So unless I'm being really dense and missing the obvious (wouldn't be the first time), I think we're safe to make the change.

[FrostyZoob]

OK, looking around, it looks like Google Code is a dead branch of the SV code. Given that GIT has repos only for 2.0 and 3.0, I'm making the assumption that this really means 2.x and 3.x. In other words, I'd expect the code to be the latest 2.2 I've used WinMerge in the past to check source trees outside of source control. Shall I check it out ?

I've finally had a chance to pull the entire source from Google Code myself and use WinMerge to compare and it seems like you're right. Things I'm seeing:

The Git source has a lot more unit tests.

The Git source has more XML documentation.

Use of the "[Rollback]" attribute in the Git source. The GC / SVN source uses the (older, if I'm not mistaken) [RollBack] attribute. (Note the case.)

...among other things.

I guess I just ran into some freak regression?!?

[jimkahl]

Source code that you download is wrong. Any source code I have ever seen for SubSonic does not contain a Distinct method in the SqlQuery class. My guess is that someone at sometime wrote it in and compiled it into your dll but never saved the source. I had to do that where I work (along with a few other things). Even the source for SubSonic 3.x does not have a Distinct method in the class.

[6pac]

Incorporated.