Recent PostsRecent Posts Popular TopicsPopular Topics
 Home Search Members Calendar Who's On

 Convert the stored procedure to "Standard SQL Select" Rate Topic Display Mode Topic Options
Author
 Message
 Posted Saturday, April 12, 2014 11:31 AM
 SSCertifiable Group: General Forum Members Last Login: Today @ 1:59 PM Points: 6,573, Visits: 17,293
 Lynn Pettis (4/12/2014)There is another way, look at the last Fiscal Year calculation:`WITH SAMPLE_DATE(XDATE) AS( SELECT CONVERT(DATETIME2(0),XDATE,120) AS XDATE FROM (VALUES ('2007-11-22 00:00:00.000'),('2007-10-03 00:00:00.000') ,('2007-09-21 00:00:00.000'),('2006-11-07 00:00:00.000') ,('2008-04-29 00:00:00.000'),('2006-10-13 00:00:00.000') ,('2008-05-07 00:00:00.000'),('2008-04-05 00:00:00.000') ,('2007-08-05 00:00:00.000'),('2008-06-02 00:00:00.000') ,('2007-12-26 00:00:00.000'),('2007-09-26 00:00:00.000') ,('2008-01-31 00:00:00.000') ) AS X(XDATE))SELECT SD.XDATE ,YEAR(SD.XDATE) + SIGN(1 + SIGN(MONTH(SD.XDATE) - 11 )) AS FiscalYear ,year(dateadd(month,2,SD.XDATE)) As AlsoFiscalYear -- This looks so much cleaner IMHOFROM SAMPLE_DATE SD;`Nice And around 10% faster than my code
