6
\$\begingroup\$

One of the reports I am working on requires me to locate searches a customer makes, and graph them in certain manners.

I have one query I'm particularly concerned about. This one takes the most amount of time (relative to the batch) and seems quite ugly to me.

Essentially, this should graph the search results by the day of the week and give me: the total number of searches, the average searches per week day, the number of searches that had no returned results, and the percent of searches that have no results.

SELECT
    DATENAME(WEEKDAY, [AtDateTime]) AS [Day of Week]
    ,COUNT(*) AS [Number of Searches]
    ,CAST(CAST(COUNT(*) AS DECIMAL(10, 2)) / COUNT(DISTINCT CONVERT(DATE, [AtDateTime])) AS DECIMAL(10, 2)) AS [Average Searches per Day]
    ,SUM(CASE WHEN [NumFound] = 0 THEN 1 ELSE 0 END) AS [Number of Searches with no Results]
    ,CAST(CAST(SUM(CASE WHEN [NumFound] = 0 THEN 1 ELSE 0 END) AS DECIMAL(10, 2)) / COUNT(*) AS DECIMAL(10, 4)) AS [Percent of Searches with no Results]
FROM [DB].[dbo].[SearchHistory] 
WHERE NOT EXISTS 
(
    SELECT 1 FROM [DB].[dbo].[CustomerExclusions]
    WHERE [CustomerNumber] = [SearchHistory].[CustomerNumber]
    AND [ExcludeFromSearch] = 1
)
GROUP BY DATENAME(WEEKDAY, [AtDateTime]), DATEPART(WEEKDAY, [AtDateTime])
ORDER BY DATEPART(WEEKDAY, [AtDateTime])

Any advice/critique is welcome.

DDL for the SearchHistory table:

CREATE TABLE [dbo].[SearchHistory](
    [CustomerNumber] [char](8) NOT NULL,
    [Username] [varchar](16) NOT NULL,
    [AtDateTime] [datetime2](7) NOT NULL,
    [Terms] [varchar](128) NOT NULL,
    [NumFound] [int] NOT NULL,
    [NumInStock] [int] NOT NULL
) ON [PRIMARY]
ALTER TABLE [dbo].[SearchHistory] ADD  CONSTRAINT [DF_SearchHistory_AtDateTime]  DEFAULT (sysdatetime()) FOR [AtDateTime]
GO

DDL for the CustomerExclusions table:

