views:

69

answers:

4

I have the following SQL:

ALTER PROCEDURE [dbo].[SP_Products_GetList]


@CatID int,
@CatName int,
@SortBy varchar(50),
@SortType varchar(50)

AS


SELECT Products.ProductID, ProductName, MAX(Price) Price FROM Products 
    INNER JOIN  ProductCategory
        on Products.ProductID = ProductCategory.ProductID 
    INNER JOIN  (
                    SELECT * FROM Categories 
                        WHERE 
                            ( @CatID is null or @CatID = CatID ) and
                            ( @CatName is null or @CatName = CatName )
                ) Categories 
        on ProductCategory.CatID = Categories.CatID 
    INNER JOIN ( 
                    SELECT ProductID, max(Price) Price  from Prices WHERE PriceID IN 
                            ( SELECT MAX(PriceID) FROM Prices 
                            GROUP BY ProductID , SizeID)
                    GROUP BY ProductID
                ) as Prices 
        on Prices.ProductID = Products.ProductID 
GROUP BY ProductName, CatName, Products.ProductID, Price
ORDER BY 
CASE @SortType 
    WHEN 'desc' THEN  
    CASE @SortBy 
        WHEN 'ProductID' THEN Products.ProductID 
        WHEN 'ProductName' THEN ProductName
        WHEN 'Price' THEN Price  
        END 
    END 

EXECUTION PASSS

EXEC    [dbo].[SP_Products_GetList]
        @CatID = 1,
        @CatName = NULL,
        @SortType = 'DESC',
        @Sortby = 'ProductID'

EXECUTION FAILED

EXEC    [dbo].[SP_Products_GetList]
        @CatID = 1,
        @CatName = NULL,
        @SortType = 'DESC',
        @Sortby = 'ProductName'

Msg 245, Level 16, State 1, Procedure SP_Products_GetList, Line 13 Conversion failed when converting the varchar value '01-My First Tools Diaper Cake' to data type int.

When I alter my query without case and write simple:

.....
ORDER BY ProductName

It works fine

Why is is trying convert varchar to int as shown in the error message?

+2  A: 

You need to separate the case statements by type. For instance, have a case statement for your int columns. Have a 2nd case statement for your varchar types. You have ProductID listed first inside the case statement, which is an int, so it will use that as the data type for the case statement.

rchern
@rchern Thanks man it works.
SOF User
+4  A: 

Your case statement's output type is an int, as that's the type of the first element of the expression (Products.ProductID). In order for it to work, you'll have to explicitly convert each value to a varchar (meaning that you'll also have to prefix your values with zeroes in order for them to sort correctly).

You'd do better doing something like this:

ORDER BY 
    CASE WHEN @SortBy = 'ProductID' THEN Products.ProductID ELSE NULL END,
    CASE WHEN @SortBy = 'ProductName' THEN ProductName ELSE NULL END,
    CASE WHEN @SortBy = 'Price' THEN Price ELSE NULL END

Obviously, this doesn't take direction (ASC vs DESC) into account, but that should be straightforward to add.

Adam Robinson
A: 

The case statement resolves to INT at this time because the ProductId returns an INT. You can rewrite this to cast it to VARCHAR to avoid the issue.

ORDER BY  
CASE @SortType  
    WHEN 'desc' THEN   
    CASE @SortBy  
        WHEN 'ProductID' THEN CONVERT(VARCHAR(MAX), Products.ProductID)
        WHEN 'ProductName' THEN CONVERT(VARCHAR(MAX), ProductName)
        WHEN 'Price' THEN CONVERT(VARCHAR(MAX), Price)
        END  
    END  

I used VARCHAR(MAX) only because I don't know the length of the ProductName field. You can alter this so that it is the length of the ProductName field.

What you appear to be trying to do is dynamically generate SQL on the fly. T-SQL isn't so great at this. The most efficient way to do this from a performance perspective is to build your query string and then execute it. This is a good technique in some situations, but your code is adequate as is. Here's an example of your code if it were dynamically generated SQL:

DECLARE @SqlString NVARCHAR(MAX) = '';
SET @SqlString = N'SELECT Products.ProductID, ProductName, MAX(Price) Price FROM Products  
    INNER JOIN  ProductCategory 
        on Products.ProductID = ProductCategory.ProductID  
    INNER JOIN  ( 
                    SELECT * FROM Categories  
                        WHERE  
                            ( @CatID is null or @CatID = CatID ) and 
                            ( @CatName is null or @CatName = CatName ) 
                ) Categories  
        on ProductCategory.CatID = Categories.CatID  
    INNER JOIN (  
                    SELECT ProductID, max(Price) Price  from Prices WHERE PriceID IN  
                            ( SELECT MAX(PriceID) FROM Prices  
                            GROUP BY ProductID , SizeID) 
                    GROUP BY ProductID 
                ) as Prices  
        on Prices.ProductID = Products.ProductID  
GROUP BY ProductName, CatName, Products.ProductID, Price 
ORDER BY  ' + @SortBy + ' ' + @SortType;

EXEC sp_ExecuteSQL @SqlString;

