Looping "poor practice"... what is the alternative? 3

[Veejc]

Hi all

I have been reading lots of posts and in most I am getting the picture that looping is "poor practice"... and that you should build a user defined function.

Can someone post a basic example of how you would accomplish simulated looping in a function?

Here is the "english" of what I am trying to accomplish.

I have a table of distinct values.
I want to "loop" through that table and for every record I want to then query another table to find any related records in there. ( a bunch of related segments) Then from there I need to make some comparisons to some date values that are found in the latter table and return a simple date.

I dont' expect anyone to write this for me, I'm just looking for a basic example of how to handle this "looping" for lack of a better term. I can then pick it apart and build what I need.

Thanks!

 

[Veejc]

I meant to add the comment that I was looking to see if anyone can tell me if I've done this terribly wrong or if it could be optimized some other way. I'm still relatively new to SQL Server, but this DOES work just fine, I just don't know if I can get it to work any better or faster.

Thanks for all of your very helpful responses!

 

[Veejc]

Dpatrick...

Thanks for that code. That would be perfect, but it looks like that only takes into account if I had a static number of rows for each member, right? I could be wrong... I never know how many "segments" each member has, I just have to keep going until I hit the end of them, comparing one date from one record to another date from the previous record. I'm not comparing dates on the same record and that's where I got a little confused during this whole process when I was trying to create the subqueries to handle it.

Could I do that with a similar query to the one you have provided?

 

[Veejc]

Fetch question with the code I posted...

If you have a cursor inside of another cursor as I do in the posted code, how do I successfully use the @@Fetch_Status to continue looping?

Everything works great until I add the
while @@Fetch_Status = 0 onto the first cursor loop

Any ideas?

 

[maswien]

Veejc, looks like you didn't see my code at all, is it that no use? Thanks

 

[maswien]

You don't actually need cursor, what you need is to update a temporary resultset multiple times.

 

[Pattycake245]

Veejc

You have both cursors set up to "fetch first from cursor...". From my understanding this will only grab the first row and unless you have it as "fetch next from cursor...", the @@FETCH_STATUS will not = 0 after the first grab, so it wil stop, unless you will only ever have one row returned in your cursors.

Basic cursor syntax is as follows:

declare cursorname cursor
for <sql statement>

open cursorname
fetch next from cursorname into <variables>

while @@FETCH_STATUS = 0
BEGIN
do some stuff

declare cursorname2 cursor
for <sql statement>

open cursorname2
fetch next from cursorname2 into <variables>

while @@FETCH_STATUS = 0
BEGIN
do some stuff
fetch next from cursorname2 into <variables>
end

CLOSE cursorname2
DEALLOCATE cursorname2

fetch next from cursorname into <variables>
end

CLOSE cursorname
DEALLOCATE cursorname

Tim

 

[Veejc]

That was the temporary code to see it all work once, I should have qualified that... Here is the code as it is now, and it updates a temporary table that I will ultimately return to a calling program (CRN actually)

I did already change the keywords to FETCH NEXT and the While @@Fetch_status doesn't appear to work

SET QUOTED_IDENTIFIER ON
GO
SET ANSI_NULLS ON
GO




ALTER PROCEDURE dt_earliest_effective_date
@num_of_days int
AS
------------------------------------------------------------------------------------------------------------------------------------
--- CONTINUOUS ENROLLMENT
--- Written by: Dottie Kennedy-Brooks
--- Written on: 4/26/05
--- What it does: We have a need to determine the continuous enrollment date of a member. The organization refers to this
--- as the continous enrollment date, but really it's the members earliest effective date without a lapse in coverage.
--- The organization does not have a clear cut business rule to determine what number of days is allowable to
--- be considered "continuous" and there are several different numbers being used for different purposes, so in
--- order to try to be consistent at least with HOW we are getting at this information, this procedure was created
--- so everyone can use this in determining "continuous enrollment"

--- if the member has several segments to their history and we are looking for the earliest effective date
--- that we have for that member without a break in coverage of > than whatever is used for @num_of_days
--- Parameters: num_of_days - the number of days that is to be used to determine the continuous enrollment
---



declare @memberid varchar(12) -- used to hold the member id
declare @persno varchar(2) -- used to hold the person number
declare @s_effdate varchar(8) -- holds the original string value of abeffdt from jmeligmo
declare @s_termdate varchar(8) -- holds the original string value of abtermdt from jmeligmo
declare @effdate datetime -- holds the converted value of effdate
declare @termdate datetime -- holds the converted value of termdate
declare @s_next_effdate varchar(8) -- holds the 'next' string value of abeffdt
declare @s_next_termdate varchar(8) -- holds the 'next' string value of abtermdate
declare @next_effdate datetime -- holds the next converted value of eff date
declare @next_termdate datetime -- holds the next converted value of term date
declare @least_date datetime -- holds the 'continuous enrollment date'
declare @days int -- holds the number of days allowable for continuous enrollment (user defined)
declare @row_count int -- holds the number of members and this is used in the outer loop to det. # times to loop
declare @cursor_cnt int