Post #1561228
 Posted Saturday, April 12, 2014 6:25 PM
 SSC-Forever Group: General Forum Members Last Login: Today @ 2:41 PM Points: 42,081, Visits: 39,473
 halifaxdal (4/12/2014)Thank you Jeff, I appreciate your enlightening, here is what I got so far:` SELECT B = Sum(Case When DocStatus = 'Active' Then 1 Else 0 End), C = Sum(Case When DocStatus = 'Active' and CAST(YEAR(TargetClosureDate) AS int) = dbo.fnFiscalYear(getdate()) Then 1 Else 0 End), D = Sum(Case When DocStatus = 'Active' and CAST(YEAR(TargetClosureDate) AS int) > dbo.fnFiscalYear(getdate()) Then 1 Else 0 End), A = Sum(Case When DocStatus = 'Retired' and CAST(YEAR(RetiredDate) AS int) = (dbo.fnFiscalYear(getdate())-1) Then 1 Else 0 End), Critical = Sum(Case When DocStatus = 'Active' and RiskRating = 4 Then 1 Else 0 End), High = Sum(Case When DocStatus = 'Active' and RiskRating = 3 Then 1 Else 0 End), Medium = Sum(Case When DocStatus = 'Active' and RiskRating = 2 Then 1 Else 0 End), Low = Sum(Case When DocStatus = 'Active' and RiskRating = 1 Then 1 Else 0 End), TBD = Sum(Case When DocStatus = 'Active' and RiskRating = 0 Then 1 Else 0 End), BCD = Sum(Case When DocStatus = 'Active' Then 1 Else 0 End) ----B + Sum(Case When DocStatus = 'Active' and CAST(YEAR(TargetClosureDate) AS int) = dbo.fnFiscalYear(getdate()) Then 1 Else 0 End) -----C + Sum(Case When DocStatus = 'Active' and CAST(YEAR(TargetClosureDate) AS int) > dbo.fnFiscalYear(getdate()) Then 1 Else 0 End) FROM Document d left outer join ITSDivision i on d.ITSDivisionID = i.ID Where i.Division = @SVP and DocType = 1`It returns all values for a specific @SVP except the DRR which is a percentage DRR = A/(B+C+D), can you help? Thanks.Heh... rush, rush, rush. No one wants to peel just one potato at a time and that's why you're having problems with the DRR calculation and why your code will run slower than it needs to. You did real well in realizing that it wasn't a COUNT that you wanted and that you wanted to do conditional SUMs of 1's, instead. That technique is known as a CROSSTAB and you can read more about it at the following two links.http://www.sqlservercentral.com/articles/T-SQL/63681/http://www.sqlservercentral.com/articles/Crosstab/65048/The reason why I accused you of not peeling one potato at a time is because you've had to do a recalculation of B, C, and D to calculate the value of the BCD column right now. As a result, your code is unneccessarily long and the extra calculations could make it run a fair bit slower unless the Optimizer recognizes those formulas as the same formulas previously calculated and just happens to reuse them.With the idea that we want to make the code do as few duplicate calculations as necessary for reasons of performance and to make the code both easier to read and troubleshoot, let's begin the simplification process...As you pointed out, your code returns all the values for a specific @SVP. Another way of saying that is that it returns all of the values for JUST ONE @SVP. If we look back at the original cursor code, @SVP isn't a parameter... it's a variable populated by the cursor and then it loops through each and every value of the ITSDivision table just to find the ITSDivisionID... WHICH IS ALREADY AVAILABLE IN THE DOCUMENT TABLE AND IT DOES A LEFT JOIN ON THE DOCUMENT TABLE. That at least implies that you want everything in the document table regardless. Also notice that the DIVISION column (presumably a name column) isn't actually used anywhere except in the final output, so let's get rid of the join to the ITSDivision table and GROUP BY the ITSDivisionID column of the document table and get rid of the join altogether. It's just not needed, yet, and there's no sense in doing the join on so many rows as what are in the Document table.Another simplification is that there's actually no need to CAST the return value of YEAR to an INT. It's guaranteed to be an INT, so casting it just burns clock cycles without adding any value.To get rid of repetative calculations, I removed the calculation for BCD. We'll add it in later just like we will for the "Division" and DRR columns... all of which will be incredibly simple because we're simplifying the base code.I also shortened the length of the code and did some nice vertical alignment to make it easier to read. We still have some more simplification and alignment to do but, if we apply the simplifications that we have so far, here's what we end up with. Note that I don't have your tables or data so I'm doing this by "feel" and I've not actually tested the code.` SELECT ITSDivisionID ,A = Sum(Case When DocStatus = 'Retired' and YEAR(RetiredDate) = (dbo.fnFiscalYear(getdate())-1) Then 1 Else 0 End) ,B = Sum(Case When DocStatus = 'Active' Then 1 Else 0 End) ,C = Sum(Case When DocStatus = 'Active' and YEAR(TargetClosureDate) = dbo.fnFiscalYear(getdate()) Then 1 Else 0 End) ,D = Sum(Case When DocStatus = 'Active' and YEAR(TargetClosureDate) > dbo.fnFiscalYear(getdate()) Then 1 Else 0 End) ,Critical = Sum(Case When DocStatus = 'Active' and RiskRating = 4 Then 1 Else 0 End) ,High = Sum(Case When DocStatus = 'Active' and RiskRating = 3 Then 1 Else 0 End) ,Medium = Sum(Case When DocStatus = 'Active' and RiskRating = 2 Then 1 Else 0 End) ,Low = Sum(Case When DocStatus = 'Active' and RiskRating = 1 Then 1 Else 0 End) ,TBD = Sum(Case When DocStatus = 'Active' and RiskRating = 0 Then 1 Else 0 End) FROM dbo.Document WHERE DocType = 1 GROUP BY d.ITSDivisionID;`The next simplification that will help this code fly is to remove the year "calculations". Simple comparisons with no calculations to the left of the relational operator can be a fair bit faster than comparisons with equations on both sides of the relational operator. We also don't need to calculate the Fiscal year more than once. If we get lucky, the Optimizer will recognize the function calculation as a "run-time constant". If we don't get so lucky, it'll be doing a whole bunch of duplicated calculations. With that in mind and assuming that we have to write this as a direct replacement for a stored procedure instead of an iTVF (Inline Table Valued Function) or a view, we can make it so that we only do a couple of calculations for fiscal year for the whole proc instead of once for each row. With the idea that we don't want to have to calculate the integer based year for multiple times for every row in the Document table, we're going to do something a little different and not use the function, at all (and there's a HUGE wakup call coming, as well).The first step in all of that is to calculate the start dates (1st of November for each respective year) for each required fiscal year... like this.`--===== Declare local variables for the start date of fiscal yearsDECLARE @FYStart DATETIME --November 1st of current FY ,@FYNextStart DATETIME --November 1st of next FY ,@FYPrevStart DATETIME --November 1st of previous FY;--===== Calculate the start date of each named fiscal year SELECT @FYStart = DateAdd(mm,-2,DateAdd(yy,DateDiff(yy,0,DateAdd(mm,2,Getdate())),0)) ,@FYNextStart = DateAdd(yy,1,@FYStart) ,@FYPrevStart = DateAdd(yy,-1,@FYStart);`Now, we can remove some of those nasty ol' CPU eating calculations from the code and replace them with simple comparisons.`--===== Declare local variables for the start date of fiscal yearsDECLARE @FYStart DATETIME --November 1st of current FY ,@FYNextStart DATETIME --November 1st of next FY ,@FYPrevStart DATETIME --November 1st of previous FY;--===== Calculate the start date of each named fiscal year SELECT @FYStart = DateAdd(mm,-2,DateAdd(yy,DateDiff(yy,0,DateAdd(mm,2,Getdate())),0)) ,@FYNextStart = DateAdd(yy, 1,@FYStart) ,@FYPrevStart = DateAdd(yy,-1,@FYStart); SELECT ITSDivisionID ,A = Sum(Case When DocStatus = 'Retired' and RetiredDate >= @FYPrevStart and RetiredDate < @FYStart Then 1 Else 0 End) ,B = Sum(Case When DocStatus = 'Active' Then 1 Else 0 End) ,C = Sum(Case When DocStatus = 'Active' and TargetClosureDate >= @FYStart and TargetClosureDate < @FYNextStart Then 1 Else 0 End) ,D = Sum(Case When DocStatus = 'Active' and TargetClosureDate >= @FYNextStart Then 1 Else 0 End) ,Critical = Sum(Case When DocStatus = 'Active' and RiskRating = 4 Then 1 Else 0 End) ,High = Sum(Case When DocStatus = 'Active' and RiskRating = 3 Then 1 Else 0 End) ,Medium = Sum(Case When DocStatus = 'Active' and RiskRating = 2 Then 1 Else 0 End) ,Low = Sum(Case When DocStatus = 'Active' and RiskRating = 1 Then 1 Else 0 End) ,TBD = Sum(Case When DocStatus = 'Active' and RiskRating = 0 Then 1 Else 0 End) FROM dbo.Document WHERE DocType = 1 AND DocStatus IN ('Active','Retired') GROUP BY ITSDivisionID;`HERE'S THE HUGE WAKEUP CALL!!!. Let's look at the way they did the date calculations in the original code...`YEAR(TargetClosureDate) = dbo.fnFiscalYear(getdate())`Do you see anything wrong with that code? Anything at all? Let's do a wee bit o' analysis. If GETDATE() is 2014-04-12, then the function will return the number 2014 as the fiscal year. All of the dates for TargetClosureDate must be in the same year of 2014. BUT, BECAUSE THEY DIDN'T APPLY THE SAME FUNCTION TO THE TARGETCLOSUREDATE COLUMN, IT'S RETURNING THE CALENDAR YEAR INSTEAD OF THE FISCAL YEAR!!!!! WHOOPS!!! How long has that problem been in place with the original code????The method that I used correctly calculates the start date of fiscal years and then compares the dates for columns like TargetClosureDate against the actual dates in a fiscal year so the calculation is actually done correctly. This is what happens when people try to take such shortcuts. NEVER take shortcuts where date ranges are involved. ALWAYS (and I don't say that word often) follow the basic format for doing the comparisons as in the following...` WHERE SomeDateColumn >= SomePeriodStartDate AND SomeDateColumn < NextPeriodStartDate`The method above is also a hidden performance enhancement because it doesn't have to calculate a value for each and ever bloody SomeDateColumn in a given table.Don'cha just love surprises? Now... how to calculate BCD, DRR, and retrieve the value of the related "Division" column WITHOUT repeating a bunch of calculations? I'm going to let you think about that a bit with the following hint... Do you know what CTEs are? --Jeff Moden"RBAR is pronounced "ree-bar" and is a "Modenism" for "Row-By-Agonizing-Row".First step towards the paradigm shift of writing Set Based code: Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column." Helpful Links:How to post code problemsHow to post performance problems
