April 15, 2010 at 2:31 pm
lmu92 (4/15/2010)
WayneS (4/15/2010)
lmu92 (4/15/2010)
blah blah blah.... Right, Wayne?uh, sure, jeff :w00t:
You consider my statement as "blah blah blah"? I'm depressed now. :crying:
I just wanted to quote the one part... to show I actually read everything up to it.
I just looked through your code, and it looks very good to me. I see only one very minor thing... in the case statements, where you're doing "case when ... then null else convert(int, ...)". You might want to do convert(int, null). I've read about some possible issues (okay, it was in Joe Celko's book, so maybe it's an issue in other products).
I didn't look through the original procedure at all (when Gregory mentioned how long it was, I just blew it off)... is the code you just posted a replacement for all of the code, or just that last un-finished part? I assume it's just the last part... I didn't see an OUTPUT clause in an UPDATE statement there which has been previously mentioned.
Good, no EXCELLENT job Lutz.
Wayne
Microsoft Certified Master: SQL Server 2008
Author - SQL Server T-SQL Recipes
April 15, 2010 at 3:05 pm
Well, Wayne, I appreciate the compliment :blush:
But it looks much more complicated than it really is:
Those "x"hundred lines of code include quite a few descriptive comments (which is, to be honest, one of the main reasons why I started to look deeper into it: well formatted code with comments -> my conclusion was: here's someone who knows how to write code but need a little advice what the code structure should look like. Seems like I was right...).
Also, if you simplify (or compress) the CASE conditions and the "standard" loop overhead (variable declaration and initialization), it comes down to an effort that is manageable.
And, like you said, the effort Gregory is taking to help us help him (including testing and modification), makes it worth the effort anyway.
There are easier threads by OP's that care a lot less...
And yes, this is what this community is all about. @gregory: SSC is not only a forum. We're a community. And proud of it! So, welcome aboard!!!!
Regarding your recent post:
I currently simply take the CASE statements "as is" and just change the referenced variables into related columns. To be honest, other than changing the references I didn't look into optimization of the CASE statements at all. Like I said in an earlier post, I consider it to be "step 1: replace the loop".
Therefore, I decided to leave the SELECT and WHERE conditions as close as possible to the original code. Which explains why the current work is not as complicated /effort-taking as it might look like...
I left out the previous reworked code as well as the TRY..CATCH block since I figured Gregory would ask if he'd have some trouble implementing it. Pure laziness on my side...
Once the loops are removed, the next step is to look into further optimization (including the case statements along with checking execution plans/indexing). But I prefer to slice an elephant before I swallow it (not that I've literally done that before nor planning to do so... :pinch: ).
I don't think it's time to call the job excellent, since it's unfinished yet. But the team is making progress.
And once the code rewrite is done, it's not MY excellent job, it's OURs.
April 15, 2010 at 5:57 pm
Gregory,
While I'm thinking of it, RBarryYoung wrote a couple of articles (and, supposedly, more are forthcoming) on how to eliminate cursors from your code. Check it out here[/url].
Wayne
Microsoft Certified Master: SQL Server 2008
Author - SQL Server T-SQL Recipes
April 16, 2010 at 6:23 am
I wasn't sure if the columns I used would make the rows unique... Obviously, they didn't.
Well, you were closing to making it unique. The combination of PermitNumber and PWDPermitTypeID would make it unique. I tried adding that to the output statement, and then went to the final Update statement, but the problem with that is I had to use a case statement in the where clause, and I wasn't able to get it to update all the rows that were actually updated. Of the 557xxx rows that got inserted, only 549xxx (or close to that) had their status updated. I wasn't able to find a pattern why that didn't work, so I was just "assuming" that it was because of me trying to do:
Where Case PWDPermitConversionData.PermitPrefix When 'D' Then 3
When 'M' Then 1
When 'V' Then 2
When 'S' Then 4
End = PWDPermit.PWDPermitTypeID
A agree, this has been fun and challenging for me at the same time. I've been trying to come up with the set-based solution for the "previous" permits part and I am going to try and beat you to the post, but we'll see who wins. 🙂
But, this will be an easy workaround just putting the PWDPermitConversionDataID into the final table.
Edit: Damn, didn't see the last page, you already beat me...maybe I'll try and finish mine before I look at yours...I did peak and see that you used a CTE, so I'll fess up right now that I've heard of them, and slightly understand them, but have never implemented one...another learning experience.
April 16, 2010 at 6:39 am
Oh, by the way, I let my old version run and finish while I/we've been working on the new set-based solution...the only version finished in 11 hours, plus 18 hours, plus around 12 hours the day before...so that's a total of ~41 hours for the "RBAR" method.
Also, I may not post for awhile because I want to see if I can come up with the CTE approach checking your links and researching...
April 16, 2010 at 9:42 am
That example of the cross apply stuff makes it look so easy. Once the article mentioned it was just like a method in programming, it kind of clicked for me.
If i understand this write, with CTE's and Cross Apply statements, are cursors basically not needed anymore? Because it sounds like should be able to do this approach, and just using that spt_values table that will do all of the counting (or iterating) for you...
So why don't the cross apply tables need a prefix, or a short name when accessed in the select portion of the statement? (meaning "PWDPermit p" so you access it like "p.PWDPermitID")
April 16, 2010 at 10:04 am
gregory.anderson (4/16/2010)
That example of the cross apply stuff makes it look so easy. Once the article mentioned it was just like a method in programming, it kind of clicked for me.If i understand this write, with CTE's and Cross Apply statements, are cursors basically not needed anymore? Because it sounds like should be able to do this approach, and just using that spt_values table that will do all of the counting (or iterating) for you...
So why don't the cross apply tables need a prefix, or a short name when accessed in the select portion of the statement? (meaning "PWDPermit p" so you access it like "p.PWDPermitID")
:blush: You got me on that! :blush:
It's simply bad coding habit together with lazyness anda good portion of luck...
The reason why the code doesn't fail is that there are no duplicate column names used. So, to change it to clean code, add the alias and prefix as you would do it with any other type of joined tables.
I'm sorry for causing confusion...
Regarding your assumption that you won't need c.u.r.s.o.r.s anymore because of CTE's and Cross Apply: That's not really true. In the given scenario a subquery and a cross join would have done the same:
/*INSERT INTO PWDPermit
(
[PermitNo],
[CustNo],
[PWDPermitTypeID],
[PWDPermitStatusID],
[PWDPermitDeviceTypeID],
[Location],
[Examiner],
[Station],
[IssueDate],
[ExpirationDate],
[InvalidDLNumber],
[Returned],
[CreatedBy],
[CreatedDate]
)
*/
SELECT
CASE WHEN PermitNumber = '' THEN NULL
ELSE CAST(PermitNumber AS INT)
END,
CustomerNumber,
CASE WHEN PermitPrefix = 'D' THEN @PermitType_Org
WHEN PermitPrefix = 'M' THEN @PermitType_Ind
WHEN PermitPrefix = 'V' THEN @PermitType_Temp
WHEN PermitPrefix = 'S' THEN @PermitType_Sticker
WHEN PermitPrefix = '' THEN NULL
ELSE PermitPrefix
END,
CASE WHEN [Status] = 'RP' THEN @PermitStatus_RU
WHEN [Status] = '' THEN NULL
ELSE CAST([Status] AS INT)
END,
CASE WHEN PermitPrefix = 'S' THEN @PermitDeviceType_Sticker
WHEN PermitPrefix IN ('D', 'M', 'V') THEN @PermitDeviceType_Placard
WHEN PermitPrefix = '' THEN NULL
ELSE @PermitDeviceType_Unknown
END,
'999', -- This is unknown
'999', -- This is unknown
'99', -- This is unknown
CASE WHEN ExpirationDate = 'NONE' OR ExpirationDate = '' OR ExpirationDate = 'NOE' THEN @MinDate
WHEN CONVERT(DATETIME, ExpirationDate) > @Today THEN DATEADD(mm, -1, CONVERT(DATETIME, IssueDate))
ELSE CAST(ExpirationDate AS DATETIME)
END, -- This is unknown
CASE WHEN ExpirationDate = 'NONE' OR ExpirationDate = '' OR ExpirationDate = 'NOE' THEN @MaxDate
ELSE CAST(ExpirationDate AS DATETIME)
END,
@False, -- This is unknown
CASE WHEN PrevPermReturn = 'Y' THEN @True
ELSE @False
END,
@User,
@Today
FROM
(
SELECT
p.PWDPermitConversionDataID,
p.CustomerNumber,
p.PermitNumber,
p.PermitPrefix,
p.[Status],
p.ExpirationDate,
p.IssueDate,
tally.n,
CASE tally.n
WHEN 1 THEN p.PrevPermPref1
WHEN 2 THEN p.PrevPermPref2
WHEN 3 THEN p.PrevPermPref3
WHEN 4 THEN p.PrevPermPref4
WHEN 5 THEN p.PrevPermPref5
WHEN 6 THEN p.PrevPermPref6
WHEN 7 THEN p.PrevPermPref7
WHEN 8 THEN p.PrevPermPref8
WHEN 9 THEN p.PrevPermPref9
WHEN 10 THEN p.PrevPermPref10
END AS [PrevPermPref],
CASE tally.n
WHEN 1 THEN p.PrevPermNum1
WHEN 2 THEN p.PrevPermNum2
WHEN 3 THEN p.PrevPermNum3
WHEN 4 THEN p.PrevPermNum4
WHEN 5 THEN p.PrevPermNum5
WHEN 6 THEN p.PrevPermNum6
WHEN 7 THEN p.PrevPermNum7
WHEN 8 THEN p.PrevPermNum8
WHEN 9 THEN p.PrevPermNum9
WHEN 10 THEN p.PrevPermNum10
END AS [PrevPermNum],
CASE tally.n
WHEN 1 THEN p.PrevPermStatus1
WHEN 2 THEN p.PrevPermStatus2
WHEN 3 THEN p.PrevPermStatus3
WHEN 4 THEN p.PrevPermStatus4
WHEN 5 THEN p.PrevPermStatus5
WHEN 6 THEN p.PrevPermStatus6
WHEN 7 THEN p.PrevPermStatus7
WHEN 8 THEN p.PrevPermStatus8
WHEN 9 THEN p.PrevPermStatus9
WHEN 10 THEN p.PrevPermStatus10
END AS [PrevPermStatus],
CASE tally.n
WHEN 1 THEN p.PrevPermReturn1
WHEN 2 THEN p.PrevPermReturn2
WHEN 3 THEN p.PrevPermReturn3
WHEN 4 THEN p.PrevPermReturn4
WHEN 5 THEN p.PrevPermReturn5
WHEN 6 THEN p.PrevPermReturn6
WHEN 7 THEN p.PrevPermReturn7
WHEN 8 THEN p.PrevPermReturn8
WHEN 9 THEN p.PrevPermReturn9
WHEN 10 THEN p.PrevPermReturn10
END AS [PrevPermReturn],
CASE tally.n
WHEN 1 THEN p.PrevPermExpiration1
WHEN 2 THEN p.PrevPermExpiration2
WHEN 3 THEN p.PrevPermExpiration3
WHEN 4 THEN p.PrevPermExpiration4
WHEN 5 THEN p.PrevPermExpiration5
WHEN 6 THEN p.PrevPermExpiration6
WHEN 7 THEN p.PrevPermExpiration7
WHEN 8 THEN p.PrevPermExpiration8
WHEN 9 THEN p.PrevPermExpiration9
WHEN 10 THEN p.PrevPermExpiration10
END AS [PrevPermExpiration],
CASE tally.n
WHEN 1 THEN p.ConversionStatusPrev1
WHEN 2 THEN p.ConversionStatusPrev2
WHEN 3 THEN p.ConversionStatusPrev3
WHEN 4 THEN p.ConversionStatusPrev4
WHEN 5 THEN p.ConversionStatusPrev5
WHEN 6 THEN p.ConversionStatusPrev6
WHEN 7 THEN p.ConversionStatusPrev7
WHEN 8 THEN p.ConversionStatusPrev8
WHEN 9 THEN p.ConversionStatusPrev9
WHEN 10 THEN p.ConversionStatusPrev10
END AS [ConversionStatusPrev]
FROM PWDPermitConversionData p
CROSS JOIN
(
SELECT number AS n FROM master..spt_values
WHERE TYPE ='P'
AND number >0
AND number<11
) tally
WHERE p.PermitPrefix = 'M'
) cte
WHERE PrevPermPref NOT IN ('P', 'H','')
AND ConversionStatusPrev IS NULL
ORDER BY PWDPermitConversionDataID,n
April 16, 2010 at 10:43 am
It's simply bad coding habit together with lazyness anda good portion of luck...
The reason why the code doesn't fail is that there are no duplicate column names used. So, to change it to clean code, add the alias and prefix as you would do it with any other type of joined tables.
I always forget that. It's so bad for me that I've gotten into the habit of a) placing the database name in front of the table (like you've seen in my stored proc), and also b) even when doing a select against one table without any joins, I'll still use an alias for the tablename...
April 16, 2010 at 10:57 am
gregory.anderson (4/16/2010)
...I always forget that. It's so bad for me that I've gotten into the habit of a) placing the database name in front of the table (like you've seen in my stored proc), and also b) even when doing a select against one table without any joins, I'll still use an alias for the tablename...
There's nothing wrong with it at all. It's actually a very straight forward habit. And, like I said, it was pure lazyness on my side...
April 16, 2010 at 12:40 pm
I've been racking my brain on the very last step of this "previous" permit section...with just a yes or no, is it even possible to update the ConversionStatusPrevX fields with anything other than writing it ten 10 times, or using dynamic sql with a counter for the 'X'? I "think" I understand everything that you guys have showed me, I wrote it all by myself with just referring back to Lutz's post as a reference......
April 16, 2010 at 1:15 pm
gregory.anderson (4/16/2010)
I've been racking my brain on the very last step of this "previous" permit section...with just a yes or no, is it even possible to update the ConversionStatusPrevX fields with anything other than writing it ten 10 times, or using dynamic sql with a counter for the 'X'? I "think" I understand everything that you guys have showed me, I wrote it all by myself with just referring back to Lutz's post as a reference......
Yes. 🙂
You could either do it based on the values of the PrevPermPrefX column or based on a pivoted PWDPermit for the related columns. I'd probably vote for the former since I wouldn't have to touch both of the rather large tables. It might be required to catch the [PWDPermitConversionDataID] values and the column that failed for the rows holding bad values (using the OUTPUT clause applied to the error checking statement for the "previous" permit section -you'd need to write that code, maybe based on the error handling stored in PWDPermitConversionErrors...) to make sure there are no updates to sections where it shouldn't be done.
Either way, I don't think it's required to touch that table 10 times with an update.
If you want me to look into it more detailed, let me know.
Once you have the code finished and producing the expected result I'd be interested how long it actually takes and I'd like to continue the process of performance tuning. Let's see how fast we can make it... I'm curious.
April 16, 2010 at 1:25 pm
It might be required to catch the [PWDPermitConversionDataID] values and the column that failed for the rows holding bad values (using the OUTPUT clause applied to the error checking statement for the "previous" permit section -you'd need to write that code, maybe based on the error handling stored in PWDPermitConversionErrors...)
Wow, for once I'm ahead of you...
I've just about got it to run right now with all of my where clauses that need to be there, once that's done, I'm going to go back to the Update statement at the end...I'll research the Pivot, et al like you said.
April 16, 2010 at 2:00 pm
gregory.anderson (4/16/2010)
It might be required to catch the [PWDPermitConversionDataID] values and the column that failed for the rows holding bad values (using the OUTPUT clause applied to the error checking statement for the "previous" permit section -you'd need to write that code, maybe based on the error handling stored in PWDPermitConversionErrors...)
Wow, for once I'm ahead of you...
I've just about got it to run right now with all of my where clauses that need to be there, once that's done, I'm going to go back to the Update statement at the end...I'll research the Pivot, et al like you said.
I misread your post regarding "very last step of this 'previous' permit section". I thought you were talking about the update section, not the "middle section". I guess it should be possible to modify the where clause in the "previous" permit code I posted earlier (probably tweaked by yourself already ;-)) to avoid that additional statement at all.
Haven't looked into that yet since you actually stopped me at April 16th, 2:23:07 PM:
maybe I'll try and finish mine before I look at yours...
😀
This is actually the point where I wait for you to ask for some specific help/advice or to tell us if the code does produce the expected result. And trust me, I didn't work on that part at all since I'm sure you'll find a solution for it and you would ask for advice if needed. Your doing an excellent job! Congrats!
April 16, 2010 at 2:05 pm
lmu92 (4/16/2010)
gregory.anderson (4/16/2010)
It might be required to catch the [PWDPermitConversionDataID] values and the column that failed for the rows holding bad values (using the OUTPUT clause applied to the error checking statement for the "previous" permit section -you'd need to write that code, maybe based on the error handling stored in PWDPermitConversionErrors...)
Wow, for once I'm ahead of you...
I've just about got it to run right now with all of my where clauses that need to be there, once that's done, I'm going to go back to the Update statement at the end...I'll research the Pivot, et al like you said.
I misread your post regarding "very last step of this 'previous' permit section". I thought you were talking about the update section, not the "middle section". I guess it should be possible to modify the where clause in the "previous" permit code I posted earlier (probably tweaked by yourself already ;-)) to avoid that additional statement at all.
Haven't looked into that yet since you actually stopped me at April 16th, 2:23:07 PM:
maybe I'll try and finish mine before I look at yours...
😀
This is actually the point where I wait for you to ask for some specific help/advice or to tell us if the code does produce the expected result. And trust me, I didn't work on that part at all since I'm sure you'll find a solution for it and you would ask for advice if needed. Your doing an excellent job! Congrats!
Now I think we are both confused...
I have everything running, and it does the insert of all of the "previous" permits (336308 inserted in 18 seconds with 386 that were caught in the error select) so what I am working on right now is how to do the Update statement for the 10 possible "previous" permits per primary record (without doing a loop or c.u.r.s.o.r :-D)
April 16, 2010 at 3:02 pm
By golly, does this look correct?
UpdateIARTS..PWDPermitConversionData2
Set
ConversionStatusPrev1 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 1) = 1 Then 1 Else Null End,
ConversionStatusPrev2 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 2) = 1 Then 1 Else Null End,
ConversionStatusPrev3 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 3) = 1 Then 1 Else Null End,
ConversionStatusPrev4 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 4) = 1 Then 1 Else Null End,
ConversionStatusPrev5 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 5) = 1 Then 1 Else Null End,
ConversionStatusPrev6 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 6) = 1 Then 1 Else Null End,
ConversionStatusPrev7 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 7) = 1 Then 1 Else Null End,
ConversionStatusPrev8 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 8) = 1 Then 1 Else Null End,
ConversionStatusPrev9 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 9) = 1 Then 1 Else Null End,
ConversionStatusPrev10 = Case When (Select 1 From PermitsInserted p2 Where p2.PWDPermitConversionDataID = p.PWDPermitConversionDataID And p2.RecordIndicator = 10) = 1 Then 1 Else Null End
From IARTS..PWDPermitConversionData2 p
Viewing 15 posts - 46 through 60 (of 112 total)
You must be logged in to reply to this topic. Login to reply