Help me refactor this monster of a query

Question

This is one huge monster, it's going into a SP so variables are usable:

SELECT OwnerName, SUM(AmountPaid) AS Paid, SUM(AmountOwedComplete) AS Owed, SUM(AmountOwedThisMonth) AS OwedMonth,
 SUM(PaidForPast) AS PaidPast, SUM(PaidForPresent) AS PaidPresent, SUM((AmountPaid - PaidForPast - PaidForPresent)) AS PaidFuture, [Description] FROM (
 SELECT OwnerName, AmountPaid, AmountOwedComplete, AmountOwedThisMonth, PaidForPast, [Description],
  (SELECT CASE WHEN (AmountPaid - PaidForPast) < ABS(AmountOwedThisMonth) THEN AmountPaid - PaidForPast
   ELSE ABS(AmountOwedThisMonth) END) AS PaidForPresent
 FROM (
  SELECT OwnerName, AmountPaid, AmountOwedTotal - AmountPaid AS AmountOwedComplete,
   AmountOwedThisMonth, 
   (SELECT CASE WHEN (AmountPaid < ABS((AmountOwedTotal - AmountPaid)) + AmountOwedThisMonth)
    THEN AmountPaid ELSE ABS((AmountOwedTotal - AmountPaid)) + AmountOwedThisMonth END) AS PaidForPast, 
   Description, TransactionDate
   FROM (
   SELECT DISTINCT t.TenantName, p.PropertyName, ISNULL(p.OwnerName, 'Uknown') AS OwnerName, (
    SELECT SUM(Amount) FROM tblTransaction WHERE 
     Amount > 0 AND TransactionDate >= @StartDate AND TransactionDate <= @EndDate
     AND TenantID = t.ID AND TransactionCode = trans.TransactionCode
   ) AS AmountPaid, (
    SELECT SUM(Amount) FROM tblTransaction WHERE 
     tblTransaction.TransactionCode = trans.TransactionCode AND tblTransaction.TenantID = t.ID
   )  AS AmountOwedTotal, (
    SELECT SUM(Amount) FROM tblTransaction WHERE  tblTransaction.TransactionCode = trans.TransactionCode AND tblTransaction.TenantID = t.ID
     AND Amount < 0 AND TransactionDate >= @StartDate AND TransactionDate <= @EndDate
   ) AS AmountOwedThisMonth, code.Description, trans.TransactionDate FROM tblTransaction trans 
   LEFT JOIN tblTenantTransCode code ON code.ID = trans.TransactionCode
   LEFT JOIN tblTenant t ON t.ID = trans.TenantID
   LEFT JOIN tblProperty p ON t.PropertyID  = p.ID
   WHERE trans.TransactionDate >= @StartDate AND trans.TransactionDate <= @EndDate AND trans.Amount > 0
  ) q
 ) q2
)q3
GROUP BY OwnerName, Description

This is what it does. It visits all the Tenants and gets what they have paid for this month, and everything they owe. Then it calculates what is paid for previous charges, this month, and future charges. It then sums them based on Description of the charge, and the Name of the property owner.

It looks horrible, and I want to know if there are some shortcuts I'm missing that could be used.

Answer 1

First, learn to format SQL so you (and I) can read it:

SELECT OwnerName, 
       SUM(AmountPaid) AS Paid, 
       SUM(AmountOwedComplete) AS Owed, 
       SUM(AmountOwedThisMonth) AS OwedMonth,
       SUM(PaidForPast) AS PaidPast, 
       SUM(PaidForPresent) AS PaidPresent, 
       SUM((AmountPaid - PaidForPast - PaidForPresent)) AS PaidFuture, 
       [Description] 
  FROM (SELECT OwnerName, 
               AmountPaid, 
               AmountOwedComplete, 
               AmountOwedThisMonth, 
               PaidForPast, 
               [Description],
               (SELECT CASE WHEN (AmountPaid - PaidForPast) < ABS(AmountOwedThisMonth) 
                            THEN AmountPaid - PaidForPast
                       ELSE ABS(AmountOwedThisMonth) END) AS PaidForPresent
          FROM (SELECT OwnerName, 
                       AmountPaid, 
                       AmountOwedTotal - AmountPaid AS AmountOwedComplete,
                       AmountOwedThisMonth, 
                       (SELECT CASE WHEN (AmountPaid < ABS((AmountOwedTotal - AmountPaid)) + AmountOwedThisMonth)
                                    THEN AmountPaid 
                               ELSE ABS((AmountOwedTotal - AmountPaid)) + AmountOwedThisMonth END) AS PaidForPast,     
                               Description, 
                               TransactionDate
                          FROM (SELECT DISTINCT 
                                       t.TenantName, 
                                       p.PropertyName, 
                                       ISNULL(p.OwnerName, 'Uknown') AS OwnerName, 
                                       (SELECT SUM(Amount) 
                                          FROM tblTransaction 
                                         WHERE Amount > 0 
                                           AND TransactionDate >= @StartDate 
                                           AND TransactionDate <= @EndDate
                                           AND TenantID = t.ID 
                                           AND TransactionCode = trans.TransactionCode) AS AmountPaid, 
                                       (SELECT SUM(Amount) 
                                          FROM tblTransaction 
                                         WHERE tblTransaction.TransactionCode = trans.TransactionCode 
                                           AND tblTransaction.TenantID = t.ID)  AS AmountOwedTotal, 
                                       (SELECT SUM(Amount) 
                                          FROM tblTransaction 
                                         WHERE tblTransaction.TransactionCode = trans.TransactionCode 
                                           AND tblTransaction.TenantID = t.ID
                                           AND Amount < 0 
                                           AND TransactionDate >= @StartDate 
                                           AND TransactionDate <= @EndDate) AS AmountOwedThisMonth, 
                                       code.Description, 
                                       trans.TransactionDate 
                                  FROM tblTransaction trans 
                                  LEFT JOIN tblTenantTransCode code ON code.ID = trans.TransactionCode
                                  LEFT JOIN tblTenant t ON t.ID = trans.TenantID
                                  LEFT JOIN tblProperty p ON t.PropertyID  = p.ID
                                 WHERE trans.TransactionDate >= @StartDate 
                                   AND trans.TransactionDate <= @EndDate 
                                   AND trans.Amount > 0) q
               ) q2
       ) q3
 GROUP BY OwnerName, Description;