CREATE TABLE [dbo].[CustomerExclusions](
    [CustomerNumber] [char](8) NOT NULL,
    [ExcludeFromSearch] [bit] NOT NULL,
    [ExcludeFromErrors] [bit] NOT NULL,
    [ExcludeFromPageHits] [bit] NOT NULL,
 CONSTRAINT [PK_CustomerExclusions] PRIMARY KEY CLUSTERED 
(
    [CustomerNumber] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
) ON [PRIMARY]

The indexes on SearchHistory are as follows:

CREATE NONCLUSTERED INDEX [IX_SearchHistory_CustomerNumber_Username_AtDateTime_Terms] ON [dbo].[SearchHistory]
(
    [CustomerNumber] ASC,
    [Username] ASC,
    [AtDateTime] ASC,
    [Terms] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
GO
CREATE NONCLUSTERED INDEX [IX_SearchHistory_NumFound] ON [dbo].[SearchHistory]
(
    [NumFound] ASC
)
INCLUDE (   [CustomerNumber],
    [Terms]) WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
GO
CREATE NONCLUSTERED INDEX [IX_SearchHistory_NumFound_CustomerNumber] ON [dbo].[SearchHistory]
(
    [NumFound] ASC,
    [CustomerNumber] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, SORT_IN_TEMPDB = OFF, DROP_EXISTING = OFF, ONLINE = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
GO

The CustomerExclusions table has no indexes on it, apart from the PK.

This is for SQL Server 2012.

There are two records in CustomerExclusions, and almost 22k records in SearchHistory. When both filters are applied, both records in CustomerExclusions match ExcludeFromSearch = 1, and ~17k records in SearchHistory are returned when the CustomerExclusions filter is applied.

\$\endgroup\$
  • \$\begingroup\$ Have you thought of creating UDFs for those big one-liner calculations? That would at least make them more readable, and reusable. Just a thought. \$\endgroup\$ – Phrancis Dec 30 '15 at 23:34
  • 3
    \$\begingroup\$ @Phrancis though that would possibly kill performance \$\endgroup\$ – Tom V - try topanswers.xyz Jan 4 '16 at 20:12
  • \$\begingroup\$ How long does the query currently take to execute if you run it in isolation with SET STATISTICS TIME ON? \$\endgroup\$ – Martin Smith Jan 4 '16 at 20:52
  • 1
    \$\begingroup\$ @MartinSmith SQL Server parse and compile time: CPU time = 109 ms, elapsed time = 162 ms. SQL Server Execution Times: CPU time = 0 ms, elapsed time = 0 ms. SQL Server Execution Times: CPU time = 78 ms, elapsed time = 81 ms. \$\endgroup\$ – Der Kommissar Jan 4 '16 at 20:54
  • \$\begingroup\$ And is that an issue? There are indexing changes that can be made but impossible for me to know how that will affect your overall workload. \$\endgroup\$ – Martin Smith Jan 4 '16 at 20:58
5
\$\begingroup\$

Regarding performance the code looks fine. Possibly you are missing useful indexes.

As far as the code itself is concerned I have the following observations.

Remove three part names.

[DB].[dbo].[SearchHistory] and [DB].[dbo].[CustomerExclusions]

It is common to want to run queries against different versions of the database. Hardcoding the database name in to SQL makes this harder.

If this SQL lives inside a stored proc in a copy of the database DB_copy on the same instance as DB you may be unaware that this is, in fact, referencing a different database for some time. Similarly if the SQL is sent by an application it is likely that most people will assume that changing the "Initial Catalogue" in the connection string will change the tables being queried.

Add table aliases and use them for all columns in the correlated sub query

/*Original Code*/
FROM [DB].[dbo].[SearchHistory] 
WHERE NOT EXISTS 
(
    SELECT 1 FROM [DB].[dbo].[CustomerExclusions]
    WHERE [CustomerNumber] = [SearchHistory].[CustomerNumber]
    AND [ExcludeFromSearch] = 1
)

/*Rewritten Code*/
FROM   dbo.SearchHistory sh
WHERE  NOT EXISTS (SELECT *
                   FROM   dbo.CustomerExclusions ce
                   WHERE  ce.CustomerNumber = sh.CustomerNumber
                          AND ce.ExcludeFromSearch = 1) 

If the table CustomerExclusions does not have any columns called CustomerNumber or ExcludeFromSearch but these do exist in SearchHistory then the original correlated sub query will resolve them from the outer scope giving you incorrect results rather than an error message. Using two part names as in the second example would give an error in that case as well as making the code more explicitly self documenting. I have also removed the square brackets as they are not required and I find them distracting. For reasons of personal preference I have replaced the 1 with a *. It makes no difference to the execution plan.

Other changes.

  • I used a CTE to remove some of the repeated formulas scattered around the code.
  • I replaced your use of SUM()/COUNT(*) with AVG
  • It is only necessary to cast one of the operands to avoid integer division - not both.
  • I changed the column names returned by the query so they meet the rules for standard identifiers and don't need to be quoted.
  • I changed to the non standard column_alias = expression syntax as I find it makes the query easier to read when the select list contains long expressions.

WITH CTE
     AS (SELECT AtDateTime,
                /*Use CASE if < SQL Server 2012*/
                IIF(NumFound = 0, 1, 0)       AS NoResults,
                DATENAME(WEEKDAY, AtDateTime) AS WeekDayName,
                DATEPART(WEEKDAY, AtDateTime) AS WeekDayNumber
         FROM   dbo.SearchHistory sh
         WHERE  NOT EXISTS (SELECT *
                            FROM   dbo.CustomerExclusions ce
                            WHERE  ce.CustomerNumber = sh.CustomerNumber
                                   AND ce.ExcludeFromSearch = 1))
SELECT WeekDayName,
       NumberOfSearches 
             = COUNT(*),
       AverageSearchesPerDay
             = COUNT(*) / CAST(COUNT(DISTINCT CONVERT(DATE, AtDateTime)) AS DECIMAL(10, 2)),
       NumberOfSearchesWithNoResults 
             = SUM(NoResults),
       PercentOfSearchesWithNoResults
             = CAST(AVG(CAST(NoResults AS DECIMAL(10, 2))) AS DECIMAL(10, 4))
FROM   CTE
GROUP  BY WeekDayNumber,
          WeekDayName
ORDER  BY WeekDayNumber 
\$\endgroup\$
3
\$\begingroup\$

One-liners

You have 3 fairly complex 1-liners, which include a bunch of various number functions, date functions, and conversions. I think it would make it easier to understand (and maintain) if you allowed so vertical space. This would also give you a bit of room for comments, if needed.

,CAST(
     CAST(
         SUM(CASE WHEN [NumFound] = 0 THEN 1 ELSE 0 END) /*Allows to sum up searches with no results only*/
     AS DECIMAL(10, 2))
     / @numberOfSearches
 AS DECIMAL(10, 4)) AS [Percent of Searches with no Results]

I admit it may look a bit weird at first. But look at how much clearer the expression looks as a whole.


Other than that, it looks good. It's consistent and there is really not a whole lot more to be said, in my opinion, until you put it into the context of a bigger stored procedure or view. Kudos!

\$\endgroup\$

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.