I have UserSignatures
, a table with clientIP
s, some of which have CountryId
s 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?
-
1\$\begingroup\$ Please read "What should I do when someone answers my question?": "Do not add an improved version of the code after receiving an answer." \$\endgroup\$BCdotWEB– BCdotWEB2016年03月30日 13:40:37 +00:00Commented Mar 30, 2016 at 13:40
2 Answers 2
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
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;
-
\$\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\$dnoeth– dnoeth2016年03月30日 16:09:40 +00:00Commented Mar 30, 2016 at 16:09 -
\$\begingroup\$ @dnoeth many thanks for pointing that one out, I changed it to
BIGINT
! \$\endgroup\$Phrancis– Phrancis2016年03月30日 16:14:42 +00:00Commented Mar 30, 2016 at 16:14