2
\$\begingroup\$

I have UserSignatures, a table with clientIPs, some of which have CountryIds that are null and need to be filled in.

A second table, IpLookups, stores IPs, and a third table Country has the needed CountryId. This is workaround at the moment, because we don't fill CountryIp yet.

declare @var1 int,@var2 int,@var3 int,@ind int = 0;
SELECT * INTO #Temp
FROM [dbo].[UserSignatures]
where CountryId is null
SELECT @var1 = count(*) FROM #Temp;
while (@ind < @var1 ) 
begin
 select @var3 = Id from #Temp order by Id OFFSET @ind ROWS FETCH NEXT 1 ROW ONLY;
 select @var2 = CONVERT(BIGINT, PARSENAME(u.ClientIp, 1)) * POWER(256, 0)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 2)) * POWER(256, 1)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 3)) * POWER(256, 2)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 4)) * POWER(256, 3) from [dbo].[UserSignatures] u
 where u.Id = @var3
 update [dbo].[UserSignatures]
 set CountryId = (
 SELECT c.Id FROM dbo.IpLookups lo with(NOLOCK)
 inner join
 [dbo].[Countries] c with(nolock)
 on
 c.Code = lo.CountryIsoCode
 WHERE 
 @var2
 BETWEEN lo.[From] AND lo.[To]
 )
 where Id = @var3
 SET @ind = @ind+1;
end
DROP table #Temp

Is there any way to simplify this?

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Mar 30, 2016 at 12:12
\$\endgroup\$
1

2 Answers 2

6
\$\begingroup\$

You process data Row By Agonizing Row instead of a single Update statement.

There's no need for temp tables, variables and a loop, simply write a single SELECT and use this in an UPDATE FROM. This should return the correct country ids:

 SELECT u.id, c.Id 
 FROM [dbo].[UserSignatures] AS u
 JOIN dbo.IpLookups AS lo
 ON CONVERT(BIGINT, PARSENAME(u.ClientIp, 1)) * POWER(256, 0)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 2)) * POWER(256, 1)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 3)) * POWER(256, 2)
 + CONVERT(BIGINT, PARSENAME(u.ClientIp, 4)) * POWER(256, 3) 
 BETWEEN lo.[FROM] AND lo.[TO]
 JOIN [dbo].[Countries] AS c
 ON c.Code = lo.CountryIsoCode
 WHERE u.CountryId IS NULL
answered Mar 30, 2016 at 13:09
\$\endgroup\$
2
\$\begingroup\$

While dnoeth's answer provides a good solution I thought the original code itself could still be improved on some general aspects.


Naming

declare @var1 int,@var2 int,@var3 int,@ind int = 0;

These names do not say anything about what the variables are used for. Try to use names that mean something, especially for SQL query code that is going into stored procedures, functions, etc. to make it more maintainable.

These would make more sense:

declare
 @MaxIndex int, -- @var1
 @IpAsDecimal int, -- @var2 - more on this one below
 @UserSignatureId int, -- @var3
 @index int = 0;

This #Temp is also a poor name:

SELECT * INTO #Temp
FROM [dbo].[UserSignatures]
where CountryId is null

Just call it what it is: #UserSignaturesWithoutCountryId

This may seem silly in this small of a script, but when you start writing stored procedures that are hundreds of lines long and have multiple temp tables, it will get confusing really quickly.

On another note, the only field you use from #Temp is Id so there is no point to putting the other fields from [dbo].[UserSignatures].

SELECT Id 
INTO #UserSignaturesWithoutCountryId
FROM [dbo].[UserSignatures]
WHERE CountryId IS NULL;

IPv4 address decimal conversion

This section is not immediately clear as to what it is meant to do, I had to break it down into smaller pieces to make sense of it

select 
 @IpAsDecimal = 
 CONVERT(BIGINT, PARSENAME(@ip, 1)) * POWER(256, 0)
 + CONVERT(BIGINT, PARSENAME(@ip, 2)) * POWER(256, 1)
 + CONVERT(BIGINT, PARSENAME(@ip, 3)) * POWER(256, 2)
 + CONVERT(BIGINT, PARSENAME(@ip, 4)) * POWER(256, 3) 
 from [dbo].[UserSignatures] u
 where u.Id = @UserSignatureId;

For something like this you would probably want to create a User-Defined Function (or UDF), for example this scalar function.

IF EXISTS (SELECT * FROM sysobjects WHERE id = object_id(N'dbo.fn_IPv4DotNotationToDecimal') AND xtype IN (N'FN', N'IF', N'TF'))
 DROP FUNCTION dbo.fn_IPv4DotNotationToDecimal;
CREATE FUNCTION dbo.fn_IPv4DotNotationToDecimal (@Ipv4AddressString varchar(15))
RETURNS BIGINT
AS
BEGIN
/* Returns the decimal version of an IPv4 address stored as a string.
 * Ex: '192.168.1.2' returns 3232235778 */
 DECLARE @IPv4Decimal BIGINT;
 SET @IPv4Decimal = 
 CONVERT(BIGINT, PARSENAME(@Ipv4AddressString, 1)) * POWER(256, 0)
 + CONVERT(BIGINT, PARSENAME(@Ipv4AddressString, 2)) * POWER(256, 1)
 + CONVERT(BIGINT, PARSENAME(@Ipv4AddressString, 3)) * POWER(256, 2)
 + CONVERT(BIGINT, PARSENAME(@Ipv4AddressString, 4)) * POWER(256, 3);
 
 IF (@IPv4Decimal IS NULL) SET @IPv4Decimal = 0;
 RETURN @IPv4Decimal;
END;

Then you can simply do this in the above query (and every other query that uses this type of operation):

select 
 @IpAsDecimal = dbo.fn_IPv4DotNotationToDecimal(u.ClientIp)
 from [dbo].[UserSignatures] u
 where u.Id = @UserSignatureId;
answered Mar 30, 2016 at 15:32
\$\endgroup\$
2
  • \$\begingroup\$ Your UDF will fail because you need an unsigned int to return 3232235778. Either switch to BIGINTor to subtract 2147483648` to match the integer range. \$\endgroup\$ Commented Mar 30, 2016 at 16:09
  • \$\begingroup\$ @dnoeth many thanks for pointing that one out, I changed it to BIGINT! \$\endgroup\$ Commented Mar 30, 2016 at 16:14

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.