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.

Just trying out my T-SQL foo.

Wrote a script to get the average time (in seconds) to first answer for a specified language.

-- Could not get this so that the user entered
-- the language in a box at the bottom working.
-- so you have to edit the code here.
declare @Language nvarchar(20) = 'php' -- ##LanguageTag##

declare @AnswerTime table (Id int, CreationDate date, AnswerTime date, Elapse bigint, Slot int ) 

insert @AnswerTime
Select Q.Id, 
       max(Q.CreationDate) AS CreateTime, 
       min(A.CreationDate) AS AnswerTime, 
       DATEDIFF(second, max(Q.CreationDate), min(A.CreationDate)) AS Elapse,
       Year(max(Q.CreationDate))*12+Month(max(Q.CreationDate)) AS Slot
From   Posts Q, Posts A 
Where  Q.PostTypeId = 1
  and  Q.Id = A.ParentId
  and  CHARINDEX(@Language, Q.Tags) != 0
Group by Q.Id, Q.Tags
Order by Elapse

Select min(CreationDate), 
       count(*)         as [Count of Tickets], 
       -- min(Elapse)   as [Min Time to Answer],
       -- max(Elapse)   as [Max Time to Answer],
       avg(Elapse)    as [Average time to first Answer]
From   @AnswerTime
Group by Slot

Here it is running.

share|improve this question
    
I took the liberty to get your parameter up and running ;) –  Vogel612 Jul 31 at 16:55
1  
You may be interested in this chart of quick-to-answer ratios –  rolfl Jul 31 at 17:55
    
@rolfl: I had that in my head as I was experimenting. I was just trying to achieve something non trivial while experimenting. –  Loki Astari Jul 31 at 20:19

6 Answers 6

up vote 16 down vote accepted

There are a few things that are off in this query. Going through them in some sense of order:

  • I am still a fan of 'old style' joins, but, the CTE concept is a real winner in SQL Server, so, instead of creating the table variable, just use the CTE.

  • I can tell that your code went through some iterations, and as a result, you have some 'cruft' that can be disposed of. Delete those things that you don't select (like you select min(A.CreationDate) as AnswerTime which is never used.

  • The SEDE tables take some getting used to. You have done a CharIndex on the tags column. That's fine, but, your example also pulls in tags, as well as . To get around this, you should be aware that SEDE stores the tag names in <...> braces in the Tags field, so searching for <php> is what you want.

    Using the CharIndex is OK, but more standard will be using like, as in: ... and Tags like '%<php>%'

  • I have actally given up on using the Tags field completely, and instead I do the join to the PostTags table which allows you to do the join natively without the like/charindex.

  • There is no need for the aggregate column on the Question's CreationDate. The max(Q.CreationDate) is redundant (there will only be one CreationDate per Question ID).

  • You should always use as many constants as you can when doing SQL Queries (at least as much as you can, until you find a performance reason to not overdo it....), so you should also add the A.PostTypeId = 2 even though its ParentID is the Q's ID.

  • I did not like the way you were 'Slotting' the data in to months. I find that subtracting the days-in-month-less-one from the given date brings the date back to the first-of-the-month, and then truncating the time, allows you to have all dates represented by exactly the first-of-the-month:

    Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate)) as CreationMonth
    
  • Getting the Variable substitution to work was easier once I used the Tags table too. The stupid 'Unexpected error, blame has already been assigned' error is a PITA, but it is now gone.

  • Finally, the scale of the Y axis was so huge, that the count of the questions was not visibly appealing/apparent. By changing the scale from Seconds to Hours, the scales are better.

I forked your query here, and this is the SQL (I have tried to keep the capitalization and other style conventions consistent with your code):

declare @Language nvarchar(25) = ##LanguageTag:string##;
declare @tagid int;

select @tagid = Id
from Tags
where TagName = @Language

print @Language + '->' + Convert(NVarchar(max), @tagid);

