Pivot script for sales numbers

Question

I've recently been tasked with migrating some code stacks for an applicatin that is moving from using Access to using SQL-Server.

I don't usually use PIVOT all that much, and most of my code is scripting intended for in-house administration of our SQL Servers. As such my code is not that great.

How could I clean up this code, and make it easier to maintain? My main concern is, I want this simpler, but how?

USE [Database1]
GO
SELECT
       Region,
       Year,
       Month,
       SalesPersonNR,
       (ISNULL(SUM(SalesA),0) + ISNULL(SUM(SalesB),0) + ISNULL(SUM(SalesC),0) + ISNULL(SUM(Sales),0) + ISNULL(SUM(NA),0)) AS Total,
       SUM(Sales) Sales,
       SUM(SalesA) SalesA,
       SUM(SalesB) SalesB,
       SUM(SalesC) SalesC
FROM
(
SELECT 
       Region,
       Year,
       Month,
       SalesPersonNR,
       COUNT(SalesNumber) as total,
       expr1
FROM (
       SELECT A.Region,
       Year([Date]) AS Year,
       Month([Date]) AS Month,
       A.SalesPersonNR,
       A.SalesNumber AS SalesNumber,
       CASE 
             WHEN B.DateSalesC IS NOT NULL THEN 'SalesC'
             WHEN B.DateSalesB IS NOT NULL THEN 'SalesB'
             WHEN B.DateSalesA IS NOT NULL THEN 'SalesA'
             WHEN B.DateSales IS NOT NULL THEN 'Sales'
             ELSE 'NA'
       END AS expr1
FROM [Table1] A 
       JOIN [Table2] B 
       ON A.SalesNumber = B.SalesNumber 
WHERE 
       A.Status = 99 
       ) Internal
GROUP BY 
       Region,
       Year,
       Month,
       SalesPersonNR,
       expr1
) AS x
PIVOT
(
       SUM(Total)
       FOR expr1 IN (SalesC, SalesB, SalesA, Sales, NA)
       ) AS y
GROUP BY 
Region,
Year,
Month,
SalesPersonNR

Answer 1

A few things that I noticed:

Nesting

You're nesting selects two levels deep. That's usually a bad sign for most of the data mining and report generation I had to do. You might be able to get rid of the innermost Select with some clever thinking.

This may greatly improve performance for that query.

Use a CTE

You can possibly simplify the second select into a "Common Table Expression" (CTE) like so:

WITH (SELECT 
    Region
    , Year
    , Month
    , SalesPersonNR
    , COUNT(SalesNumber) as total
    , expr1
FROM  -- ...

Formatting

As you may have noticed above I personally prefer the leading comma style for column specifications. I find it easier to maintain, because you can just delete a single row and can be sure the SELECT (or whatever else) is still valid.

Additionally I strongly recommend you indent your Statements according to the level of nesting they're in.

Usually I follow the folllowing structure

SELECT col1
    , col2
FROM tableName
JOIN otherTable ON tableName.id = otherTable.foreign
WHERE condition
    && condition
HAVING condition
    || condition
GROUP BY col1
    , col2
ORDER BY col1

Any time you increase the level of nesting, I'd increase the indent, too, to make it easier to follow.

Naming

Your table aliases are .. suboptimal. I assume this may be because you anonymized them, but A and B are really not great table names.

Same applies to expr1 and x

Magic numbers

What's the idea behind Status 99? If I read that query I have no idea which sales you're actually selecting. Drop a comment there (or even run a self-documenting select onto a named table)

Answer 2

@Vogel612 posted a great answer and I wanted to expand on the Common Table Expression (CTE) part, as I find these types of expressions are powerful in clarifying the intention of queries, and spotting redundancies and inefficiencies.

Your 2 subqueries could be moved to the top (I made a few small changes to them as well, see code comments throughout):

USE [Database1]
GO
DECLARE @someMeaningfulStatusName INT = 99;
WITH Internal AS (
    SELECT 
        A.Region,
        Year([Date]) AS [Year],     -- best to quote these aliases since they are also SQL keywords
        Month([Date]) AS [Month],
        A.SalesPersonNR,
        A.SalesNumber AS SalesNumber,
        CASE 
            WHEN B.DateSalesC IS NOT NULL THEN 'SalesC'
            WHEN B.DateSalesB IS NOT NULL THEN 'SalesB'
            WHEN B.DateSalesA IS NOT NULL THEN 'SalesA'
            WHEN B.DateSales IS NOT NULL THEN 'Sales'
            ELSE 'NA'
            END AS SalesIdentifier -- original alias `expr1`
    FROM [Table1] A 
        JOIN [Table2] B ON A.SalesNumber = B.SalesNumber 
    WHERE 
        A.Status = @someMeaningfulStatusName
), 
Counted AS ( -- original alias `x`
    SELECT 
        Region,
        Year,
        Month,
        SalesPersonNR,
        COUNT(SalesNumber) as total,
        SalesIdentifier
    FROM Internal
    GROUP BY
        Region,
        Year,
        Month,
        SalesPersonNR,
        SalesIdentifier
)
SELECT ...
FROM Counted
PIVOT (
    SUM(Total)
    FOR expr1 IN (SalesC, SalesB, SalesA, Sales, NA)
) ...

It already reads quite a bit better without changing the way it works. Now as we inspect the two CTEs, it becomes clear that we are querying a lot of the same fields twice, as far as I can tell for the sake of being able to group by the inner query's column names.

We could optimize that away by instead grouping by the column expressions themselves. As such, the two queries could almost certainly be merged into one, like so:

WITH Aggregated AS ( --note the alias name change to reflect the purpose better
    SELECT 
        A.Region,
        Year([Date]) AS [Year],  
        Month([Date]) AS [Month],
        A.SalesPersonNR,
        COUNT(A.SalesNumber) AS Total,
        CASE 
            WHEN B.DateSalesC IS NOT NULL THEN 'SalesC'
            WHEN B.DateSalesB IS NOT NULL THEN 'SalesB'
            WHEN B.DateSalesA IS NOT NULL THEN 'SalesA'
            WHEN B.DateSales IS NOT NULL THEN 'Sales'
            ELSE 'NA' END AS SalesIdentifier -- original alias `expr1`
    FROM [Table1] AS A 
        JOIN [Table2] AS B ON A.SalesNumber = B.SalesNumber 
    WHERE 
        A.Status = @someMeaningfulStatusName
    GROUP BY 
        -- can't use column aliases here, use expressions instead:
        A.Region,
        Year([Date]),
        Month([Date]),
        A.SalesPersonNR,
        --Copy of CASE expression for SalesIdentifier:
        CASE 
            WHEN B.DateSalesC IS NOT NULL THEN 'SalesC'
            WHEN B.DateSalesB IS NOT NULL THEN 'SalesB'
            WHEN B.DateSalesA IS NOT NULL THEN 'SalesA'
            WHEN B.DateSales IS NOT NULL THEN 'Sales'
            ELSE 'NA' END
)
SELECT ...
FROM Aggregated
PIVOT (
    SUM(Total)
    FOR expr1 IN (SalesC, SalesB, SalesA, Sales, NA)
) ...

Now, there is one thing that is a bit odd-looking there: GROUP BY ... CASE WHEN .. THEN... expression. While there is nothing wrong per-se in doing that, and it should be quite fast, we could use a CTE just for that CASE expression (although, this will be likely less performant due to additional query). Your call.