-- FYI re: the row_count variable
--@row_count may seem a strange way to loop when you can use the @@Fetch_status, however I tried the fetch status
-- and when I use it in both loops (outer and inner), the inner loop is no longer seen at one point in the loop and fetch status
-- does not really qualify which cursor to look at so I believe it is looking at the cursor that is no longer and thus returning
-- the value that returns the -1 which means to stop looping, so while this seems an odd way to handle this, it was the only
-- way I could find at the time

create table #temp_earliest_eff_date (member varchar(12),
persno varchar(2),
memberid varchar(14),
earliest_eff datetime)

--set @days = @num_of_days
set @days = 90 -- for testing
set @cursor_cnt = 1

-- FYI: this cursor will ONLY hold active members per the eligibility flag clause
-- sometimes the member could have a termdate in the most current segment
-- but this will not affect anything because if there is a date there and member is
-- still active then that elig. flag will remain correct so that is the only criteria you
-- need to be concerned with
set @row_count = (select count( absubno)
from nhpri_diam_ds01.dbo.tbl_CurrentMemberDimension
where eligibility_flag = 1 )
print @row_count

DECLARE members_cursor SCROLL CURSOR
FOR select distinct absubno, abpersno
from nhpri_diam_ds01.dbo.tbl_CurrentMemberDimension
where eligibility_flag = 1


OPEN members_cursor


--while @@Fetch_status = 0
while @cursor_cnt < (@row_count + 1)
--set @cursor_cnt= (@cursor_cnt + 1)
--print @cursor_cnt
begin

FETCH next FROM members_cursor into @memberid, @persno
set @cursor_cnt= (@cursor_cnt + 1)
----------------------------------------------------------------------------------------------
--we have one memberid so grab that member's segments
--I've discovered that it's best to return the dates in their original varchar format because
--if I convert at the time of gathering then if there is a null value, this will crash the statement
--because the value is not convertable to a datetime
-- the order of this cursor needs to be ascending on the abeffdt as you see here in order to
-- work correctly, don't change this or you will be looking at the segments upside down
-----------------------------------------------------------------------------------------------

-- START LOOPING through the member_cursor
-- we have the member cursor to loop through but now
-- we need to loop through the member segments, that's
-- what this cursor is for
declare member_segments scroll cursor
for select abeffdt as effdate,
abtermdt as TERMDaTe
from nhpri_diam_ds01.dbo.jmeligm0_dat
where absubno = @memberid and
abpersno = @persno
order by absubno, abpersno, abeffdt asc

open member_segments
fetch first from member_segments into @s_effdate, @s_termdate
-- this will remain the "least date" until we meet the condition of a new effective date, which is dynamic and defined
-- by whatever the user has passed in as number of days acceptable to determine "continuous enrollment"

----------------------------------------------------------------------------------------------------------------------------------
-- THE CODE IN THE FOLLOWING SECTION IS WHERE WE DETERMINE THE 'CONTINUOUS ENROLLMENT --
----------------------------------------------------------------------------------------------------------------------------------
-- We've pulled in another row so now compare that row's values to the previous to determine if we have a new effective date
-- the value in the abtermdt field is never null, it's just blank when there isn't one there, so is not null is not a valid condition


set @least_date = convert(datetime,left(@s_effdate,4) + '/' + substring(@s_effdate,5,2) + '/' + substring(@s_effdate,7,2) )
if (@s_effdate <> '')
begin
set @effdate = convert(datetime,left(@s_effdate,4) + '/' + substring(@s_effdate,5,2) + '/' + substring(@s_effdate,7,2) )
end
if (@s_termdate <> '')
begin
set @termdate = convert(datetime,left(@s_termdate,4) + '/' + substring(@s_termdate,5,2) + '/' + substring(@s_termdate,7,2) )
end
while @@Fetch_status = 0
begin

fetch next from member_segments into @s_next_effdate, @s_next_termdate

if (@s_next_effdate <> '')
begin
set @next_effdate = convert(datetime,left(@s_next_effdate,4) + '/' + substring(@s_next_effdate,5,2) + '/' + substring(@s_next_effdate,7,2) )
end

