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?

\$\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
\$\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;
\$\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\$
    – dnoeth
    Commented Mar 30, 2016 at 16:09
  • \$\begingroup\$ @dnoeth many thanks for pointing that one out, I changed it to BIGINT! \$\endgroup\$
    – Phrancis
    Commented Mar 30, 2016 at 16:14

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.