Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I've been working on a semi-awkward query in that it uses a very high number of functions given its relatively small size and scope. I was hoping to get some feedback on any ways I could format or re-factor this better?

select  Name ,
        avg(TimeProcessing / 1000 + TimeRendering / 1000 + TimeDataRetrieval / 1000) as 'Current Month' ,
        isnull(count(TimeProcessing), 0) as 'Sample' ,
        min(l2.[Avg Exec Previous Month]) as 'Previous Month' ,
        isnull(min(l2.[Sample Previous Month]), 0) as 'Sample' ,
        min(l3.[Avg Exec Two Months Ago]) as 'Two Months ago' ,
        isnull(min(l3.[Sample Two Months Ago]), 0) as 'Sample'
from    marlin.report_execution_log l
        inner join      marlin.report_catalog c on l.ReportID = c.ItemID
        left outer join ( select    l2.ReportID ,
                                    avg(TimeProcessing / 1000 + TimeRendering
                                        / 1000 + TimeDataRetrieval / 1000) as 'Avg Exec Previous Month' ,
                                    count(TimeProcessing) as 'Sample Previous Month'
                          from      marlin.report_execution_log l2
                          where     TimeEnd between dateadd(MONTH, -2,
                                                            getdate())
                                            and     dateadd(MONTH, -1,
                                                            getdate())
                          group by  l2.ReportID
                        ) l2 on l.ReportID = l2.ReportID
        left outer join ( select    l3.ReportID ,
                                    avg(TimeProcessing / 1000 + TimeRendering
                                        / 1000 + TimeDataRetrieval / 1000) as 'Avg Exec Two Months Ago' ,
                                    count(TimeProcessing) as 'Sample Two Months Ago'
                          from      marlin.report_execution_log l3
                          where     TimeEnd between dateadd(MONTH, -3,
                                                            getdate())
                                            and     dateadd(MONTH, -2,
                                                            getdate())
                          group by  l3.ReportID
                        ) l3 on l.ReportID = l3.ReportID
group by l.ReportID, Name
order by Name
share|improve this question
Because I sincerely hope that you aren't actually getting a 'Month' value (say, 1 for 'January') by using the AVG() function, you should probably rename Current Month to whatever the actual measure is, including units - perhaps Average Time Elapsed in Seconds or something. And you might consider changing the operations to (TimeProcessing + TimeRendering + TimeDataRetrieval) / 1000, which will guarantee that the same conversion is used for each field. – Clockwork-Muse Jan 3 '12 at 18:05
Not at all, avg is receiving the average execution times of the references columns (see the where clause to understand how it's identifying what month) – ElvisLikeBear Jan 3 '12 at 22:17
1  
See, that's exactly what I'm talking about - The result column name doesn't identify, at all what the data is. Some other interesting things I didn't really look at before: Are you sure your main SELECT AVG() is only getting the current month? Without a WHERE clause, it won't restrict it - not if the previous two months are also in the table. Also, be careful when using between - currently, anything equal to dateadd(MONTH, -2, getdate()) will be counted twice - once for l2, and once for l3 - I reccomend exclusive upper bounds. – Clockwork-Muse Jan 4 '12 at 16:36
add comment (requires an account with 50 reputation)

2 Answers

up vote 2 down vote accepted

You might want to use Common Table Expression or Should use meaningful table alias in Join.

You might also want to use indexes on your date column with <= and >= operator instead of between.

Surround your column names in [] instead of single quotes.

share|improve this answer
Can you elaborate more on what makes a meaningful table alias? I would have thought ReportID (matching over each table and being key fields) would be a great match here and a cte seems unnecessary? – ElvisLikeBear Jan 3 '12 at 5:42
l2 and l3 doesn't convey anything. Rather you should name it something like (e.g) AvgExecutionPreviousMonth. – Nil Jan 3 '12 at 8:37
Right. But that doesn't address my question about why we would use a CTE here? – ElvisLikeBear Jan 3 '12 at 22:18
If it's easy to read and understand the whole thing, you should not get your subquery as CTE. – Nil Jan 4 '12 at 5:14
I second the CTE idea, as it will allow the exact same query to be used for the two subqueries (meaning that maintanence can't cause divergent results). Nil - maybe you could show an example of how you'd write the CTE? – Clockwork-Muse Jan 4 '12 at 16:30
add comment (requires an account with 50 reputation)
up vote 0 down vote

I agree with Nil; it seems like this might be a good situation to use a Common Table Expression.

I haven't tested this out, but below is my attempt to rewrite this query using a CTE:

with ReportByMonth (ReportID, [Year], [Month], [Avg Exec], [Sample]) as
(
    select  ReportID ,
            avg(TimeProcessing / 1000 + TimeRendering / 1000 + TimeDataRetrieval / 1000) as [Avg Exec] ,
            datepart(YEAR, TimeEnd) as [Year] ,
            datepart(MONTH, TimeEnd) as [Month] ,
            count(TimeProcessing) as [Sample]
    from Marlin.report_execution_log
    group by
        ReportID,
        datepart(YEAR, TimeEnd),
        datepart(MONTH, TimeEnd)
)
select  Name ,
        CurrentMonthReport.[Avg Exec] as [Current Month Avg Exec] ,
        isnull(CurrentMonthReport.[Sample], 0) as [Current Month Sample] ,
        PreviousMonthReport.[Avg Exec] as [Previous Month Avg Exec] ,
        isnull(PreviousMonthReport.[Sample], 0) as [Previous Month Sample] ,
        TwoMonthAgoReport.[Avg Exec] as [Two Months Ago Avg Exec] ,
        isnull(TwoMonthAgoReport.[Sample], 0) as [Two Months Ago Sample]
from marlin.report_catalog c
inner join ReportByMonth CurrentMonthReport on
    CurrentMonthReport.ReportID = c.ItemID
    and CurrentMonthReport.Year = datepart(YEAR, getdate())
    and CurrentMonthReport.Month = datepart(MONTH, getdate())
left outer join ReportByMonth PreviousMonthReport on
    PreviousMonthReport.ReportID = c.ItemID
    and PreviousMonthReport.Year = datepart(YEAR, dateadd(MONTH, -1, getdate()))
    and PreviousMonthReport.Month = datepart(MONTH, dateadd(MONTH, -1, getdate()))
left outer join ReportByMonth TwoMonthAgoReport on
    TwoMonthAgoReport.ReportID = c.ItemID
    and TwoMonthAgoReport.Year = datepart(YEAR, dateadd(MONTH, -2, getdate()))
    and TwoMonthAgoReport.Month = datepart(MONTH, dateadd(MONTH, -2, getdate()))
order by
    Name
;

EDIT: I changed my query to try to match the behavior OP's original query. I changed the join for CurrentMonthReport from "left outer join" to "inner join". This means that if a particular report has not been run during this month, then it won't show up in the result.

I also included use of the isnull function and added an order by clause.

share|improve this answer
On initial testing this doesn't return any results. Just got in the door so I'll get some work done and will then take a look at this and update you further. – ElvisLikeBear Jan 4 '12 at 22:01
add comment (requires an account with 50 reputation)

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.