Second, make sure you have the right indexes -- you'll need to be able to read the SQL to do this.

Answer 2

The killer here is the PaidForPast (and derived PaidForPresent) calculation, which seems to be trying to indicate whether or not a tenant paid his balance is full (if I understand correctly - it's not easy). This calculation literally has to be performed for every single payment transaction and depends on another aggregate derived from the entire tenant history, which guarantees an O(n^2) operation.

This is a cruel, cruel thing to do to your database, and while I might be able to make your query look prettier, there's no way for you to extricate yourself from the performance problem as long as you are forced to produce that information based on the specific set of available data that this query implies.

What you have here is not a refactoring/optimization problem but a serious design problem. In fact you have two problems. The smaller problem is that you are encoding business logic into your data layer; the larger problem is that the system has not been designed to keep track of all the information that it actually needs.

Basically all of the information you are generating in this query should be kept in some kind of A/R history table that records these aggregates/statistics for each and every transaction. That table could be maintained by the app itself or by a trigger on your tblTransaction table.

Probably not the answer you're looking for, but I actually was about halfway through refactoring this when I realized that it wasn't going to be possible to remove the PaidForXYZ nesting.

In fact the fastest way to produce these results will probably be with a cursor, because then you can work with the partial aggregates and turn it into an O(n) operation. But do you really want a cursor running over an entire transaction table with no filter?

---

Update:

If you really don't care about performance and just want to clean it up so it's easier to read/maintain, here's the best I could come up with using some CTEs:

;WITH Transactions_CTE AS
(
    SELECT
        TenantID,
        TransactionCode,
        Amount,
        CASE
            WHEN Amount > 0 AND TransactionDate BETWEEN @BeginDate AND @EndDate
                THEN Amount
            ELSE 0
        END AS AmountPaid,
        CASE
            WHEN Amount < 0 AND TransactionDate BETWEEN @BeginDate AND @EndDate
                THEN Amount
            ELSE 0
        END AS AmountOwed,
    FROM tblTransaction
),
Summary_CTE AS
(
    SELECT
        t.PropertyID,
        tr.TransactionCode,
        SUM(tr.Amount) AS CumulativeBalance,
        SUM(tr.AmountPaid) AS CurrentPaid,
        SUM(tr.AmountOwed) AS CurrentOwed
    FROM Transactions_CTE tr
    INNER JOIN tblTenant t ON tr.TenantID = t.ID
    GROUP BY t.PropertyID, tr.TransactionCode
),
Past_CTE AS
(
    SELECT
        PropertyID, TransactionCode,
        CumulativeBalance, CurrentPaid, CurrentOwed,
        CASE
            WHEN CurrentPaid < 
              ABS(CumulativeBalance - CurrentPaid) + CurrentOwed
            THEN CurrentPaid
            ELSE ABS(CumulativeBalance - CurrentPaid) + CurrentOwed
        END AS PaidForPast
    FROM Summary_CTE
),
Present_CTE AS
(
    SELECT
        PropertyID, TransactionCode,
        CumulativeBalance, CurrentPaid, CurrentOwed,
        PaidForPast,
        CASE
            WHEN (CurrentPaid - PaidForPast) < ABS(CurrentOwed)
            THEN CurrentPaid - PaidForPast
            ELSE ABS(CurrentOwed)
        END AS PaidForPresent
     FROM Past_CTE
)
SELECT
    ISNULL(p.OwnerName, 'UNKNOWN') AS OwnerName,
    c.[Description],
    CumulativeBalance, CurrentPaid, CurrentOwed,
    CumulativeBalance - CurrentPaid AS CumulativeOwed,
    PaidForPast, PaidForPresent,
    CurrentPaid - PaidForPast - PaidForPresent AS PaidForFuture,
    [Description]
FROM Present_CTE s
LEFT JOIN tblProperty p ON p.ID = s.PropertyID
LEFT JOIN tblTenantTransCode c ON c.ID = s.TransactionCode

You could eliminate the last two CTEs by writing out the entire PaidForXYZ expressions in full as opposed to building one on top of the other, but IMO this would just end up making it less readable, and I'm pretty sure the optimizer will figure it out and map it all into expressions instead of subqueries.

Also, I have to say that all of those ABS blocks make me suspicious. I don't know the details of what's going on here, but it feels like those are being used in place of a negation operator, possibly one that should have been used further upstream (i.e. to convert either debits or credits into negative amounts) and this could lead to some subtle bugs down the road.

Answer 3

Do you have any say in the schema? As it stands, it looks like you're attempting to generate a report off of an operational data store which is transactional in nature. In many high-volume scenarios, it's not uncommon to create a less-normalized schema known as a Decision Support database which your transactional data is copied/summarized to on a particular interval. You then can write pretty simplistic queries against the DSS while your highly-normalized ODS continues chugging along.