Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was tasked today with cleaning up data from a SSRS report put together by a DB programmer. The original report was to find duplicate serial number sales within a user-defined period of time. I was given an Excel file with the result data set and it smelled really funny, so I decided to go into SSRS and find out what the DBA did.

Turns out the person who requested the report did not explain it very well, and our data services team was overworked, so the result set was bogus and the report took over 30 minutes to execute. It looked like a desperate attempt to make a self-join-ish operation and had SELECT DISTINCT * which threw a big red flag.

So I decided to rewrite it.

Background

  1. A certain number of orders were processed as duplicates resulting in multiple coverage sales for the same unit, and invoicing problems etc.

  2. We wanted to sort out manually cancelled orders (Status = 'CAN') from deleted orders (Status = 'DEL'), with the deleted ones being what we were looking for, as deleted orders don't show up in any of the invoicing, service, etc. applications so they are more difficult to track down (by design).

  3. The report below runs in about 3 minutes. I know nothing about XML (so please don't crucify me, and I didn't write that code either) but I suspect the hundreds and hundreds of lines of formatting after the SQL query might bog it down. Unfortunately I have no way to benchmark it against SSMS performance with just the SQL, as I only have access to SSRS. Any advice on cleaning up redundant/unnecessary XML is welcome.

So here goes. Any tips on performance, best practices, etc. appreciated!