with AnswerTime as (
 Select Q.Id,
       Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate)) as CreationMonth,
       Convert(float, DATEDIFF(second, Q.CreationDate, min(A.CreationDate))) AS Elapsed
 From   PostTags T,
        Posts Q,
        Posts A 
 Where  T.TagId = @tagid
  and  T.PostId = Q.Id
  and  Q.PostTypeId = 1
  and  A.PostTypeId = 2
  and  Q.ClosedDate is null
  and  Q.Id = A.ParentId
 Group by Q.Id,
          Q.CreationDate,
          Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate))
)
Select CreationMonth, 
       count(*)         as [Count of Tickets], 
       avg(Elapsed)/3600.0 as [Average hours to first Answer]
From   AnswerTime
Group by CreationMonth

Edit/Update

I ran the query on Stack Overflow, and it fails because there are so many questions that the avg(Elapsed) fails with an arithmetic overflow. I have updated the query to only consider the past 68 years worth of data... (in case there are large date differences in seconds), and also to convert the elapsed in to a float (which does not overflow on avg())

share|improve this answer
    
The not-closed criterion that you added is a good idea. –  200_success Jul 31 at 17:53

I think overall your code is good! Nothing stands out as being particularly inefficient.

Nitpicking

I find your keyword formatting to be inconsistent: I see declare and Select and DATEDIFF etc. While it is true that SQL is not case-sensitive, it is a good practice to be consistent. Just pick your favorite one and stick to it.

Old-style join

From   Posts Q, Posts A
...
and  Q.Id = A.ParentId

This has been deprecated since ANSI-92 and should be avoided. There are a few good reasons to avoid it.

Instead, write:

From Posts Q 
Inner Join Posts A
On Q.Id = A.ParentId

Seconds?

While seconds are a precise metric, I see them as somewhat difficult to relate to, in human terms. I think hours (or at least minutes) may be preferred.

share|improve this answer

Logical correctness

  • is not ; is not . Using CHARINDEX(…, Q.Tags) to inspect a comma-delimited string is a bad idea. Join against the PostTags table instead.
  • Keep in mind that this query only gives the time to first answer for questions that received an answer. Questions that never received an answer have an infinite response time, in a sense. You have implicitly filtered out those outliers, which may or may not be a good thing. In any case, the average is likely to be skewed due to a long tail in the answer time. Perhaps the median might be a more interesting statistic than the mean.
  • How do you think migrated questions should be treated?
  • There is some jitter in the X-axis of the plot, since the min(CreationDate) does not necessarily fall on the first day of the month, especially in the early days of the site. You probably want to truncate dates to the start of the month rather than picking the min(CreationDate) of each Slot.
  • You want answer times in seconds? This is Code Review, not Stack Overflow. To get a meaningful plot of time-to-first-answer and number-of-questions on the same Y-axis, you would need to measure the answer times in days!

Expressiveness

  • Prefer Common Table Expressions to variables. Formulating the entire query as one SELECT is more elegant and gives the query planner more freedom.
  • You shouldn't place an ORDER BY in the intermediate query. It doesn't guarantee that the final result will be similarly ordered.
  • I would write this using two CTEs: one to gather all the relevant question–answer pairs, and another to compute the time spans.
  • Be consistent in your capitalization of function calls.
  • Tags are tags.  It does not matter if it's a language tag, so I wouldn't name the variable or label the input field as "Language".

Suggested solution

WITH FirstAnswers AS (
    SELECT Q.Id
         , Q.CreationDate AS QuestionTime
         , min(A.CreationDate) AS AnswerTime
        FROM Posts AS Q
            INNER JOIN Posts AS A
               ON Q.Id = A.ParentId
        WHERE EXISTS (
                SELECT PostId
                    FROM PostTags
                        INNER JOIN Tags
                            ON TagId = Tags.Id
                    WHERE PostId = Q.Id AND TagName = ##Tag:string?php##
            )
        GROUP BY Q.Id, Q.CreationDate
), AnswerTime AS (
    SELECT Id
         , QuestionTime
         , AnswerTime
         , datediff(second, QuestionTime, AnswerTime) / 86400.0 AS ElapsedDays
         , dateadd(m, datediff(m, 0, QuestionTime), 0) AS Month
        FROM FirstAnswers
)
SELECT Month
     , count(QuestionTime) AS [Question count]
     , avg(ElapsedDays) AS [Average time to first answer (Days)]
    FROM AnswerTime
    GROUP BY Month
    ORDER BY Month;

