6
\$\begingroup\$

This code is written in a SSIS Script Component that basically accomplishes what I previously had as a T-SQL script, that was reviewed here.

I need to split a 80 character string that contains 20 buckets of 4 characters each, without any delimiter, and generate a new row for each bucket. I'm using a script component to achieve this in SSIS - here's the code:

using System;
using System.Linq;
using System.Text.RegularExpressions;
using System.Data;
using Microsoft.SqlServer.Dts.Pipeline;
using Microsoft.SqlServer.Dts.Pipeline.Wrapper;
using Microsoft.SqlServer.Dts.Runtime.Wrapper;

[SSISScriptComponentEntryPoint]
public class ScriptMain : UserComponent
{
    /// <summary>
    /// This code splits a row's [Sizes] column into buckets of 4 characters,
    /// and adds a new row to the output buffer for each non-empty bucket found.
    /// </summary>
    /// <param name="Row">The row that is currently passing through the component</param>
    public override void Input0_ProcessInputRow(Input0Buffer Row)
    {
        var buckets = Regex.Match(Row.Sizes, "^(?<bucket>.{4})+$")
                           .Groups["bucket"].Captures
                           .Cast<Capture>()
                           .Select(capture => capture.Value)
                           .ToArray();
        try
        {
            for (int index = 0; index < buckets.Length; index++)
            {
                var bucket = buckets[index].Trim();
                if (bucket.Contains(' '))
                {
                    // size codes don't contain whitespaces; something went really wrong if that's the case:
                    throw new FormatException("Size code '" + bucket +"' contains a whitespace. This is interpreted as a parse error.");
                }

                if (string.IsNullOrEmpty(bucket))
                {
                    // don't output a row for empty buckets
                    continue;
                }

                OutputSizesBuffer.AddRow();
                OutputSizesBuffer.ETLStart = Row.ETLStart;
                OutputSizesBuffer.SourceId = Row.SourceId;
                OutputSizesBuffer.SourceTable = Row.SourceTable;
                OutputSizesBuffer.SizeRangeId = Row.SizeRangeId;
                OutputSizesBuffer.SizeRangeIndex = index + 1; // make it 1-based.
                OutputSizesBuffer.Code = bucket;
            }
        }
        catch (Exception exception)
        {
            var cancel = false;
            ComponentMetaData.FireError(0, "Script Component", exception.ToString(), string.Empty, 0, out cancel);
        }
    }

}

I would like this code to be reviewed from a performance perspective, because I will be needing very similar script components soon, to process hundreds of thousands of rows; this specific instance works well, but only processes the "master data", which is only a dozen rows or so: if there's a performance issue with this code, it's only later that I'm going to find out - I'd rather get it peer reviewed now and make adjustments before I use similar code to process many, many rows.

Is the error/exception handling overkill? Is the regex pattern self-documenting? (wait, is that even possible?)

\$\endgroup\$
7
\$\begingroup\$

The regex silently fails on inputs that don't have length a multiple of 4; i.e. no results are returned. Things that fail silently are incredibly hard to debug. It's easy to imagine trying to figure out why no rows are being inserted for the input

jvqbjqbxvvlxlcwkuivasoeljaxamrthcecbudnxvqdboxroudfvrwoureqisaldcduhauoqtxdclcw

until you realise that it only has 79 characters.

You would want to measure the performance, but I would suggest not using a regex and writing a simple method, like so

private static IEnumerable<string> ToBuckets(string input)
{
    const int Length = 4;
    for (var i = 0; i < input.Length; i += Length)
    {
        yield return input.Substring(i, Length);
    }
}

We should add input validation to make it easier to find out exactly what went wrong.

if (input == null)
{
    throw new ArgumentNullException("input");
}

if (input.Length % Length != 0)
{
    throw new ArgumentException(string.Format("Argument length must be a multiple of {0}", Length));
}

I made a microbenchmark comparing the two methods that you can find here. For 100,000 calls, the regex solution takes ~2.5s compared to ~0.2s for this version.


It looks like you're using exceptions for control flow with the FormatException. I would suggest making the try-catch more specific, both in the code that is executed within the try, and in the exceptions that you're catching.

If we split out the OutputSizesBuffer parts to its own method, we end up with something like this, which I find a bit cleaner (illustrative code only)

var buckets = ToBuckets(Row.Sizes).Select(bucket => bucket.Trim()).ToArray();
for (int index = 0; index < buckets.Length; index++)
{
    // Don't output a row for empty buckets.
    if (bucket.Length == 0)
    {
        continue;
    }

    if (bucket.Contains(' '))
    {
        // Size codes don't contain whitespace; something went really wrong if that's the case.
        FireError(string.Format(Resources.BucketContainsWhiteSpace, bucket));
        return;
    }

    try
    {
        AddRow(index + 1, bucket);
    }
    catch (SomeSpecificException e)
    {
        FireError(e.Message);
        return;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I like how your answer makes me want to overcome the overwhelming temptation to cram everything into the Input0_ProcessInputRow auto-generated method. And I knew the RegEx was going to be a bit of a performance downer... but wow. #downwithregex \$\endgroup\$ – Mathieu Guindon Nov 28 '14 at 0:17
6
\$\begingroup\$

Is the error/exception handling overkill?

No. It is definitely not overkill. Particularly because you're throwing an error on certain conditions. This gives you the option of redirecting rows into an error output of some kind. Which in turn gives you a chance to inspect the bad data and hopefully correct it. I find this to be a wonderful practice and I wish more database devs did it. I've seen far too many instances of Script Tasks returning success no matter what happens to not like what you've done.

Is the regex pattern self-documenting?

Yes. I think so at least.

  • begin at start of line
  • Create a group named bucket
  • match any character four times
  • end group
  • match one or more bucket
  • end of line

Pretty straightforward as far as regular expressions go.

// make it 1-based.

Comments should say why, not what. It's cool that you're letting the maintainer know what's going on here, but it would be nice to let them know why too.

Performance will need to be addressed by someone who actually knows what they're talking about... I've no idea.

\$\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.