Original report: approx 550 rows returned
My updated version: 120 rows returned (for May '14)

<?xml version="1.0" encoding="utf-8"?>
<Report xmlns:rd="http://schemas.microsoft.com/SQLServer/reporting/reportdesigner" xmlns:cl="http://schemas.microsoft.com/sqlserver/reporting/2010/01/componentdefinition" xmlns="http://schemas.microsoft.com/sqlserver/reporting/2010/01/reportdefinition">
  <AutoRefresh>0</AutoRefresh>
  <DataSources>
    <DataSource Name="DataSource1">
      <DataSourceReference>/Data Sources/Reporting</DataSourceReference>
      <rd:SecurityType>None</rd:SecurityType>
      <rd:DataSourceID> REMOVED </rd:DataSourceID>
    </DataSource>
  </DataSources>
  <DataSets>
    <DataSet Name="DataSet1">
      <Query>
        <DataSourceName>DataSource1</DataSourceName>
        <QueryParameters>
          <QueryParameter Name="@startdate">
            <Value>=Parameters!startdate.Value</Value>
          </QueryParameter>
          <QueryParameter Name="@enddate">
            <Value>=Parameters!enddate.Value</Value>
          </QueryParameter>
        </QueryParameters>
        <CommandText>

USE Reporting;

SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;

--DECLARE @StartDate DATETIME;
--DECLARE @EndDate DATETIME;

-- Extract the list of serials for the desired accounts into a temp table
IF OBJECT_ID('tempdb..#ContractDupSerial') IS NOT NULL DROP TABLE #ContractDupSerial;

SELECT
    cdo.Serial_Number,
    cdo.Model_Number,
    cmo.Contract_Number,
    cmo.Dealer_ID,
    cmo.Status,
    cmo.Status_Date,
    cmo.CSR_ID
INTO #ContractDupSerial
FROM
Servicer_Reporting.dbo.Contract_Master_Original AS cmo
INNER JOIN Servicer_Reporting.dbo.contract_detail_original AS cdo
    ON cdo.Contract_Number = cmo.Contract_Number
    AND cdo.Data_Source = cdo.Data_Source
INNER JOIN Servicer_Reporting.dbo.Dealer_Handling AS dh
    on dh.Dealer_ID = cmo.Dealer_ID
WHERE
dh.Account_ID IN (102,108,103,107,138,139,142,147,148,162,163,169,170)
AND cmo.Creation_Date &gt;= @startdate
AND cmo.Creation_Date &lt; @enddate

-- Filter to get only records where duplicate serials exist

SELECT     
    c.Serial_Number,
    c.Model_Number,
    c.Contract_Number,
    c.Dealer_ID,
    c.Status,
    c.Status_Date,
    c.CSR_ID
FROM #ContractDupSerial AS c
-- Clean up bogus data
WHERE c.Serial_Number IS NOT NULL
AND c.Serial_Number NOT LIKE 'N/A%'
AND c.Serial_Number NOT LIKE 'NA%'
AND c.Serial_Number NOT LIKE 'X%'
AND c.Serial_Number NOT LIKE '0%'
AND c.Serial_Number NOT LIKE ''
AND c.Serial_Number NOT LIKE '.'
-- Filter out manual cancellations and entries
AND c.Serial_Number NOT IN
    (
    SELECT c.Serial_Number
    FROM #ContractDupSerial AS c
    WHERE c.CSR_ID IS NOT NULL
    )
-- Filter in duplicate serial numbers
AND c.Serial_Number IN
    (
    SELECT c.Serial_Number
    FROM #ContractDupSerial AS c
    GROUP BY c.Serial_Number
    HAVING COUNT(c.Serial_Number) > 1
    )
ORDER BY c.Serial_Number ASC, c.Contract_Number ASC;


</CommandText>
        <rd:UseGenericDesigner>true</rd:UseGenericDesigner>
      </Query>
      <Fields>
        <Field Name="Serial_Number">
          <DataField>Serial_Number</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
        <Field Name="Model_Number">
          <DataField>Model_Number</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
        <Field Name="Contract_Number">
          <DataField>Contract_Number</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
        <Field Name="Dealer_ID">
          <DataField>Dealer_ID</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
        <Field Name="Status">
          <DataField>Status</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
        <Field Name="Status_Date">
          <DataField>Status_Date</DataField>
          <rd:TypeName>System.Decimal</rd:TypeName>
        </Field>
        <Field Name="CSR_ID">
          <DataField>CSR_ID</DataField>
          <rd:TypeName>System.String</rd:TypeName>
        </Field>
      </Fields>
    </DataSet>
  </DataSets>
  <ReportSections>
    <ReportSection>
      <Body>
        <ReportItems>
          <Tablix Name="Tablix1">
            <TablixBody>
              <TablixColumns>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
                <TablixColumn>
                  <Width>1in</Width>
                </TablixColumn>
              </TablixColumns>
              <TablixRows>
                <TablixRow>
                  <Height>0.25in</Height>
                  <TablixCells>
                    <TablixCell>
                      <CellContents>
                        <Textbox Name="Textbox3">
                          <CanGrow>true</CanGrow>
                          <KeepTogether>true</KeepTogether>
                          <Paragraphs>
                            <Paragraph>
                              <TextRuns>
                                <TextRun>
                                  <Value>Dealer ID</Value>
                                  <Style>
                                    <FontSize>11pt</FontSize>
                                    <FontWeight>Bold</FontWeight>
                                    <Color>White</Color>
                                  </Style>
                                </TextRun>
                              </TextRuns>
                              <Style />
                            </Paragraph>
                          </Paragraphs>
                          <rd:DefaultName>Textbox3</rd:DefaultName>
                          <Style>
                            <Border>
                              <Color>#4e648a</Color>
                              <Style>Solid</Style>
                            </Border>
                            <BackgroundColor>#384c70</BackgroundColor>
                            <PaddingLeft>2pt</PaddingLeft>
                            <PaddingRight>2pt</PaddingRight>
                            <PaddingTop>2pt</PaddingTop>
                            <PaddingBottom>2pt</PaddingBottom>
                          </Style>
                        </Textbox>
                      </CellContents>
                    </TablixCell>
                    <TablixCell>
                      <CellContents>
                        <Textbox Name="Textbox4">
                          <CanGrow>true</CanGrow>
                          <KeepTogether>true</KeepTogether>
                          <Paragraphs>
                            <Paragraph>
                              <TextRuns>
                                <TextRun>
                                  <Value>Contract Number</Value>
                                  <Style>
                                    <FontSize>11pt</FontSize>
                                    <FontWeight>Bold</FontWeight>
                                    <Color>White</Color>
                                  </Style>
                                </TextRun>
                              </TextRuns>
                              <Style />
                            </Paragraph>
                          </Paragraphs>
                          <rd:DefaultName>Textbox4</rd:DefaultName>
                          <Style>
                            <Border>
                              <Color>#4e648a</Color>
                              <Style>Solid</Style>
                            </Border>
                            <BackgroundColor>#384c70</BackgroundColor>
                            <PaddingLeft>2pt</PaddingLeft>
                            <PaddingRight>2pt</PaddingRight>
                            <PaddingTop>2pt</PaddingTop>
                            <PaddingBottom>2pt</PaddingBottom>
                          </Style>
                        </Textbox>
                      </CellContents>
                    </TablixCell>

<!-- CONTINUE FOR HUNDREDS OF LINES -->

      </Page>

    </ReportSection>
  </ReportSections>
  <ReportParameters>
    <ReportParameter Name="startdate">
      <DataType>DateTime</DataType>
      <Prompt>Contract Create Start Date:</Prompt>
    </ReportParameter>
    <ReportParameter Name="enddate">
      <DataType>DateTime</DataType>
      <Prompt>Contract Create End Date:</Prompt>
    </ReportParameter>
  </ReportParameters>
  <rd:ReportUnitType>Inch</rd:ReportUnitType>
  <rd:ReportServerUrl> REMOVED </rd:ReportServerUrl>
  <rd:ReportID> REMOVED </rd:ReportID>
</Report>
share|improve this question
    
So there's a discrepancy between your results and your DBA's query's results? Which one is right? –  200_success Jun 19 at 3:43
    
What are your keys (or unique constraints) for the Contract_Master_Original and contract_detail_original tables? –  200_success Jun 19 at 3:57
    
Re: discrepancy: Mine is right as far as anyone can tell. His had weird anomalies like multiple records with the same Contract_Number but different serial/model/etc. and looking up individual records using our main business application, results were inconsistent. –  Phrancis Jun 19 at 6:06
    
Re: keys/constraints. cmo.Contract_Number is the most basic primary key in our RDBMS. cmo largely contains foreign keys to other tables, e.g. dealer, customer, pricing, etc. along with a bit of data related to that contract, mostly dates. cdo is similar to an invoice detail, where cmo.Contract_Number is part of a composite key along with a cdo.Sequence_Number. –  Phrancis Jun 19 at 6:09
add comment

2 Answers 2

up vote 5 down vote accepted

I'm going to focus on the SQL in your question.

While it's easier to let the machine define temporary tables for you, it is generally better to define the #temp tables yourself. Avoid the Select Into syntax if you know the datatypes. There are several benefits to this.

  1. We can add primary keys and indexes to the temp table to help improve performance.
  2. Mr. Maintainer now has all of your knowledge about the incoming datatypes and constraints on the table.
  3. We only have to bring in the data that we absolutely need downstream. There will be no wasted memory or io.

Of course, this means we have to use the stricter Insert Into...Values() syntax. I think it's worth the time though.

A couple of other notes about the creation of the #ContractDupSerial table.

  • The table aliases are over abbreviated. Aliases are great, but like variables, should have meaningful names.
  • Mr. Maintainer desperately needs a comment near the WHERE IN statement. What accounts do those Account_IDs represent?
  • The capitalization is inconsistent. (nitpick)
  • I don't like your indentation, but this is mostly a matter of personal preference. I've never seen a standard for indentation in SQL.

Please note that I do like that you're not using that Devil Between for your date logic.

This is how I would write it:

CREATE TABLE #ContractDupSerial (
    /*
        table definition here
    --*/

)

