3
\$\begingroup\$

I came up with this TSQL for SQL Server 2008:

DECLARE @TimeWindowInSeconds INT

SET @TimeWindowInSeconds = 10

IF OBJECT_ID('tempdb..#Temp') IS NOT NULL
    DROP TABLE #Temp
CREATE TABLE #Temp
    (
      Class INT,
      DT DateTime
    )

INSERT INTO #Temp (Class, DT)
SELECT 1, '2014-11-05 10:55:00'
    UNION ALL
SELECT 1, '2014-11-05 10:55:10'
    UNION ALL
SELECT 1, '2014-11-05 11:55:10'
    UNION ALL
SELECT 2, '2014-11-05 10:55:11'
    UNION ALL
SELECT 2, '2014-11-05 13:56:10'

;WITH CTE1 AS
(
    SELECT
        Class,
        DT,
        ROW_NUMBER() OVER (ORDER BY Class, DT ) AS RowNumber
    FROM #Temp 
)
,CTE2 AS
(
    -- A is the successor 
    SELECT
        A.Class,
        A.RowNumber,
        B.RowNumber AS RowNumber1,
        A.DT,
        B.DT AS DT2,
        DATEDIFF(second, B.DT, A.DT) AS DifferenceInSeconds,
        CASE WHEN B.DT IS NULL THEN 1 END z
    FROM CTE1 AS A
    LEFT OUTER JOIN CTE1 AS B ON A.RowNumber = B.RowNumber + 1 AND A.Class = B.Class 
    AND DATEDIFF(second, B.DT, A.DT) <= @TimeWindowInSeconds

)
,CTE3 AS
(
    SELECT
        CTE2.Class,
        CTE2.DT,
        ( 
            SELECT    
                SUM(z)                           
            FROM      CTE2 c
            WHERE     c.DT <= CTE2.DT AND c.Class = cte2.Class
                ) AS SubGroup
    FROM CTE2
)
SELECT 
    *
FROM CTE3
    WHERE CAST(CTE3.Class AS NVARCHAR(100)) + '-' + CAST(CTE3.SubGroup AS NVARCHAR(100)) IN
    (
        SELECT CAST(CTE3.Class AS NVARCHAR(100)) + '-' + CAST(CTE3.SubGroup AS NVARCHAR(100)) FROM CTE3 GROUP BY CTE3.Class, CTE3.SubGroup
HAVING COUNT(*) > 1
    )

This clusters datetime values inside groups (class) if they fall inside the same time window and if the cluster contains more than 1 entry. Please review the code.

\$\endgroup\$
2
\$\begingroup\$

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.

