7
\$\begingroup\$

You're running a jeans company. Your system is collecting detailed orders' data, but it's a bit archaic and it's storing the number of units ordered per size in a delimited string with 20 "fields", each taking up 4 characters. You want to normalize this data to make it easier to eventually analyze orders at size level.

Given this string:

declare @input varchar(80);
set @input = '                       1   2   2   4       4       2   2   1                    ';

I want to extract the values, and store them in a way that enables me to correlate a field's index with the corresponding size in the size scale, which might look like this:

-- '                      28  30  32  34  35  36  37  38  40  42  44  46  48        '

Here's the I came up with:

create function dbo.GetSizeBreakdown(@input varchar(80))
returns @result table (SizeIndex int, Units int)
as
begin

    -- declare @result table (SizeIndex int, Units int);
    -- declare @input varchar(80);
    -- set @input = '                       1   2   2   4       4       2   2   1                    ';

    declare @fieldCount int = 20;
    declare @fieldLength int = 4;

    declare @index int = 0;
    declare @fieldValue varchar(4);

    while(@index < (@fieldCount))
    begin

        set @fieldValue = substring(@input, @index * @fieldLength + 1, @fieldLength)
        if (isnumeric(@fieldValue) != 0)
        begin
            insert into @result
            select @index + 1, cast(ltrim(@fieldValue) as int);
        end;

        set @index = @index + 1;
    end;

    -- select * from @result;
    return;
end;

select * from dbo.GetSizeBreakdown('                       1   2   2   4       4       2   2   1                    ');

Returns this:

    SizeIndex    Units
 1|  7            1
 2|  8            2
 3|  9            2
 4|  10           4
 5|  12           4
 6|  14           2
 7|  15           2
 8|  16           1

Which is perfect because from there I can put each quantity in its bucket, and call it a day.

I'm sure there's a better way of doing this though, even though it runs in 0ms. Right? Where, how and why can this function be optimized?

\$\endgroup\$
  • \$\begingroup\$ This field of fields is stored in the database this way, or it's coming in from a text file? \$\endgroup\$ – RubberDuck Jul 15 '14 at 4:10
  • \$\begingroup\$ It's coming from text files, dumped in a database. I'm creating the normalized schema :) \$\endgroup\$ – Mathieu Guindon Jul 15 '14 at 4:12
  • \$\begingroup\$ I'll try to come back and give a proper review, but in case I don't get around to it, take a look at pivot. \$\endgroup\$ – RubberDuck Jul 15 '14 at 4:27
4
\$\begingroup\$

The Good:

The code itself is pretty clean. You used clear and concise names and some of the best indentation I've ever seen in SQL. I would prefer that keywords like substring and select be ALLCAPS to differentiate them from fields and variables, but you were consistent.

The Bad:

  1. @fieldvalue is declared as varchar(4). You know exactly how long it is, so char(4) is a better choice of datatype. Even better yet would be to declare it as an int and let the RDBMS implicitly cast it so you don't have to later.
  2. This is a function that returns multiple fields for multiple records. Functions should only return a single field and record. This is better designed as a stored procedure. Scratch that. You used a table valued function.
  3. "dbo" stands for "Database Owner". The dbo schema should be reserved for database maintenance tasks and data. Business data and logic should be kept separate in its own schema(s).

The Ugly:

You're using Structured Query Language to do procedural programming. just isn't designed to do that efficiently. The code has to loop twenty times for each "record" you pass into it. I'm sure this works great for 1 row and is probably still good for 20,000 rows, but what about 200,000? 2,000,000? This just won't scale well. A set based approach would be better.

You could take two different approaches to this. What you do may depend on what you're able to do.

  1. If you have control over how the data is loaded from the text file, use an ETL (Extract-Transform-Load) tool to split the columns and unpivot them. A single field containing other fields shouldn't ever make it into the database to begin with. Assuming you have access to SSIS and BIDS (or whatever Microsoft named it for your version of SQL server), you would set up a Flat File fixed width datasource and a pivot transformation task.
  2. If you don't, you'll need to transform the data with an Un-Pivot Query after it's been loaded from the text file. That would look something like this.