Post #1561248
 Posted Saturday, April 12, 2014 7:01 PM
 SSC Eights! Group: General Forum Members Last Login: Yesterday @ 12:50 PM Points: 941, Visits: 1,651
 Thank you guys so much and especially to Jeff for the analysis.The one with function was accepted by the Dundas and the other one with DECLARE was rejected as DECLARE is not allowed in building Virtual Table in Dundas Dashboard.And, HUGE THANKS TO YOUR FINDING OF THE TARGETCLOSUREDATE AND FISCAL YEARGood thing is the original sp is not in production yet. Yes I know CTE although I don't use it often, in Dundas I get this error for script:`WITH test (ID, Ref, Title )AS-- Define the CTE query.( SELECT ID, Ref, Title FROM Document WHERE DocStatus = 'Active' and DocType = 1 )-- Define the outer query referencing the CTE name.SELECT * FROM test`Incorrect syntax near the keyword 'WITH'.Incorrect syntax near the keyword 'with'. If this statement is a common table expression, an xmlnamespaces clause or a change tracking context clause, the previous statement must be terminated with a semicolon.Incorrect syntax near ')'.
Post #1561249
 Posted Saturday, April 12, 2014 7:13 PM
 SSC-Forever Group: General Forum Members Last Login: Today @ 2:41 PM Points: 42,081, Visits: 39,473
 halifaxdal (4/12/2014)Thank you guys so much and especially to Jeff for the analysis.The one with function was accepted by the Dundas and the other one with DECLARE was rejected as DECLARE is not allowed in building Virtual Table in Dundas Dashboard.And, HUGE THANKS TO YOUR FINDING OF THE TARGETCLOSUREDATE AND FISCAL YEARGood thing is the original sp is not in production yet. Yes I know CTE although I don't use it often, in Dundas I get this error for script:`WITH test (ID, Ref, Title )AS-- Define the CTE query.( SELECT ID, Ref, Title FROM Document WHERE DocStatus = 'Active' and DocType = 1 )-- Define the outer query referencing the CTE name.SELECT * FROM test`Incorrect syntax near the keyword 'WITH'.Incorrect syntax near the keyword 'with'. If this statement is a common table expression, an xmlnamespaces clause or a change tracking context clause, the previous statement must be terminated with a semicolon.Incorrect syntax near ')'.The CTE is wrong for the problem we're working on. That's not what's causing the error though. Look at the error. I there code before the CTE? Such code would be known as the "previous statement". That's a hint.And what the heck is "Dundas"??? If it's an application, I can't believe that such a thing wouldn't accept a DECLARE in a stored procedure (and we haven't yet finished converting this to a stored procedure). --Jeff Moden"RBAR is pronounced "ree-bar" and is a "Modenism" for "Row-By-Agonizing-Row".First step towards the paradigm shift of writing Set Based code: Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column." Helpful Links:How to post code problemsHow to post performance problems
Post #1561250
 Posted Saturday, April 12, 2014 8:06 PM
 SSC Eights! Group: General Forum Members Last Login: Yesterday @ 12:50 PM Points: 941, Visits: 1,651
 Jeff Moden (4/12/2014)halifaxdal (4/12/2014)Thank you guys so much and especially to Jeff for the analysis.The one with function was accepted by the Dundas and the other one with DECLARE was rejected as DECLARE is not allowed in building Virtual Table in Dundas Dashboard.And, HUGE THANKS TO YOUR FINDING OF THE TARGETCLOSUREDATE AND FISCAL YEARGood thing is the original sp is not in production yet. Yes I know CTE although I don't use it often, in Dundas I get this error for script:`WITH test (ID, Ref, Title )AS-- Define the CTE query.( SELECT ID, Ref, Title FROM Document WHERE DocStatus = 'Active' and DocType = 1 )-- Define the outer query referencing the CTE name.SELECT * FROM test`Incorrect syntax near the keyword 'WITH'.Incorrect syntax near the keyword 'with'. If this statement is a common table expression, an xmlnamespaces clause or a change tracking context clause, the previous statement must be terminated with a semicolon.Incorrect syntax near ')'.The CTE is wrong for the problem we're working on. That's not what's causing the error though. Look at the error. I there code before the CTE? Such code would be known as the "previous statement". That's a hint.And what the heck is "Dundas"??? If it's an application, I can't believe that such a thing wouldn't accept a DECLARE in a stored procedure (and we haven't yet finished converting this to a stored procedure).The CTE is just for testing if it can be accepted in the Dundas, it works in SQL with no problem. I talked to Dundas technical support on Friday and was replied only standard SELECT grammar can be used to create Virtual Table which is part of the procedure of creating Dashboard.Dundas accepts 3 methods creating Virtual table. 1. Based on Table 2. Based on Stored Procedure 3. Based on Manual codeIf I choose to use second method, there is no any problem using DECLARE or CTE or very complex grammar, but there are cases I want to use method 3. That's my current situation.Dundas can be seen here: dundas.comThank you again for your time and patience.
Post #1561257
 Posted Saturday, April 12, 2014 9:05 PM
 SSC-Forever Group: General Forum Members Last Login: Today @ 2:41 PM Points: 42,081, Visits: 39,473
 Pesonally, I think that option 3 is the worst (it sounds like you're setting up to write embedded code, which is a huge mistake for maintenance, IMHO). Option 2 would be the best but it's your problem. Here's how you can do option 3 and still achieve what I was talking about.`WITH cteFYStart AS (SELECT FYStart = DateAdd(mm,-2,DateAdd(yy,DateDiff(yy,0,DateAdd(mm,2,Getdate())),0))),cteFYAll AS (SELECT FYStart ,FYNextStart = DateAdd(yy,1,FYStart) ,FYPrevStart = DateAdd(yy,-1,FYStart) FROM cteFYStart) SELECT ITSDivisionID ,A = Sum(Case When d.DocStatus = 'Retired' and d.RetiredDate >= fy.FYPrevStart and d.RetiredDate < fy.FYStart Then 1 Else 0 End) ,B = Sum(Case When d.DocStatus = 'Active' Then 1 Else 0 End) ,C = Sum(Case When d.DocStatus = 'Active' and d.TargetClosureDate >= fy.FYStart and d.TargetClosureDate < fy.FYNextStart Then 1 Else 0 End) ,D = Sum(Case When d.DocStatus = 'Active' and d.TargetClosureDate >= fy.FYNextStart Then 1 Else 0 End) ,Critical = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 4 Then 1 Else 0 End) ,High = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 3 Then 1 Else 0 End) ,Medium = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 2 Then 1 Else 0 End) ,Low = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 1 Then 1 Else 0 End) ,TBD = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 0 Then 1 Else 0 End) FROM dbo.Document d CROSS JOIN cteFYAll fy WHERE DocType = 1 AND DocStatus IN ('Active','Retired') GROUP BY ITSDivisionID;` Again, the code isn't yet complete. I'm waiting for you to think outside the box a bit and make it so BCD and DRR are calculated without repeating code and to add the "Division" name column using just one more CTE. --Jeff Moden"RBAR is pronounced "ree-bar" and is a "Modenism" for "Row-By-Agonizing-Row".First step towards the paradigm shift of writing Set Based code: Stop thinking about what you want to do to a row... think, instead, of what you want to do to a column." Helpful Links:How to post code problemsHow to post performance problems
Post #1561259
 Posted Sunday, April 13, 2014 9:09 AM
 SSC Eights! Group: General Forum Members Last Login: Yesterday @ 12:50 PM Points: 941, Visits: 1,651
 Jeff Moden (4/12/2014)Pesonally, I think that option 3 is the worst (it sounds like you're setting up to write embedded code, which is a huge mistake for maintenance, IMHO). I totally agree with you on this, however Option 3 sometimes is the best approach indeed: please just think about if it takes you two weeks or longer to implement any change in production. Option 3 doesn't ask you to go through the painful process, this is the beautiful advantage! Option 2 is best in terms of development while Option 1 is the simplest one.Option 2 would be the best but it's your problem. Here's how you can do option 3 and still achieve what I was talking about.`WITH cteFYStart AS (SELECT FYStart = DateAdd(mm,-2,DateAdd(yy,DateDiff(yy,0,DateAdd(mm,2,Getdate())),0))),cteFYAll AS (SELECT FYStart ,FYNextStart = DateAdd(yy,1,FYStart) ,FYPrevStart = DateAdd(yy,-1,FYStart) FROM cteFYStart) SELECT ITSDivisionID ,A = Sum(Case When d.DocStatus = 'Retired' and d.RetiredDate >= fy.FYPrevStart and d.RetiredDate < fy.FYStart Then 1 Else 0 End) ,B = Sum(Case When d.DocStatus = 'Active' Then 1 Else 0 End) ,C = Sum(Case When d.DocStatus = 'Active' and d.TargetClosureDate >= fy.FYStart and d.TargetClosureDate < fy.FYNextStart Then 1 Else 0 End) ,D = Sum(Case When d.DocStatus = 'Active' and d.TargetClosureDate >= fy.FYNextStart Then 1 Else 0 End) ,Critical = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 4 Then 1 Else 0 End) ,High = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 3 Then 1 Else 0 End) ,Medium = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 2 Then 1 Else 0 End) ,Low = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 1 Then 1 Else 0 End) ,TBD = Sum(Case When d.DocStatus = 'Active' and d.RiskRating = 0 Then 1 Else 0 End) FROM dbo.Document d CROSS JOIN cteFYAll fy WHERE DocType = 1 AND DocStatus IN ('Active','Retired') GROUP BY ITSDivisionID;` Again, the code isn't yet complete. I'm waiting for you to think outside the box a bit and make it so BCD and DRR are calculated without repeating code and to add the "Division" name column using just one more CTE.Your code works perfectly in SQL, I have to think about how to make DRR, weekend is full to me. Your code is still not accepted by Dundas (because of the CTE), I will check with them to see if there could be any work around. Thank you once again for your kind help and patient. Have a break and enjoy your weekend with your family.
Post #1561278
 Posted Sunday, April 13, 2014 10:14 AM
 SSC-Forever Group: General Forum Members Last Login: Today @ 2:41 PM Points: 42,081, Visits: 39,473
Post #1561280
 Posted Sunday, April 13, 2014 10:22 AM
 SSC-Insane Group: General Forum Members Last Login: Today @ 2:35 PM Points: 23,522, Visits: 37,764
 As much as I hate saying this, since semicolons are statement terminators not statement begininators, try putting a semicolon in front of the WITH of the CTE declaration. I.E. ;with ... .
Post #1561281
 Posted Sunday, April 13, 2014 1:51 PM
 SSC Eights! Group: General Forum Members Last Login: Yesterday @ 12:50 PM Points: 941, Visits: 1,651
 [quote]Jeff Moden (4/13/2014)There's a reason why it takes 2 weeks to get something into production. I suspect it's because people need time to test your code. So now YOU stop and think about it... YOU had an error in your code that mixed up the Fiscal Year with the Calendar Year. If YOU had put that into production, you might be looking for a new place to work! [quote]In my company, any changes must go through UAT and then PROD (IST is not rigorously required but is suggested), they are on different domains that are not communicable and require different user id to test and UAT does not have PROD data and we are not allowed to use PROD data in UAT. There is weekly meeting to approve those change request. For database change it involves four departments + at least 5 forms to be created and approved by up to Director.I hope you can imagine the lengthy process and understand why I think sometimes Option 3 is the one I like to go (and also my manager), reason is very simple, I don't need to go through the bureaucratic and debatable procedures.
Post #1561296

 Permissions