-- we need to compare the new effective date against the previous termdate before we get the new termdate
if (datediff(day, @termdate, @next_effdate) ) > @days
begin
-- if the condition for the number of days between segments meets what the user passed in
-- then we modify the @least date to hold the new next_effective date as the new least date
set @least_date = @next_effdate
end

if (@s_next_termdate <> '')
begin
set @termdate = convert(datetime,left(@s_next_termdate,4) + '/' + substring(@s_next_termdate,5,2) + '/' + substring(@s_next_termdate,7,2) )
end


end -- this goes with the member_segment loop

insert into #temp_earliest_eff_date (member, persno, memberid, earliest_eff)
values (@memberid, @persno, ltrim(rtrim(@memberid)) + '-' + ltrim(rtrim(@persno)), @least_date)


set @least_date = null
set @s_effdate = ''
set @s_termdate = ''
set @s_next_effdate = ''
set @s_next_termdate = ''
set @termdate = Null
set @effdate = Null
set @next_effdate = null
set @next_termdate = null
close member_segments

end -- this goes with the members_cursor loop
deallocate member_segments
select * from #temp_earliest_eff_date


---- close everything / destroy the cursor
--close member_segments
close members_cursor
deallocate members_cursor
drop table #temp_earliest_eff_date









GO
SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS ON
GO

 

[maswien]

Using cursor in this case will for sure degrade the performance. It's not neccessary and can be avoid.

 

[Veejc]

The performance isn't really all that bad surprisingly, we have app. 80000 distinct records to get the history for and it finishes in less than 30 seconds....

I am looking at your suggestion now... I will let you know how/if that works for me, stay tuned!

 

[vongrunt]

This @row_count looks unnecessary.

Here is classic template for using a cursor:

declare blah cursor for	select ....
open blah
[b]fetch next from blah into @some_variables[/b]
while @@fetch_status = 0
begin
	-- do something
	[b]fetch next from blah into @some_variables[/b]
end

close blah
deallocate blah

Note the code in bold - first FETCH serves to "load a gun", second to loop over rows. If you keep it that way, nested cursors = nested templates:

declare blah cursor for	select ....
open blah
fetch next from blah into @some_variables
while @@fetch_status = 0
begin
	[b]declare nestedblah cursor for select ...
	fetch next from nestedblah into @some_nested_variables
	open nestedblah
	while @@fetch_status = 0
	begin
		-- do something 
		fetch next from nestedblah into @some_nested_variables
	end
	close nestedblah
	deallocate nestedblah[/b]
	
	fetch next from blah into @some_variables
end

close blah
deallocate blah

Important thing: @@FETCH_STATUS is affected after every FETCH - don't move FETCH code around, otherwise looping conditions will get broken.

These date conversions can be simplified... what is exact format of values @s_effdate and @S_termdate? yyyymmdd?

And of course this can be done without cursors... later .

