2
\$\begingroup\$

I have a system that outputs a comma separated string into a single column. I need to grab the individual values out of that list. The list could have random spaces and commas that need to be accounted for. I'm new to MySQL and I have a working example that just seems rather long. I'm wondering if there is a simpler way to achieve this.

delete from example;

drop procedure if exists testProc;

DELIMITER ;;
CREATE PROCEDURE `testProc`(
  in p_willBeUsedLater int(10) unsigned
)
    MODIFIES SQL DATA
myProc:begin

DECLARE theValue varchar(10);

/* Test Cases - PASSED
DECLARE myList varchar(255) default '';
DECLARE myList varchar(255) default null;
DECLARE myList varchar(255) default '1';
DECLARE myList varchar(255) default '1,2';
*/

/* This passes as well. */
DECLARE myList varchar(255) default ' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ';

SET myList = trim(myList);

/* If first char is a comma remove it. */
WHILE (LOCATE(',',myList) = 1)
DO
    SET myList = SUBSTR(myList, 2);
    SET myList = trim(myList); /* could have been spaces between internal commas */
END WHILE;

/* If last char is a comma remove it. */
SET myList = REVERSE(myList);
WHILE (LOCATE(',',myList) = 1)
DO
    SET myList = SUBSTR(myList, 2);
    SET myList = trim(myList); /* could have been spaces between internal commas */
END WHILE;
/* Put back to correct order. */
SET myList = REVERSE(myList);

WHILE (length(myList) > 0)
DO
    IF (LOCATE(',',myList) = 0) THEN
        SET theValue = trim(myList);
        SET myList = '';
    ELSE
        SET theValue = trim(substring(myList,1,LOCATE(',',myList)-1));
        SET myList = trim(substring(myList,LOCATE(',',myList)+1,length(myList)));
    END IF;

    INSERT INTO `EXAMPLE`(text1, text2) VALUES(myList, theValue);
END WHILE;
end;;
DELIMITER ;

call testProc(36);

select * from example;

Successful output:

    text1                      text2
--------------------------------------------
    2  ,3, 456 , 7 8 9 ABC         1
    3, 456 , 7 8 9 ABC             2
    456 , 7 8 9 ABC                3
    7 8 9 ABC                  456
                               7 8 9 ABC

text1 just shows the current state of the list shrinking with each loop. text2 is the money column, that's the correct data I need.

\$\endgroup\$
5
\$\begingroup\$

Example-ish code

It's clear that you have anonymized the procedure code by replacing real names by example names. In the future, it is better to use your real object & variable names, as someone may actually have things to say to improve your naming, which is one of the most important aspects to code clarity/readability.


Structure of procedure

There are some really strange things about the way your procedure is structured. For one, the test string that it's meant to parse is hard-coded into the procedure code.

/* This passes as well. */
DECLARE myList varchar(255) default ' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ';

I feel sure that this is temporary, but if one of my colleague SQL developers handed that to me for a code review, I would tell them they need to go finish the code first so it's actually usable for any test case. So the string to parse should be an argument to the procedure...

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255),
  in p_willBeUsedLater int(10) unsigned
)

And as such, you can just document your test cases in block comment with procedure calls. This also documents to a reasonable extent how the procedure is meant to be used.

/* Test Cases - PASSED
call sp_ParseCommaSeparatedString('');   --0 rows
call sp_ParseCommaSeparatedString(null); --0 rows
call sp_ParseCommaSeparatedString('1');  --1 row
call sp_ParseCommaSeparatedString('1,2');--2 rows
--Complex test case, 5 rows
call sp_ParseCommaSeparatedString(' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ');
*/

Unused parameter

This is dead code, as the name indicates: in p_willBeUsedLater int(10) unsigned

It would be more beneficial to not add it in until it is ready, so that the stored procedure calls aren't unnecessarily confusing. You would have 2 options here which would be better. MySQL is a bit clunky for something like this, as far as SQL goes, but it can still be done fine.

1) Document as a TODO

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255)
)
  MODIFIES SQL DATA
myProc:begin

/* 
 * TODO: implement additional input param:
 *   in p_someSensiblyNamedNumber int(10) unsigned
 * This parameter will be used for [...]
 */

2) Provide a default argument

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255),
  in p_someSensiblyNamedNumber int(10) unsigned
)
  MODIFIES SQL DATA
myProc:begin

IF p_someSensiblyNamedNumber IS NULL THEN 
  SET p_someSensiblyNamedNumber = 36;

Either will work fine and which is better really depends on what you are needing to do with it. From the code your presented, that parameter is not used for anything right now and it's not clear from what you named it what it is supposed to do, so... hard to say.

Now we can clean up the top a bit.

DECLARE stringToParse varchar(255);
SET stringToParse = TRIM(p_commaSeparatedString);

Results table

At the moment, you are finding yourself having to delete the rows from the table before you call the procedure, which is not ideal at all and will lead to errors. Instead, do that in your procedure, either at the top when you start, or just before you start inserting values in it (in case the code prior to that takes long, this would mitigate the possibility of another process adding rows into the same table which this is running).

Just check if there are any rows in it, if there are not just skip, and if there are, delete them.

IF EXISTS (SELECT 1 FROM example) THEN
    DELETE FROM example;
END IF;

WHILE (length(myList) > 0)
DO...

Overall approach

SQL clearly was not designed to parse strings of text like a more general language/domain could; if things are not in tables, rows and columns, they are much more difficult to manipulate with SQL. With that in mind, I would say your logic is probably about as good as it is going to get, while still maintaining readability.

Perhaps consider adding a small amount of code comments in the conditionals in your last WHILE section so that the next person who has to look and/or debug this code (which may very well be your future self) will not have too much difficulty.


Nitpicks

DELIMITER ;;

This would make more sense if you set the delimiter to something that looks less like the normal delimiter. I have most often seen used $$, // and || as delimiters.

trim(substring(myList,LOCATE(',',myList)+1,length(myList)));

Try to get in the habit of spacing your operators, commas, etc. to make it easier to read. Also, you are using mixed capitalization of keywords, which just looks a bit messy. Pick one capitalization style and stick to it.

TRIM(SUBSTRING(myList, LOCATE(',', myList) + 1, LENGTH(myList)));
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for all the details above! Sorry, I guess I should have been clearer. Everything you mention I'm aware of and they were done that way so my intentions were clear and it could easily be ran by others w/o modification. Yes, it the real world the string is passed in along w/ other things. I was really wanting to know if "the guts" of the proc, the main task at hand, could be written more efficiently. In learning MySql I determined that WHILE..DO and string manipulation was the way to go. I don't know if I missed an easier route. Thanks! \$\endgroup\$ – gfrobenius Mar 30 '16 at 15:21
  • \$\begingroup\$ For example: In Oracle I would have just looped a query like this: stackoverflow.com/a/28677266/3112803 then trimmed resulting data afterwards. I'm finding it harder to do the things I could easily do in Oracle. Thanks! \$\endgroup\$ – gfrobenius Mar 30 '16 at 15:23
  • 1
    \$\begingroup\$ @gfrobenius That's an unfortunate fact with MySQL: it cannot do many of the things that other (arguably better) DBMS can do. I mostly use T-SQL in my work and really find myself dis-empowered when I have to use MySQL for anything beyond basic queries. \$\endgroup\$ – Phrancis Mar 30 '16 at 15:41

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.