INSERT INTO #ContractDupSerial(
    Serial_Number,
    Model_Number,
    Contract_Number,
    Dealer_ID,
    Status,
    Status_Date,
    CSR_ID
    )
SELECT
    CDetail.Serial_Number,
    CDetail.Model_Number,
    CMaster.Contract_Number,
    CMaster.Dealer_ID,
    CMaster.Status,
    CMaster.Status_Date,
    CMaster.CSR_ID
FROM Servicer_Reporting.dbo.Contract_Master_Original AS CMaster
INNER JOIN Servicer_Reporting.dbo.contract_detail_original AS CDetail
    ON CDetail.Contract_Number = CMaster.Contract_Number
    AND CDetail.Data_Source = CDetail.Data_Source
INNER JOIN Servicer_Reporting.dbo.Dealer_Handling AS Dealer
    ON Dealer.Dealer_ID = CMaster.Dealer_ID 
WHERE Dealer.Account_ID IN ( --The Smith & Johnson accounts
        102,108,103,107,138,139,142,147,148,162,163,169,170
        )
    AND CMaster.Creation_Date &gt;= @startdate
    AND CMaster.Creation_Date &lt; @enddate

The only other thing I notice is this.

AND c.Serial_Number NOT LIKE 'N/A%'
AND c.Serial_Number NOT LIKE 'NA%'
AND c.Serial_Number NOT LIKE 'X%'
AND c.Serial_Number NOT LIKE '0%'
AND c.Serial_Number NOT LIKE ''
AND c.Serial_Number NOT LIKE '.'

