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 a large Sql Server view with this schema:

  • [archive_ID] (int, not null)
  • [archive_date] (datetime, not null)
  • [archdata_path_ID] (varchar(50), not null)
  • [archdata_value] (int not null)

I need to group the records by the Date, and I need to extract just the first record for each group.

This is the current query:

WITH cte
AS (
        SELECT *
               ,CAST(archive_date AS DATE) AS C
               ,ROW_NUMBER() OVER (
                       PARTITION BY CAST(archive_date AS DATE) ORDER BY CAST(archive_date AS DATE) ASC
                       ) AS ad
        FROM ArchiveData
        WHERE archdata_path_ID = @PathID
        )
SELECT DISTINCT C
        ,archdata_value AS val
FROM cte
WHERE ad = 1
ORDER BY C ASC

The main problem is to improve the readability. Would be great to optimize also the performance, but it's not mandatory.

share|improve this question
    
Can you include the version of SQL Server this is for in the question? –  nhgrif Aug 4 at 17:29

2 Answers 2

up vote 4 down vote accepted

I believe that DISTINCT is redundant, since the CTE should produce only one row for each date whose ROW_NUMBER() is 1.

Avoid selecting * in the CTE, and list the columns you want explicitly.

Your naming is poor: CTE, C, val, ad. Please find more descriptive names.

If you are using any SQL Server ≥ 2012, then FIRST_VALUE() is the function you want.

SELECT CAST(archive_date AS DATE) AS C
     , FIRST_VALUE(archdata_value) OVER (
           PARTITION BY CAST(archive_date AS DATE)
               ORDER BY CAST(archive_date AS DATE)
       ) AS val
    FROM ArchiveData
    WHERE archdata_path_ID = @PathID
    ORDER BY C;
share|improve this answer

Okay, you firstly, in your CTE you bring back SELECT * even though you don't use most of the columns. That Select * can be removed and replaced with the actual columns you need.

The CTE, while arguably helping readability a little is not very quick, I would recommend avoiding CTE's, and using subqueries or temporary tables. Table Variables are okay if you only have a handful of rows.

From what I can see, the CTE in this query is not needed anyway, so removing it should help you to understand what is actually happening.

Here is my suggestion

--return archive_date and archdata_value
SELECT DISTINCT CAST(archive_date AS DATE) AS date,
archdata_value AS val
FROM ArchiveData
WHERE archdata_path_ID = @PathID
--this is in the having clause because aggregate functions can not be in the where clause 
HAVING ROW_NUMBER() OVER (
       PARTITION BY CAST(archive_date AS DATE) ORDER BY CAST(archive_date AS DATE) ASC) = 1
--I changed from C to date to make it clearer what was being returned
ORDER BY date ASC

This should do exactly the same as the query you provided but in a single step.

share|improve this answer
    
Thanks for your help. How can I translate the "HAVING ROW_NUMBER() OVER ( PARTITION BY ... )" part into Linq? –  simoneL Aug 4 at 14:30
    
You may find this page and this page helpful. They both have what look to be pretty good examples of how to do a having clause in Linq. –  PenutReaper Aug 4 at 14:38
1  
I'm not sure I like the idea of blindly avoiding CTEs in favor of subqueries, temp tables, and table variables. Use the right tool for the right job, and frequently CTEs will be the right tool. Also, simple (non-recursive) CTEs are not slower than subqueries, but they do tend to make queries more readable. Meanwhile, recursive CTEs do stuff that subqueries can't do. In this specific example however, I agree that the CTE is probably unnecessary. –  nhgrif Aug 4 at 17:35
    
It might just be the queries I've had to deal with at work. But removing the CTEs has almost always improved performance. Almost always, I realise they still have a place, I just found that using them as more of a last resort worked better. –  PenutReaper Aug 4 at 18:45

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.