I agree with the other feedback given here so far; I have
two three suggestions of my own, two minor and one more significant.
Minor point (1)
Initial assignation to a variable can be done in one line, so instead of:
DECLARE @TimeWindowInSeconds INT
SET @TimeWindowInSeconds = 10
you could just write:
DECLARE @TimeWindowInSeconds INT = 10
Minor point (2)
I am not a fan of selecting columns in intermediate rowsets that are never used subsequently. They add clutter and unnecessarily make the reader spend time understanding or visualising them.
CTE1 selects just three columns, all of which are necessary for the join in
CTE2: this is good.
CTE2 then selects seven columns, of which only
A.DT and the expression aliased as
z are required by CTE3 or presented in the final output.
I suspect that you put these columns in whilst writing
CTE2, because the others are all used in the join within that CTE. This is fine for work-in-progress, but these columns should be removed from the finished query. They don't need to be in the
SELECT list just because they're used in the
The bigger one
Your code uses both common table expressions (CTEs) and subqueries. For me, this is quite a painful inconsistency. CTEs were effectively an evolution of subqueries in the ANSI SQL standard; mixing both in one query feels to me like designing an aeroplane that has both propellers and a jet engine.
Aside: please don't read too far into that analogy: this answer is not about their relative performance, nor am I claiming that one is made redundant by the other. People still use prop planes, after all.
Unless performance is particularly important, and you know for certain that subqueries perform measurably better on your server, with your data, I recommend sticking with CTEs alone:
- The syntax is easier to follow
- They mimic step-by-step problem solving, walking your reader through the solution steadily
- Well-chosen CTE names are tantamount to good comments, explaining your rationale
- They can make code shorter because you can re-use them
In your particular case, I think the
SubGroup subquery will need to become a separate CTE between
CTE3 (you'd then join to that CTE in the current
c.DT <= CTE2.DT AND c.Class = cte2.Class as the joining conditions).
Having done that, if I've understood your intentions correctly, the correlated subquery right at the end (
WHERE...IN ( subquery )) could be removed entirely, and replaced with a partitioned
COUNT() function in the current
Subgroup will be available as a regular column in
In other words, in order to work out which combinations of
Subgroup occur more than once, you'd add a column to your refactored
CTE3 something like:
COUNT(*) OVER ( PARTITION BY Class, Subgroup ) as CountBy_Cls_Sgp
and then your final query outside the CTEs would simply restrict where
CountBy_Cls_Sgp > 1.
This also removes altogether the need to
CAST and concatenate
Subgroup, since you're not including that concatenation in your result set.
Even if you didn't want to change the
WHERE...IN ( subquery ) part, you could simplify things by adding an extra, final CTE in which you do the casting and concatenation of
Subgroup just once, give it an alias, and then do
WHERE alias IN ( SELECT alias FROM cte3 GROUP BY ... )