(Note that it contains some setup code for my testing.)

CREATE TABLE #OneBigBucket(
    FieldOfFields char(80)
);

INSERT INTO #OneBigBucket
VALUES('                       1   2   2   4       4       2   2   1                    ');

-- The interesting stuff
SELECT SizeIndex,Units
FROM(
    SELECT 
        SUBSTRING(FieldOfFields,1,4) AS [1],
        SUBSTRING(FieldOfFields,5,4) AS [2],
        SUBSTRING(FieldOfFields,9,4) AS [3],
        SUBSTRING(FieldOfFields,13,4) AS [4],
        SUBSTRING(FieldOfFields,17,4) AS [5],
        SUBSTRING(FieldOfFields,21,4) AS [6],
        SUBSTRING(FieldOfFields,25,4) AS [7],
        SUBSTRING(FieldOfFields,29,4) AS [8],
        SUBSTRING(FieldOfFields,33,4) AS [9],
        SUBSTRING(FieldOfFields,37,4) AS [10],
        SUBSTRING(FieldOfFields,41,4) AS [11],
        SUBSTRING(FieldOfFields,45,4) AS [12],
        SUBSTRING(FieldOfFields,49,4) AS [13],
        SUBSTRING(FieldOfFields,53,4) AS [14],
        SUBSTRING(FieldOfFields,57,4) AS [15],
        SUBSTRING(FieldOfFields,61,4) AS [16],
        SUBSTRING(FieldOfFields,65,4) AS [17],
        SUBSTRING(FieldOfFields,69,4) AS [18],
        SUBSTRING(FieldOfFields,73,4) AS [19],
        SUBSTRING(FieldOfFields,77,4) AS [20]
    FROM #OneBigBucket) buckets
UNPIVOT 
    (Units FOR SizeIndex IN
        ([1],[2],[3],[4],[5],
        [6],[7],[8],[9],[10],
        [11],[12],[13],[14],[15],
        [16],[17],[18],[19],[20])
    ) AS unpvt
WHERE Units <> '';

DROP TABLE #OneBigBucket;

Is it pretty? No. Is it set based? Yes.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ It's a table-valued function, a full-fledged citizen of the SQL Server fauna, and I need it that way to be able to select and join with its result - a stored procedure doesn't let me do that. \$\endgroup\$ – Mathieu Guindon Jul 15 '14 at 13:36
  • \$\begingroup\$ Hmm the SSIS part can't work. I wasn't clear about the flatfiles part - the files are already loaded in a db, what I need to parse is actually stored in a varchar(80) field. \$\endgroup\$ – Mathieu Guindon Jul 15 '14 at 17:23
  • \$\begingroup\$ I wasn't sure how much access you had to it, so I wanted to provide both options. Replace #OneBigBucket with whatever table the data is already loaded into. (just don't drop it =P) \$\endgroup\$ – RubberDuck Jul 15 '14 at 17:55
  • 1
    \$\begingroup\$ @ckuhn203 kudos for your set-based approach. It is indeed ugly, but easy to understand and probably a lot faster on a large dataset. \$\endgroup\$ – Phrancis Jul 15 '14 at 23:55
4
\$\begingroup\$

It has been a while since I messed with SQL Server procedures, but, in principle, I don't mind your solution. The while loop is not set based, but that is not insanely slow, either. For a limit of 20 fields I don't see it being a real problem.

SQL often ends up being a cacophony of copy/paste, and a set-based solution using raw substring operations is possible. @ckuhn203 has shown that. I am not a huge fan of all the UNPIVOT and other operations though. I would consider a much simpler (though heavily copy/pasted) version:

