February 26, 2017 at 12:29 pm
Hello!
I wrote the following stored procedure, using SQL SERVER 2008, which works; however, I wanted to know if it could be written more efficiently. I am trying to take my SQL programming skills from novice to professional. Any thoughts on improvements?
USE [Galactic]
GO
/****** Object: StoredProcedure [dbo].[stp_EmployeesHoursWorked] Script Date: 02/25/2017 23:22:51 ******/
SET ANSI_NULLS OFF
GO
SET QUOTED_IDENTIFIER OFF
GO
/*
This stored procedure returns a list of employees at the given hub with hours worked for the given week.
*/
ALTER PROCEDURE [dbo].[stp_EmployeesHoursWorked]
@Weekvarchar(7) = NULL,
@HubCodevarchar(4) = NULL,
@HoursWorked int = NULL
AS
BEGIN
IF @Week = 'All' AND @HubCode = 'All'
BEGIN
SELECT Employee.EmployeeNumber
,FirstName
,LastName
,Hub.HubCode
,RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) AS [Week]
,SUM(HoursWorked) AS HoursWorked
FROM TimeEntry INNER JOIN Assignment ON TimeEntry.AssignmentID = dbo.Assignment.AssignmentID
INNER JOIN Employee ON Assignment.EmployeeNumber = Employee.EmployeeNumber
INNER JOIN Hub on Assignment.HubCode = Hub.HubCode
WHERE RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) IN (SELECT DISTINCT RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2) +'-'+ CONVERT(char(4), YEAR(WorkDate)) AS WeekNumber
FROM TimeEntry
UNION
SELECT 'All')
AND Hub.HubCode IN (SELECT DISTINCT HubCode FROM Hub)
GROUP BY Employee.EmployeeNumber, FirstName, LastName, Hub.HubCode, RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate))
HAVING SUM(HoursWorked) >= @HoursWorked
ORDER BY Employee.EmployeeNumber
END
ELSE IF @HubCode = 'All'
BEGIN
SELECT Employee.EmployeeNumber
,FirstName
,LastName
,Hub.HubCode
,RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) AS [Week]
,SUM(HoursWorked) AS HoursWorked
FROM TimeEntry INNER JOIN Assignment ON TimeEntry.AssignmentID = dbo.Assignment.AssignmentID
INNER JOIN Employee ON Assignment.EmployeeNumber = Employee.EmployeeNumber
INNER JOIN Hub on Assignment.HubCode = Hub.HubCode
WHERE RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) = @Week
AND Hub.HubCode IN (SELECT DISTINCT HubCode FROM Hub)
GROUP BY Employee.EmployeeNumber, FirstName, LastName, Hub.HubCode, RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate))
HAVING SUM(HoursWorked) >= @HoursWorked
ORDER BY Employee.EmployeeNumber
END
ELSE IF @Week = 'All'
BEGIN
SELECT Employee.EmployeeNumber
,FirstName
,LastName
,Hub.HubCode
,RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) AS [Week]
,SUM(HoursWorked) AS HoursWorked
FROM TimeEntry INNER JOIN Assignment ON TimeEntry.AssignmentID = dbo.Assignment.AssignmentID
INNER JOIN Employee ON Assignment.EmployeeNumber = Employee.EmployeeNumber
INNER JOIN Hub on Assignment.HubCode = Hub.HubCode
WHERE RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) IN (SELECT DISTINCT RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2) +'-'+ CONVERT(char(4), YEAR(WorkDate)) AS WeekNumber
FROM TimeEntry
UNION
SELECT 'All')
AND Hub.HubCode = @HubCode
GROUP BY Employee.EmployeeNumber, FirstName, LastName, Hub.HubCode, RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate))
HAVING SUM(HoursWorked) >= @HoursWorked
ORDER BY Employee.EmployeeNumber
END
ELSE
BEGIN
SELECT Employee.EmployeeNumber
,FirstName
,LastName
,Hub.HubCode
,RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) AS [Week]
,SUM(HoursWorked) AS HoursWorked
FROM TimeEntry INNER JOIN Assignment ON TimeEntry.AssignmentID = dbo.Assignment.AssignmentID
INNER JOIN Employee ON Assignment.EmployeeNumber = Employee.EmployeeNumber
INNER JOIN Hub on Assignment.HubCode = Hub.HubCode
WHERE RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) = @Week
AND Hub.HubCode = @HubCode
GROUP BY Employee.EmployeeNumber, FirstName, LastName, Hub.HubCode, RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate))
HAVING SUM(HoursWorked) >= @HoursWorked
ORDER BY Employee.EmployeeNumber
END
END
GO
February 26, 2017 at 10:34 pm
What you have there is a "Catch-All" query where the input parameters could be missing, have a special meaning (All), or have an actual value. As you found out, that makes a pretty good train wreck in your code not to mention performance and maintainability.
Also, you have "Non SARGable" code in the WHERE clause. "SARG" stands for "Search ARGument" and "Non SARGable" means that the best code can do is an index scan. "Non SARGable" code cannot do a complete index seek. One of the things that makes code "Non SARGable" is encapsulating a column in a function or otherwise modifying the value of the column. Here's your "Non SARGable" WHERE clause where every row must be modified before it can do the comparison
WHERE RIGHT('0'+CONVERT(varchar(2),DATEPART(wk, WorkDate)),2)+'-'+CONVERT(char(4), YEAR(WorkDate)) IN (whatever)
With that in mind, here's an iTVF (Inline Table Valued Function) that will help us get rid of the "Non SARGable" code as well as providing a standard method for converting week/year (wk-yyyy or w-yyyy) into valid start (inclusive) and cutoff (exclusive) dates.
CREATE FUNCTION dbo.WeekBoundaryDates (@Week VARCHAR(7))
/**********************************************************************************************************************
Purpose:
Given an unchecked parameter of ww-yyyy or w-yyyy, find the inclusive start date and exclusive end date for the week.
Programer Notes:
1. This is a high performance iTVF (Inline Table Valued Function) to be used in the FROM clause or APPLY clause.
2. The returned inclusive start date (WeekStartDate) will be on the first of the year for short weeks on the first
week of the year.
3. The returned exclusive end date (CutOffDate) will be on the first of the next year for short weeks on the last
week of the year.
4. Except for the instances noted above, the start date and the cutoff date always occur on Sundays.
5. Note that if the "week" value is larger than the number of weeks in the year is used, the Cutoff date will be
<= the WeekStartDate, which will cause no returns if used in traditional SomeDate >= StartDate AND somedate <
CutoffDate scenarios.
Usage:
--===== Single Value Usage to constrain return from a table
SELECT t.WorkDate
FROM dbo.TimeEntry t
JOIN dbo.WeekBoundaryDates('17-2006') wkbd
ON t.WorkDate >= wkbd.WeekStartDate
AND t.WorkDate < wkbd.CutoffDate
;
--===== Usage with a table
WITH cteSimulatedTable(SomeWkYear) AS
(
SELECT '1-2015' UNION ALL --Short week range returned
SELECT '9-2015' UNION ALL
SELECT '23-2015' UNION ALL
SELECT '52-2015' UNION ALL --Full week range returned
SELECT '53-2015' UNION ALL --Short week range returned
SELECT '54-2015' UNION ALL --Illegal week, CutoffDate <= WeekStartDate returned
SELECT '55-2015' --Illegal week, CutoffDate <= WeekStartDate returned
)
SELECT t.SomeWkYear
,wkbd.WeekStartDate
,wkbd.CutoffDate
FROM cteSimulatedTable t
CROSS APPLY dbo.WeekBoundaryDates(t.SomeWkYear) wkbd
;
Revision History:
Rev 00 - 26 Feb 2017 - Jeff Moden
- Initial creation and unit test.
**********************************************************************************************************************/
RETURNS TABLE WITH SCHEMABINDING AS
RETURN WITH
cteParm AS
(--===== Splits the input parameter and calculates the start of next year
SELECT Week# = LEFT(@Week,CHARINDEX('-',@Week)-1)-1 --1 less than the number given
,ThisYear = CONVERT(DATETIME,RIGHT(@Week,4)) --First of the year
,NextYear = DATEADD(yy,1,CONVERT(DATETIME,RIGHT(@Week,4))) --First of the next year
)
,
cteBaseDate AS
(--===== Calculates the Sunday start of the given week
SELECT BaseDate = DATEADD(dd,DATEDIFF(dd,-1,ThisYear)/7*7+(Week#*7),-1) --(-1) is 31 Dec 1899 and is a Sunday
,ThisYear
,NextYear
FROM cteParm
)--==== Calculates the boundaries of the week and observes short start or end weeks in the year.
SELECT WeekStartDate = CASE WHEN BaseDate < ThisYear THEN ThisYear ELSE BaseDate END
,CutOffDate = CASE WHEN DATEADD(dd,7,BaseDate) > NextYear THEN NextYear ELSE DATEADD(dd,7,BaseDate) END
FROM cteBaseDate
;
GO
Of course, it would be easier and better if you used ISO standard weeks but that drives most accountants absolutely nuts. Since you didn't, I didn't. 😉
That's brings us to the code. I combined all the conditional queries into one and tested it. I found two pieces of code (a join and the WHERE clause) that changed the whole personality of the code based on whether they were present or not. For example, If @HubCode was 'ALL', thee was no reason to include the WHERE clause. If @Week was 'ALL', there was no reason to include it as a JOIN (a SARGable join... have a look at the code to see how I changed it).
I was also careful about matching the data-types to the tables to avoid any implicit converts that might also make for some "Non SARGable" code.
The code is completely safe from SQL Injection because there is no concatenation of user provided parameters. We conditionally change the dynamic SQL based on the parameters and then us sp_executesql to tokenize the inputs.
Here's the code. Note that I even format the dynamic SQL for readability during troubleshooting. This is how I write production code with comments, headers, usage examples, formatting, etc, etc. And also notice how I replace the sections of code based on the inputs. There's also a link in the code for what I think should be required for anyone and everyone that needs to write "Catch-All" queries.
CREATE PROCEDURE dbo.EmployeesHoursWorked
/**********************************************************************************************************************
Purpose:
Given any, all, or no parameters, return a report of hours worked by EmployeeID.
Usage Examples:
--===== Full Syntax
EXEC dbo.EmployeesHoursWorked @Week, @HubCode, @MinHoursWorked
;
--===== No parameters. Same as 'All' for @week and @HubCode, 0 for @MinHoursWorked
EXEC dbo.EmployeesHoursWorked
;
--===== Specified Week and HubCode.
EXEC dbo.EmployeesHoursWorked '17-2006','BLND'
;
--===== Specified HubCode only for all weeks
EXEC dbo.EmployeesHoursWorked @HubCode = 'BLND'
;
Revision History
Rev 00 - 26 Feb 2017 - Jeff Moden
- Initial creation and unit test.
**********************************************************************************************************************/
@Week VARCHAR(7) = NULL --'ALL' or wk-yyyy or w-yyyy.
,@HubCode CHAR(4) = NULL --'ALL' or some valid HubCode. Changed to same datatype as in table.
,@MinHoursWorked NUMERIC(6,2)= NULL --Changed to same datatype as in table
AS
--===== Default the variables to wide-open if they're NULL
SELECT @Week = ISNULL(@Week,'All')
,@HubCode = ISNULL(@HubCode,'All')
,@MinHoursWorked = ISNULL(@MinHoursWorked,0)
;
--===== Define variables the dynamic SQL and two conditional pieces of the dynamic SQL.
DECLARE @sql NVARCHAR(4000)
,@WeekJoin NVARCHAR(4000)
,@WHERE NVARCHAR(4000)
;
--===== Based on the content of the parameters, figure out what the dynamic pieces should be.
-- Note that there is no contatenation of strings from the outside world so this is completely safe from SQL Injection.
-- http://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/
SELECT @WeekJoin = CASE
WHEN @Week = N'ALL'
THEN N''
ELSE N' JOIN dbo.WeekBoundaryDates(@Week) wkbd ON t.WorkDate >= wkbd.WeekStartDate and t.WorkDate < wkbd.CutoffDate'
END
,@WHERE = CASE
WHEN @HubCode = N'All'
THEN N''
ELSE N' WHERE h.HubCode = @HubCode'
END
,@SQL = REPLACE(REPLACE(REPLACE('
SELECT e.EmployeeNumber
,e.FirstName
,e.LastName
,h.HubCode
,[Week] = RIGHT(100+DATENAME(wk,t.WorkDate),2)+"-"+DATENAME(yy,t.WorkDate)
,HoursWorked = SUM(t.HoursWorked)
FROM dbo.TimeEntry t
JOIN dbo.Assignment a ON t.AssignmentID = a.AssignmentID
JOIN dbo.Employee e ON a.EmployeeNumber = e.EmployeeNumber
JOIN dbo.Hub h ON a.HubCode = h.HubCode
<<@WeekJoin>>
<<@WHERE>>
GROUP BY e.EmployeeNumber
,e.FirstName
,e.LastName
,h.HubCode
,RIGHT(100+DATENAME(wk,t.WorkDate),2)+"-"+DATENAME(yy,t.WorkDate)
HAVING SUM(HoursWorked) >= @MinHoursWorked
ORDER BY e.EmployeeNumber;'
,'"' ,'''') --The next 3 lines are the other end of the REPLACEs
,'<<@WeekJoin>>',@WeekJoin)
,'<<@WHERE>>' ,@WHERE)
;
--===== We're ready to rock. Let's see the dynamic SQL and then execute it.
PRINT @sql;
EXEC sp_executesql
@sql
,N'@Week VARCHAR(7), @HubCode CHAR(4), @MinHoursWorked NUMERIC(6,2)' --Variables in dynamic SQL
,@Week,@HubCode,@MinHoursWorked --One for one values for variables in dynamic SQL
;
GO
--Jeff Moden
Change is inevitable... Change for the better is not.
February 27, 2017 at 2:18 am
Man, Jeff. Leave some crumbs for the rest of us.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
February 27, 2017 at 9:22 am
Grant Fritchey - Monday, February 27, 2017 2:18 AMMan, Jeff. Leave some crumbs for the rest of us.
Heh... thanks for the feedback, Grant. I finally beat you guys at one-in-a-row. 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
February 27, 2017 at 7:50 pm
Jeff Moden - Monday, February 27, 2017 9:22 AMGrant Fritchey - Monday, February 27, 2017 2:18 AMMan, Jeff. Leave some crumbs for the rest of us.Heh... thanks for the feedback, Grant. I finally beat you guys at one-in-a-row. 😉
Hey - Jeff - Stop thinking one row at a time. Someone wise on here told me to think in columns not rows 🙂
----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?
February 27, 2017 at 8:18 pm
Matt Miller (#4) - Monday, February 27, 2017 7:50 PMJeff Moden - Monday, February 27, 2017 9:22 AMGrant Fritchey - Monday, February 27, 2017 2:18 AMMan, Jeff. Leave some crumbs for the rest of us.Heh... thanks for the feedback, Grant. I finally beat you guys at one-in-a-row. 😉
Hey - Jeff - Stop thinking one row at a time. Someone wise on here told me to think in columns not rows 🙂
😉
--Jeff Moden
Change is inevitable... Change for the better is not.
February 27, 2017 at 8:24 pm
Hi Jeff!
Many thanks for your response! Clearly, I have lots to learn in order to move from novice to expert. You've given me lots to digest and research. Many thanks for sharing your knowledge and expertise.
DataStorm
February 27, 2017 at 9:02 pm
My pleasure. You're on the right path because you "try". Practice and the experienced gained from trying is worth a ton more than just being book learned although the books are important as well. From what I see, you're going to do fine. Keep up the good work and thank you for the feedback.
--Jeff Moden
Change is inevitable... Change for the better is not.
February 27, 2017 at 10:06 pm
Hi Jeff!
Speaking of books, do you have any recommendations? Any favorites for someone at my level?
Thanks,
DataStorm
February 28, 2017 at 3:40 am
DataStorm - Monday, February 27, 2017 10:06 PMHi Jeff!Speaking of books, do you have any recommendations? Any favorites for someone at my level?Thanks,DataStorm
I can make a couple of recommendations. Look down there in my signature.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
February 28, 2017 at 4:18 am
Great! Thanks again!
DataStorm
February 28, 2017 at 9:05 am
DataStorm - Monday, February 27, 2017 10:06 PMHi Jeff!Speaking of books, do you have any recommendations? Any favorites for someone at my level?Thanks,DataStorm
Yes. The books that Grant has in his signature are "must reads" for anyone that wants to get the most performance and lowest resource usage out of their code, so far as I'm concerned.
For actual T-SQL Coding, any book on the subject written by Itzik Ben-Gan is a winner. Same goes for the articles that many of the heavy hitters on this site and Simple-Talk.com. They're great and they're free. Gail Shaw is a Microsoft Certified Master and she's got a great collection of methods on her site at http://sqlinthewild.co.za/ . (I referred you to one of her posts in the code I posted). Also, any article by Greg Robidoux on the https://www.mssqltips.com/ site makes for an incredible read. Also, the late, great Dwain Camps started out much as you have and quickly became a force to be reckoned with. Here's the Google search for his stuff. https://www.google.com/?gws_rd=ssl#q=Dwain+Camps+sql&*
One of the first articles you might read can be found at http://www.sqlservercentral.com/articles/T-SQL/62867/ . It's about a thing called a "Tally" or "Numbers" table. Now, by itself, it's not a panacea of performance for everything. It does, however, demonstrate that even the humble SELECT statement has a machine language loop behind it and, if you can take advantage of that, then your code is allowing SQL Server to do its best rather than you telling it exactly what to do. It's a fundamental bit of knowledge that many people never learn or understand. I've also written a couple of articles for SSC, which can be found at the following link. Understand that, just like anyone else, I've learned over time and some of the things in the discussions are worth more than what the original article has in them. The article on how to replace multiple adjacent spaces is just such an article and I went back and highlighted the most important part of the discussion at the beginning of the article. That's what makes this community great. Someone puts something out there and that incites a riot of innovation and helpful tips in the discussion.
And, just a suggestion on how to think about SQL. Read the note about the "paradigm shift" in my signature line below. When I first came to that realization, my code as a newbie got a whole lot better.
Also understand that you can't test a thing, especially for performance, without substantial volumes of test data and that you won't always have enough in "sample databases" like the Galactic or Adventure Works database. You sometimes (many times, actually) need to know how to build the proverbial "million row test table". With that, I'd suggest the second thing you read are the following two articles. Without such knowledge, the paradigm shift wouldn't have been enough and I'd still be struggling. Both further enhance the idea of thinking in columns rather than in rows, as well.
http://www.sqlservercentral.com/articles/Data+Generation/87901/
http://www.sqlservercentral.com/articles/Test+Data/88964/
As you'll find out in some of Dwain Camp's and other's posts, a bit of math also goes a long way in solving some of the more difficult problems you may run into. Here's a couple of articles where a bit of math and understanding of working in columns solved what many considered to be a really tough problem. A day long process that was generally accepted to be the "only way" was reduced to 54 seconds in the first one by using a little math.
http://www.sqlservercentral.com/articles/Hierarchy/94040/
http://www.sqlservercentral.com/articles/T-SQL/94570/
Last and certainly not the least, participation in the forums on this site will provide you with a wealth of real-world problems that you just won't find in any book. Check out some of the questions that folks have and try to solve them. If you come up with something good, don't be afraid to post it as a possible answer and then take any critique as a learning tool even if it's about what you learned about the people making the critique. You'll quickly identify the heavy hitters and, man, can they ever write some amazing code.
--Jeff Moden
Change is inevitable... Change for the better is not.
February 28, 2017 at 5:32 pm
Jeff, you are a treasure chest of information! Thanks for dropping those gems! I will most certainly take a look at these resources and apply these techniques to my learning.
DataStorm
February 28, 2017 at 6:10 pm
My pleasure again. You seem like you really want to hit it off on SQL server and if I can help you help your self not only on developing the skill but in further developing your obvious passion as well, then you might do the same for someone else in the future. Thanks again for your interest in it all.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 14 posts - 1 through 14 (of 14 total)
You must be logged in to reply to this topic. Login to reply