Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this currently working code, but it's extremely slow. I'm talking almost an hour to execute, if it executes at all (our server isn't all that great to start with). Is there a better way to write this?

Basically, I am wanting to return the Top 5 parts, that match a distinct pattern code within the same table. So, if there were 100 distinct pattern codes in my [PartNumber] table, I would end up with 500 records.

FINAL COMPLETED AND WORKING CODE (executes in 15 seconds):

SET NOCOUNT ON
GO
SET ARITHABORT ON
GO
SET ANSI_PADDING ON
GO

DECLARE @TopCount int = 5;

SELECT [DistinctTop].[PartNumber]
      ,[DistinctTop].[PatternCode]
      ,[DistinctTop].[LanguageCode]

FROM (SELECT [PartNumber]
            ,[PatternCode]
            ,[LanguageCode]
            ,RANK() OVER 
                (PARTITION BY [PatternCode] ORDER BY [PartNumber]) AS RowRank
     FROM PartNumberInfo) AS [DistinctTop]

WHERE [DistinctTop].[RowRank] <= @TopCount

ORDER BY [DistinctTop].[PatternCode];

ORIGINAL at time of question posting (Executes in 32 minutes):

DECLARE @PartInfo TABLE(
    [PartNumber] [varchar](64) NOT NULL,
    [PatternCode] [varchar](64) NOT NULL,
    [LanguageCode] [varchar](24) NOT NULL);

DECLARE @PatternCode nvarchar(64);
DECLARE pattern_Cursor CURSOR LOCAL SCROLL STATIC
    FOR
    SELECT TOP 1000 [PartNumber].[PatternCode]
    FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
    WHERE PatternCode NOT LIKE 'NOT FOUND'
    GROUP BY [PartNumber].[PatternCode]

    OPEN pattern_Cursor;

    FETCH NEXT FROM pattern_Cursor
    INTO @PatternCode;

    WHILE @@FETCH_STATUS = 0
        BEGIN
            INSERT INTO @PartInfo
            SELECT TOP 5 [PartNumberInfo].*
            FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
            WHERE [PartNumber].[PatternCode] = @PatternCode;

            FETCH NEXT FROM pattern_Cursor
            INTO @PatternCode;
        END
CLOSE pattern_Cursor;
DEALLOCATE pattern_Cursor;

SELECT *
FROM @PartInfo
ORDER BY [@PartInfo].[PatternCode]

First revision (Executes in 24 minutes):

CREATE TABLE #PartInfo ([PartNumber] [varchar](64) NOT NULL,
                        [PatternCode] [varchar](64) NOT NULL,
                        [LanguageCode] [varchar](24));

DECLARE @PatternCode nvarchar(64);

DECLARE pattern_Cursor CURSOR FAST_FORWARD
    FOR 
    SELECT [PartNumber].[PatternCode]
    FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
    WHERE PatternCode <> 'NOT FOUND'
    GROUP BY [PartNumber].[PatternCode]

    OPEN pattern_Cursor;

    FETCH NEXT FROM pattern_Cursor
    INTO @PatternCode;

    WHILE @@FETCH_STATUS = 0
        BEGIN
            INSERT INTO #PartInfo
            SELECT TOP 5 [PartNumber].*
            FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
            WHERE [PartNumber].[PatternCode] = @PatternCode
            ORDER BY [PartNumber].[DateTimeValidated] DESC;

            FETCH NEXT FROM pattern_Cursor
            INTO @PatternCode;
        END

CLOSE pattern_Cursor;

DEALLOCATE pattern_Cursor;

SELECT *
FROM #PartInfo
ORDER BY [#PartInfo].[PatternCode]
share|improve this question
1  
If a query ever takes more than 30s to run, it's not being correctly done. (Usually, 1s is the limit for "something is fishy".) –  ANeves Mar 13 at 14:32
1  
@ANeves I can't say I fully agree with that. I've seen some pretty efficient, fully indexed queries take upwards of a minute. I mean, in most cases I can probably agree, but also considering our server isn't all that great I can't really base anything on that logic. A query could take 2 seconds at 6am, but 2 minutes at 11am and there's really nothing I can do on that end. –  Volearix Mar 13 at 14:49
1  
Make it a rule of thumb, more than a rule. :) –  ANeves Mar 13 at 14:56
1  
Please do not update the original code from answers. That will invalidate them. You may add the updated code below the original. –  Jamal Mar 13 at 14:58
1  
Just a coding style note: It is customary in SQL to leave no blank lines between elements of a query. Each query should form a "paragraph", with blank lines used only to separate one unit of work from another. This will make your code more readable to peers, successors, and StackExchange readers. –  Jonathan Van Matre Mar 13 at 18:56

3 Answers 3

up vote 5 down vote accepted

RANK()

In the past I have found that for these purposes, using RANK() is the way to go.
In all honesty, I don't know how cursors perform, in comparison.