Like statements are expensive. I don't think it will make a difference in the query plan for this particular case (it's already scanning c.Serial_Number anyway), but the last two should use an equals. At the least, it's more semantically correct.

AND c.Serial_Number <> ''
AND c.Serial_Number <> '.'

I think that overall you have a pretty elegant solution here though. I've been trying to think about how I would self-join the table for this query and I don't like what I've come up with. I'll be tucking your pattern into the back of my mind for future reference.

share|improve this answer
    
I'll rewrite the query once I get a chance to tinker around with it, very good comments IMO. The DB design is pretty bad so I could use any amount of improvement on queries against it –  Phrancis Jun 19 at 3:21
add comment

I noticed one suspicious join condition: cdo.Data_Source = cdo.Data_Source. That should always be true, unless both sides are NULL. Did you mean to say cdo.Data_Source IS NOT NULL? Or was that join condition totally superfluous? (I'll assume the latter.)

Temporary tables are rarely a good idea, I think. They involve a lot of I/O, and they force a complex query to be executed in a certain way, rather than letting the query optimizer decide the join order.

Instead, I suggest using Common Table Expressions to manage complex queries.

WITH Relevant_Dealers AS (
    SELECT Dealer_Id
        FROM Servicer_Reporting.dbo.Dealer_Handling
        WHERE Account_ID IN (102,108,103,107,138,139,142,147,148,162,163,169,170)
), Relevant_Contracts AS (     -- Your #ContractDupSerial
    SELECT cdo.Serial_Number
         , cdo.Model_Number
         , cmo.Contract_Number
         , cmo.Dealer_ID
         , cmo.Status
         , cmo.Status_Date
         , cmo.CSR_ID
        FROM Servicer_Reporting.dbo.Contract_Master_Original AS cmo
            INNER JOIN Servicer_Reporting.dbo.contract_detail_original AS cdo
                ON cdo.Contract_Number = cmo.contract_Number
        WHERE cmo.Dealer_Id IN (SELECT Dealer_Id FROM Relevant_Dealers)
            AND cmo.Creation_Date >= @startdate
            AND cmo.Creation_Date < @enddate
), Dup_SN AS (
    SELECT Serial_Number
        FROM Relevant_Contracts
        GROUP BY Serial_Number
        HAVING COUNT(*) > 1
), Canceled_Contracts AS (
    SELECT Serial_Number
        FROM Relevant_Contracts
        WHERE CSR_ID IS NOT NULL
)
SELECT *
    FROM Relevant_Contracts
    WHERE Serial_Number IS NOT NULL
        AND Serial_Number IN (SELECT Serial_Number FROM Dup_SN)
        AND Serial_Number NOT IN (SELECT Serial_Number FROM Canceled_Contracts)
        -- Clean up bogus data...
        AND Serial_Number <> ''
        AND Serial_Number <> '.'
        AND Serial_Number NOT LIKE 'N/A%'
        AND Serial_Number NOT LIKE 'NA%'
        AND Serial_Number NOT LIKE 'X%'
        AND Serial_Number NOT LIKE '0%';
    ORDER BY Serial_Number ASC, Contract_Number ASC;
share|improve this answer
    
That JOIN was actually leftover from the DB programmer's original query, I didn't even really pay attention to it until you mentioned it. Now one reason (I may be wrong on this, mind you) I used a temp table instead of CTE is that the first query is ran against a VERY LARGE data set (over 1 billion records just in dbo.Contract_Master alone) –  Phrancis Jun 19 at 5:56
2  
I'd be very curious to know how the performance of the CTEs vs. the temp table. –  200_success Jun 19 at 5:58
    
Me as well. I like the way you wrote it, I'll certainly give it a try when I'm able to. –  Phrancis Jun 19 at 5:59
1  
Makes three of us. Please do let us know. –  ckuhn203 Jun 20 at 1:24
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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