Stack Exchange Data Explorer link

share|improve this answer

Code looks good, few improvement can be done:

  1. Avoid using function/operator like charindex in the WHERE clause as it will be executed for the every single value which can decrease performance over time as data will be increased.

  2. Avoid using cross JOIN. Rather, specify which kind of JOIN you want to do, else first it join every row and they will apply the WHERE clause. I have used LEFT JOIN make sure you have the right results.

One more thing read about the temp table, and table variable too.

SELECT  A.Id ,
        A.Tags ,
        MAX(A.CreationDate) AS CreateTime ,
        MIN(A.AnswerCreationDate) AS AnswerTime ,
        DATEDIFF(second, MAX(A.CreationDate), MIN(A.CreationDate)) AS Elapse ,
        YEAR(MAX(A.CreationDate)) * 12 + MONTH(MAX(A.CreationDate)) AS Slot
FROM    ( SELECT    Q.Id ,
                    Q.Tags ,
                    Q.CreationDate ,
                    A.CreationDate AS AnswerCreationDate ,
                    CHARINDEX(@Language, Q.Tags) languageIndex
          FROM      Posts Q
                    LEFT JOIN Posts A ON Q.Id = A.ParentId
          WHERE     Q.PostTypeId = 1
        ) A
WHERE   A.languageIndex != 0
GROUP BY Q.Id ,
        Q.Tags
ORDER BY Elapse
share|improve this answer
1  
You have just moved CHARINDEX from the WHERE clause into the SELECT part. Does that really change efficiency? –  Loki Astari Jul 31 at 17:23
    
Yes that is for sure , as select is like applying the wrapper around the data –  paritosh Jul 31 at 17:25

Formatting wise, (and you probably already know this, most people find it a PITA to do) is to make sure that all your keywords are Capitalized, it makes for easier reading.

other than that the one thing that jumped out at me was

WHERE Q.Id = A.ParentId

You could JOIN the tables on that link instead.

Other than that it looks good

share|improve this answer
    
I am not sure what PITA means. –  Loki Astari Jul 31 at 17:16
    
@LokiAstari it means Pain in the Ass –  Malachi Jul 31 at 18:01

I just wanted to add to @rolfl's answer. I have no love at all for CTEs. They are almost always slower than using a temporary table or selecting from a subquery.

CTEs are usually slower than Table Variables too, in my experience with SQL Server at least, although there is little reason to use a Table Variable if you have more than a handful of rows.

I took the liberty of taking the code from ROFL's answer and changing the CTE into a subquery that is directly selected from, it significantly improved the speed, down from ~60ms to ~30ms.

Here is my alteration:

declare @Language nvarchar(25) = ##LanguageTag:string##;
declare @tagid int;
declare @epoch datetime = DateDiff(yy, -68, current_timestamp);

select @tagid = Id
from Tags
where TagName = @Language

print @Language + '->' + Convert(NVarchar(max), @tagid);

Select CreationMonth, 
count(*) as [Count of Tickets], 
avg(Elapsed)/3600.0 as [Average hours to first Answer]
From 
(
 Select Q.Id,
 Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate)) as CreationMonth,
 Convert(float, DATEDIFF(second, Q.CreationDate, min(A.CreationDate))) AS Elapsed
 From PostTags T,
 Posts Q,
 Posts A 
 Where T.TagId = @tagid
 and T.PostId = Q.Id
 and Q.PostTypeId = 1
 and A.PostTypeId = 2
 and Q.ClosedDate is null
 and Q.Id = A.ParentId
 and A.CreationDate > Q.CreationDate -- validation, and also merges.
 and Q.CreationDate >= @epoch
 and A.CreationDate < CURRENT_TIMESTAMP
 Group by Q.Id,
 Q.CreationDate,
 Convert(Date, DateAdd(day, 1 - DatePart(day, Q.CreationDate), Q.CreationDate))
) As AnswerTime
Group by CreationMonth
order by CreationMonth

And here is it working

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.