SELECT *
FROM (
    select *, RANK() OVER (PARTITION BY PatternCode ORDER BY PartNumberInfo.Foo) AS RowRank
    FROM PartNumberInfo
) AS Bar
WHERE Bar.RowRank < 5

http://sqlfiddle.com/#!6/5a06b/1

This might benefit from indexing.

Use nulls instead of NOT FOUND

You should be using nulls instead of such a default value.

nvarchar

You should be using nvarchar instead of varchar: http://stackoverflow.com/questions/144283/what-is-the-difference-between-varchar-and-nvarchar

If the content of your columns is never user-input, then you can consider exceptions.

share|improve this answer
    
BEAST! I knew about RANK, but wasn't sure how to use it! Took me from around 24 minute execution to 15 seconds! Final code posted. :-) –  Volearix Mar 13 at 16:07
    
15 seconds still feels too long for such a simple thing. I have a nagging suspicion that you could profit from good indexing. –  ANeves Mar 13 at 18:35
    
CREATE NONCLUSTERED INDEX IX_PartNumberInfo_PatternCode ON [PartNumber] (PartNumber); Exists already. Not good enough? I do not do much indexing, something better that you could suggest? –  Volearix Mar 13 at 18:58
    
My guess is that it does not significantly affect the query - try to "explain"/"check the execution plan of" the query. I would look into doing a composite index on the two columns that you use in the query: PatternCode and PartNumber (note that the order of the columns in the index is relevant). I suggest you ask a question in StackOverflow about the index. –  ANeves Mar 13 at 19:12

You have three problems that I can see. Two performance, and one logic. In least-to-most significant order:

  1. Performance: You should no use the NOT LIKE 'NOT FOUND' statement, it should be <> 'NOT FOUND'
  2. Potential logic problem: You should have an order-by clause on the inner select for the TOP 5 ... otherwise, what TOP 5 are you getting?
  3. Performance: Using declare @table... syntax has different performacne to create table #table ... For example, this blog entry shows some real differences. I recommend trying the same query with a #temp table instead.

Also, read up on this SO Answer: SQL Server tables: what is the difference between @, # and ##?

Edit:

It was almost too obvious to ask... but, you have done the two most basic items... right?

  1. an index on the [PartNumber].[PatternCode]
  2. updated all table statistics

... right?

share|improve this answer
    
So, I have replaced the NOT LIKE with <> and modified the query to use #. Please see my updated code. It's still really slow though. –  Volearix Mar 13 at 14:54
    
@Volearix - updated the answer with two more suggestions.... –  rolfl Mar 13 at 15:12
    
Yes on the index (CREATE CLUSTERED INDEX [PartInfo$PatternCode] on #PartInfo ([PatternCode]);), though there might be a better index available? On the second, I'm not familiar with updating table statistics. Unless it's the index or in the code I posted, it didn't happen. –  Volearix Mar 13 at 15:20
1  
The index needs to be on [Web_Service].[dbo].[PartNumber] not on #PartInfo .... –  rolfl Mar 13 at 15:21
    
CREATE NONCLUSTERED INDEX IX_PartNumberInfo_PatternCode ON [PartNumber] (PartNumber); Increased performance 2 seconds. Any better indexing idea? There is already a PK on PattenCode and PartNumber, which I cannot modify. –  Volearix Mar 13 at 18:54

On your final and completed code, there is no reason to create a variable for a number that doesn't change.

So this:

WHERE [DistinctTop].[RowRank] <= @TopCount

Should become this:

WHERE [DistinctTop].[RowRank] <= 5

The less variables and things that are created on the database the better, this variable is unneeded.


You could use a CTE (Common Table Expression)

rather than this:

DECLARE @TopCount int = 5;

SELECT [DistinctTop].[PartNumber]
      ,[DistinctTop].[PatternCode]
      ,[DistinctTop].[LanguageCode]

FROM (SELECT [PartNumber]
            ,[PatternCode]
            ,[LanguageCode]
            ,RANK() OVER 
                (PARTITION BY [PatternCode] ORDER BY [PartNumber]) AS RowRank
     FROM PartNumberInfo) AS [DistinctTop]

WHERE [DistinctTop].[RowRank] <= @TopCount

ORDER BY [DistinctTop].[PatternCode];

Do this:

WITH [DistinctTop]
AS 
(
    SELECT [PartNumber]
        ,[PatternCode]
        ,[LanguageCode]
        ,RANK() OVER (PARTITION BY [PatternCode] ORDER BY [PartNumber]) AS RowRank
    FROM PartNumberInfo
)
SELECT [DistinctTop].[PartNumber]
      ,[DistinctTop].[PatternCode]
      ,[DistinctTop].[LanguageCode]
FROM [DistinctTop]
WHERE [DistinctTop].[RowRank] <= 5

This is a little cleaner than what you have in the way that it is clear to a coder what you are doing here and doesn't look messy.

As far as creating variables on the Database, if I remember right this makes the execution plan a little cleaner as well. You would have to test this though.

If you want to post the code with the results, please post a new question.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.