The above code isn't completely correct. The parameterized section in the middle probably should be reworked or you will need to pass the values as parameters to the sp_executesql command. There are two draw backs to using dynamically generated SQL. 1st, the code may be subject to SQL injection attacks. If this is an internal report, then it doesn't seem likely that this would be an issue. The 2nd drawback is that it is harder to understand. Just look at the code to see what I mean!

Registered User
-1 Do not do this; converting the numeric values to a string will sort them lexicographically, *not* numerically (meaning `10` will be considered "smaller" than `2`). For this to work correctly you have to pad the values with zeroes on the left side, the number of zeroes being determined by the maximum number of digits you'd ever expect to see on the left side of the decimal.
Adam Robinson
The problem with this is alpha sorting order vs. numeric sorting order when price is chosen, you will get 1, 11, 12, 2, 21, 22, 3, etc.
Cade Roux
+1  A: 

When you use a CASE statement, the output has to be a value/column of a particular type. In the case of Price, this is a most likely not a VARCHAR data type.

There are a few ways to do this effectively.

My personal favorite is to take the re-usable part of the query above the order by and put it into an inline table-valued function.

Then in your SP:

IF (@OrderBy = 'COL1')
    SELECT *
    FROM UDF(params)
    ORDER BY COL1
ELSE IF (@OrderBy = 'COL2')
    SELECT *
    FROM UDF(params)
    ORDER BY COL2

This is going to have the most efficient execution plan (actually it's going to have a very efficient conditional plan) since it's not trying to convert every column or do fancy workarounds like sorting on columns with no data in some cases.

And it's relatively modular in terms of the complex code which you don't want to have to repeat.

Cade Roux
+1: Most performant, short of dynamic SQL
OMG Ponies
@OMG: Certainly not; the most performant would be write separate queries for each sort clause. I don't know that I'd suggest adding additional schema objects that pertain only to one query/sproc.
Adam Robinson
OMG Ponies
@OMG: I'm curious as to why I "clearly don't know what dynamic SQL is"; I certainly do. Nonetheless, having three distinct pre-compiled stored procedures that differ only by sort clause being called dynamically by a single procedure that examines the sort clause variable would, in fact, outperform dynamic SQL.
Adam Robinson
@Adam Robinson: You're embarrassing yourself - queries aren't "pre-compiled", their execution path is stored for a limited period of time. Google "Hard/Soft parse".
OMG Ponies
@OMG: I'm not certain where your vitriol is coming from, but in any case, yes, a query *plan* (not path, if we're going to nitpick each other's irrelevant mistakes in vocabulary) is what's compiled and cached (though you can control when, and for how long, a procedure's plan is compiled and cached). In any case, I won't be further participating in conversation if you can't maintain a respectful attitude.
Adam Robinson
@Adam Robinson Dynamic SQL vs. my solution or any manually expanded version would have very little difference. An SP has to be compiled the first time, and a dynamic SQL execution has to be compiled the first time, so I'm not sure that's a good argument unless the queries have a lot of variation. In that case, if there are 6 possible search types, you will get 6 plans vs one conditional plan or 6 individual plans for 6 SPs. I really think it is six of one and half-dozen of the other. Where dynamic SQL falls down with plans is where the queries themselves change a lot.
Cade Roux
@Cade: You're absolutely right; the difference would likely be immeasurable. I only wanted to point out that it wasn't the absolute "most performant" as @OMG Ponies stated. You can, however, precompile the execution plan for a stored procedure, so you can bypass the first-run compilation. I wouldn't actually advocate creating distinct procedures that differ only by sort, since my only real objection was creating an additional database object that (ostensibly) pertains to one specific query just to be able to vary the sort value.
Adam Robinson
@Adam Robinson: "query plan" and "execution path" are synonyms - a query will not always execute the same for sake of outdated statistics if the query is no longer in the cache. You've consistently made unfounded, incorrect claims and I've corrected you every time - you've been very aggressive in your defensiveness.
OMG Ponies
@Cade Roux: Sure, there'll be a hard parse for the first time for each iteration of the query - but unless the query falls out of the cache, it'll still only be a soft parse which can never be guaranteed for the OPs query.
OMG Ponies
@Adam Robinson: Honestly, where does this "additional database object" nonsense come from? You realize Cade is throwing you a bone, right?
OMG Ponies
@OMG Ponies: It is my understanding that you get a single conditional plan on the first execution: http://msdn.microsoft.com/en-us/library/aa174792(SQL.80).aspx And that this will also defeat parameter sniffing, since plans do need to be generated for other parameter-dependent paths. In the last couple months I read a blog posting on this very topic, and now I can't remember who wrote it. So in the single proc case, you will have a single (perhaps longer) compilation instead of up to 6 compilations on demand.
Cade Roux
For the record, I have no objection to using dynamic SQL versus 6 separate SPs, but I think there are some drawbacks to dynamic SQL in terms of system integrity (dependencies not as easy to search for, parsing cannot ensure objects exist, cannot get SCHEMABINDING to help control changes) and security (may need broader rights to underlying tables, injection, etc) which mean that dynamic SQL is a technology which generally comes later in the design decision matrix for me.
Cade Roux