However, CTE2 then selects seven columns, of which only A.Class, 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 JOIN.

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 CTE2 and CTE3 (you'd then join to that CTE in the current CTE3, using 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 CTE3 (because Subgroup will be available as a regular column in CTE3 now).

In other words, in order to work out which combinations of Class and 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 Class and 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 Class and Subgroup just once, give it an alias, and then do

WHERE alias IN ( SELECT alias FROM cte3 GROUP BY ... )
\$\endgroup\$
1
\$\begingroup\$

Kudos

I think your query is written well. Your style is easy to read, you are completely consistent with capitalization of keywords. Those are good practices that should be commended.

That said, here are some things I think could be improved.

Naming

CTE1, CT2, CT3 are not very good names. This kind of structure would be made clearer if they were named according to the information they are selecting. Further, aliasing them as A, B, c does not add to the clarity, in fact I think it just blurs things out.

INSERT INTO #Temp

I think this:

INSERT INTO #Temp (Class, DT)
SELECT 1, '2014-11-05 10:55:00'
    UNION ALL
SELECT 1, '2014-11-05 10:55:10'
    UNION ALL
SELECT 1, '2014-11-05 11:55:10'
    UNION ALL
SELECT 2, '2014-11-05 10:55:11'
    UNION ALL
SELECT 2, '2014-11-05 13:56:10'

Would read better like this:

INSERT INTO #Temp (Class, DT) VALUES
    (1, '2014-11-05 10:55:00'),
    (1, '2014-11-05 10:55:10'),
    (1, '2014-11-05 11:55:10'),
    (2, '2014-11-05 10:55:11'),
    (2, '2014-11-05 13:56:10');

End your SQL statements;

MS SQL Server is pretty permissive in syntax as far as ending statements are concerned. In pretty much every RDBMS you would have multiple syntax errors because of missing semicolons. It is good practice to always end your statements with ; even though it is not required right now. I believe Microsoft is planning on enforcing that as some point in the future and that will make maintenance of your code base more difficult.

DECLARE @TimeWindowInSeconds INT;

SET @TimeWindowInSeconds = 10;

IF OBJECT_ID('tempdb..#Temp') IS NOT NULL
    DROP TABLE #Temp;
CREATE TABLE #Temp
    (
      Class INT,
      DT DateTime
    );
\$\endgroup\$
  • \$\begingroup\$ Thanks. But would this be your way of achieving this? \$\endgroup\$ – cs0815 Nov 5 '14 at 18:04
  • \$\begingroup\$ It seems like it is a good way to do this with CTEs. You could have used subqueries as well, but CTEs generally look cleaner. My main concern was the way you were inserting using SELECT... UNION ALL if you were doing a lot of those in one operation it might bog down your query some, I would expect. \$\endgroup\$ – Phrancis Nov 6 '14 at 0:19
1
\$\begingroup\$

I can't say how I would do this differently, because I can't understand what it does. And that is a problem for any future maintainer. Let's take a look at CTE2.

,CTE2 AS
(
    -- A is the successor 
    SELECT
        A.Class,
        A.RowNumber,
        B.RowNumber AS RowNumber1,
        A.DT,
        B.DT AS DT2,
        DATEDIFF(second, B.DT, A.DT) AS DifferenceInSeconds,
        CASE WHEN B.DT IS NULL THEN 1 END z
    FROM CTE1 AS A
    LEFT OUTER JOIN CTE1 AS B ON A.RowNumber = B.RowNumber + 1 AND A.Class = B.Class 
    AND DATEDIFF(second, B.DT, A.DT) <= @TimeWindowInSeconds

)

As it was already pointed out, numbering variables is always a bad idea.

FROM CTE1 AS A

First off, there is zero advantage to using single letter alias names. I know there are a ton of examples out there that do this, but don't. It only obfuscates the code. Particularly when your CTE name is only four characters long. Don't be lazy. Type these things out. Give CTE1 a reasonable name, like ClassDates for example, and don't bother aliasing it.

        A.RowNumber,
        B.RowNumber AS RowNumber1,
        A.DT,
        B.DT AS DT2,

Again, you're numbering aliases, which you shouldn't do. But that's not why I quoted this snippet. I quoted it because DT is a terrible name. Sometimes we don't have control over the names in the columns of our tables. That's okay though. Because that's what aliases are for. It's for taking long or confusing names and making sense of them.

Okay, I take it back a little bit. It turns out you're doing a self-join and A, B aliases do make some sense here... However, if you're going to number things, do it consistently. You have rownumber1 and DT2 both referring to the same instance of the CTE.

SELECT
    A.Class,
    A.RowNumber,
    B.RowNumber AS RowNumber2,
    A.DT AS ClassTime,
    B.DT AS ClassTime2,
    -- ...
FROM ClassTimes AS A
LEFT OUTER JOIN ClassTimes AS B 
    ON A.RowNumber = B.RowNumber + 1 
    AND A.Class = B.Class 
    AND DATEDIFF(second, B.DT, A.DT) <= @TimeWindowInSeconds

Note that I gave the join conditions some breathing space. It was really difficult to understand as a one liner. In SQL, new lines are your friend. Use them liberally. Personally, I like to make sure that all keywords are the first word on a newline.

I'm going to pick on just one more alias...

 CASE WHEN B.DT IS NULL THEN 1 END z

Need I say more? What is z? It might be clear here where it's defined, but it won't be later when we need to select or join on it.

Always keep in mind that code gets read many more times than it gets written. It needs to make sense to anyone who reads it (including yourself) 6 months or 2 years from now.

\$\endgroup\$
  • 1
    \$\begingroup\$ "because it can't understand what it does" I completely agree! \$\endgroup\$ – Phrancis Dec 20 '14 at 16:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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