create function GetSizeBreakdown(@input varchar(80))
returns @result table (SizeIndex int, Units int)
as
begin


    with fields as (
      
      select 1 as field, LTRIM(SUBSTRING(@input,1,4)) AS value
      UNION ALL
      select 2 as field, LTRIM(SUBSTRING(@input,5,4)) AS value
      UNION ALL
      select 3 as field, LTRIM(SUBSTRING(@input,9,4)) AS value
      UNION ALL
      select 4 as field, LTRIM(SUBSTRING(@input,13,4)) AS value
      UNION ALL
      select 5 as field, LTRIM(SUBSTRING(@input,17,4)) AS value
      UNION ALL
      select 6 as field, LTRIM(SUBSTRING(@input,21,4)) AS value
      UNION ALL
      select 7 as field, LTRIM(SUBSTRING(@input,25,4)) AS value
      UNION ALL
      select 8 as field, LTRIM(SUBSTRING(@input,29,4)) AS value
      UNION ALL
      select 9 as field, LTRIM(SUBSTRING(@input,33,4)) AS value
      UNION ALL
      select 10 as field, LTRIM(SUBSTRING(@input,37,4)) AS value
      UNION ALL
      select 11 as field, LTRIM(SUBSTRING(@input,41,4)) AS value
      UNION ALL
      select 12 as field, LTRIM(SUBSTRING(@input,45,4)) AS value
      UNION ALL
      select 13 as field, LTRIM(SUBSTRING(@input,49,4)) AS value
      UNION ALL
      select 14 as field, LTRIM(SUBSTRING(@input,53,4)) AS value
      UNION ALL
      select 15 as field, LTRIM(SUBSTRING(@input,57,4)) AS value
      UNION ALL
      select 16 as field, LTRIM(SUBSTRING(@input,61,4)) AS value
      UNION ALL
      select 17 as field, LTRIM(SUBSTRING(@input,65,4)) AS value
      UNION ALL
      select 18 as field, LTRIM(SUBSTRING(@input,69,4)) AS value
      UNION ALL
      select 19 as field, LTRIM(SUBSTRING(@input,73,4)) AS value
      UNION ALL
      select 20 as field, LTRIM(SUBSTRING(@input,77,4)) AS value

    )
    insert into @result (SizeIndex, Units)
    select field, cast(value as int)
    from fields
    where value <> ''


    return

end

A better solution would be to encode the fields in a lookup table.... something like:

create table SizeFieldParse (
    field int  PRIMARY KEY not null,
    start int not null,
    len int not null);

insert into SizeFieldParse values (1, 1, 4);
insert into SizeFieldParse values (2, 5, 4);
insert into SizeFieldParse values (3, 9, 4);
insert into SizeFieldParse values (4, 13, 4);
insert into SizeFieldParse values (5, 17, 4);
insert into SizeFieldParse values (6, 21, 4);
insert into SizeFieldParse values (7, 25, 4);
insert into SizeFieldParse values (8, 29, 4);
insert into SizeFieldParse values (9, 33, 4);
insert into SizeFieldParse values (10, 37, 4);
insert into SizeFieldParse values (11, 41, 4);
insert into SizeFieldParse values (12, 45, 4);
insert into SizeFieldParse values (13, 49, 4);
insert into SizeFieldParse values (14, 53, 4);
insert into SizeFieldParse values (15, 57, 4);
insert into SizeFieldParse values (16, 61, 4);
insert into SizeFieldParse values (17, 65, 4);
insert into SizeFieldParse values (18, 69, 4);
insert into SizeFieldParse values (19, 73, 4);
insert into SizeFieldParse values (20, 77, 4);

create function GetSizeBreakdown(@input varchar(80))
returns @result table (SizeIndex int, Units int)
as
begin

    with fields as (
      select field, LTRIM(SUBSTRING(@input, start, len)) as value
      from SizeFieldParse
    )
    insert into @result (SizeIndex, Units)
    select field, cast(value as int)
    from fields
    where value <> ''


    return

end

I put this together in an SQLFiddle

Bug

When I compared my results against yours, I found my index was off-by-one.

With the input you gave, I get the first index at field 6, but you have output showing field 7. I have manually gone through the parsing, and I am sure field 6 is right.... so I think you have a problem ... ;-)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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