------
heisenbug: A bug that disappears or alters its behavior when one attempts to probe or isolate it
schroedinbug: A bug that doesn't appear until someone reads source code and realizes it never should have worked, at which point the program promptly stops working for everybody until fixed.
[ba

 

[Veejc]

I am wide open to doing this without a cursor, just can't seem to figure it out.

The fetch_status for the outer loop generates an error and that was why I did the rowcount, it was for lack of being able to figure out the error, which I know is bad, but who has enough time to get stuff done anyway....

 

[vongrunt]

OK... is current performance level acceptable? 30 seconds for 80k rows doesn't look too bad. I'm asking that because no-cursors method is definitely less readable - and takes more time to write properly.

And... to repeat: what is exact format of values @s_effdate and @S_termdate? yyyymmdd?

------
heisenbug: A bug that disappears or alters its behavior when one attempts to probe or isolate it
schroedinbug: A bug that doesn't appear until someone reads source code and realizes it never should have worked, at which point the program promptly stops working for everybody until fixed.
[ba

 

[Veejc]

maswien

Thanks so much for your code!

Here's what I came up with using your template

drop table #temp_member
select absubno, abpersno, abeffdt as start_date
into #temp_member from dbo.tbl_CurrentMemberDimension
where eligibility_flag = 1

update #temp_member set start_date = h.min_date
from #temp_member p inner join
(select absubno, abpersno, min(abeffdt) as min_date
from dbo.jmeligm0_dat
group by absubno, abpersno) h
on p.absubno = h.absubno and p.abpersno = h.abpersno

select * from #temp_member


while @@rowcount > 0
begin
update #temp_member set start_date
=(select min(abeffdt) as start_date
from dbo.jmeligm0_dat
where datediff(day, abtermdt, #temp_member.start_date) <=90
and #temp_member.absubno = absubno and #temp_member.abpersno = abpersno )
where exists
(select * from dbo.jmeligm0_dat
where datediff(day, abtermdt, #temp_member.start_date) < 90
and datediff(day, abtermdt, #temp_member.start_date) > 0 )
end



Which works GREAT, however, I have to tell you that I stopped the query 6.5 minutes into it because I didn't know if I got something wrong somewhere and wasn't sure if I was in a never ending loop.... It was almost finished. So tomorrow I will run again and compare against the nested loops I wrote, but I have to say that the one time today I ran with the nested loops, it was finished in less than 3 minutes.

Is there something not optimized in my code? Did I forget something in the above code that could make it faster? As I mentioned, it worked GREAT, but not any faster, in fact a bit slower. Any ideas?

 

[Veejc]

Hi vongrunt...

Yes you have the correct format, it's yyyymmdd... you know I just realized that the code I wrote using Maswien's template, the last post I wrote, I didn't do anything to take that date format into account and it worked. I need to take another look at what I did and make sure I didn't do something wrong there,

but take a look at that and tell me if that's the kind of self join you had in mind and if I can optimize it even more... the 30 seconds I quoted earlier was not using the whole set of data, but 3 minutes is what it took when I used the whole set... I won't write it all over again, take a look at the previous post I wrote...

What do you think?

 

[vongrunt]

yyyymmdd format is unseparated string format, always recognized by SQL Server without any ambiguity. So instead of:

set @next_effdate = convert(datetime,left(@s_next_effdate,4) + '/' + substring(@s_next_effdate,5,2) + '/' + substring(@s_next_effdate,7,2) )

... this is enough:

set @next_effdate = @s_next_effdate

I'm not sure about self-joins. They can eat lots of resources, especially on larger data sets. And cursors for mostly read-only operations (as in your code) aren't so ugly. This is one of these try-and-measure things

There is third alternative: instead of nested cursors, create only one cursor then do nested looping.

------
heisenbug: A bug that disappears or alters its behavior when one attempts to probe or isolate it
schroedinbug: A bug that doesn't appear until someone reads source code and realizes it never should have worked, at which point the program promptly stops working for everybody until fixed.
[ba

 

[dpatrickcollins]

Hi Veejc:

Yes, I was missing a dimension of your spec, but here goes again. I am going to try to turn this thing on its head:

Thought: If you are truly looking for the earliest enrollment date, where enrollment equals the first date of continuous rows whose lapse do not exceed x days, then aren't you simply looking for the earliest of all rows that do not have a previous row representing continuous coverage? Code as follows:


[COLOR=green ]/*Select the exclusion of all rows you are about to define in subquery below */[/color]
SELECT MH.EnrollDate
FROM MemHistory MH
WHERE NOT EXISTS
(

[COLOR=green ]/*Selects all rows representing prior row if row above is continous coverage (not true "enrollment") */[/color]
SELECT
FROM MemHistory M_previous
WHERE
MH.MemID = M_previous.MemID AND
MH.EnrollDate <> M_previous.EnrollDate AND
DATEDIFF( day, MH.EnrollDate, M_previous.TerminationDate) < xdays )

)

note: I have purposely not muddied up the query with a group by to pull earliest for each member, for illustrative purposes, but this would be easy enough to do.)

 

[maswien]

I think dpatrickcollins deserve a star for his sticking to the right direction.

I tried following code in my test database, and it did return the correct results,

select t0.member_id, max(t0.start_date)
from history t0 
 where not exists 
 (select * from history t1
  where datediff(dd, t1.end_date, t0.start_date ) <= 90
  and datediff(dd, t1.end_date, t0.start_date ) > 0
  and t0.member_id = t1.member_id
 )
group by t0.member_id

 

[vongrunt]

If one member has more than one discontinuity, will this return correct results?

Definitely interesting, though.

------
heisenbug: A bug that disappears or alters its behavior when one attempts to probe or isolate it
schroedinbug: A bug that doesn't appear until someone reads source code and realizes it never should have worked, at which point the program promptly stops working for everybody until fixed.
[ba

 

[Veejc]

Hi again maswien

I am trying to finish up the example that you gave me, I think I may go with that method, however it only works for the first segment.

The while loop loops through the list that contains all the distinct member and not the segments, so wouldn't I need another loop inside of that?

Tell me if I'm missing something?

 

[maswien]

vongrunt,

If one member has more than one discontinuity, will this return correct results?

That's why there is a max(start_date) and group by. Here is how this query works:

list member_id and it's latest continous start_date
group by member_id.

'continous' means exist a end_date before it and less than 90 days


Veejc,

 however it only works for the first segment.

which example you use? the latest one?