4

I have a query that needs to check that all fields have values are in a list of valid codes. Right now I'm calling the same subquery over and over and over again. I want to abstract the subquery out so that it is faster and the code isn't repeated. This is the query in question:

select count(*)
into cnt
from pdv_validcodes c
where c.code_type = 'YNNA'
 and (upper(:new.spec_1) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_1 is null)
 and (upper(:new.spec_2) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_2 is null)
 and (upper(:new.spec_3) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_3 is null)
 and (upper(:new.spec_4) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_4 is null)
 and (upper(:new.spec_5) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_5 is null)
 and (upper(:new.spec_6) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_6 is null)
 and (upper(:new.spec_7) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_7 is null)
 and (upper(:new.spec_8) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_8 is null)
 and (upper(:new.spec_9) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_9 is null)
 and (upper(:new.spec_10) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.spec_10 is null)
 and (upper(:new.add_spec_1) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.add_spec_1 is null)
 and (upper(:new.add_spec_2) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.add_spec_2 is null)
 and (upper(:new.add_spec_3) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.add_spec_3 is null)
 and (upper(:new.add_spec_4) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.add_spec_4 is null)
 and (upper(:new.add_spec_5) in
     (select code from pdv_validcodes where code_type = 'YNNA') or
     :new.add_spec_5 is null);
7
  • 2
    As a side note, that's a pretty horrible, lazy database design...
    – Blindy
    Aug 11, 2011 at 17:59
  • It absolutely is. Another set of columns goes up to 33 (in this same table). It's coming from an external source though, so unfortunately not much can be done about it. Aug 11, 2011 at 18:04
  • 1
    If it's coming from an external source and being loaded into your database, then remember that the "T" in ETL stands for Transform
    – Tom H
    Aug 11, 2011 at 18:14
  • Does this have to be done in SQL? It looks suspiciously like code in a PL/SQL trigger, if so there is a much better way of achieving your goal.
    – Ollie
    Aug 11, 2011 at 18:28
  • 3
    Have you considered defining foreign key constraints on all the spec_<n> fields? Then you wouldn't need a trigger to check that they valid values. Since the set of valid values looks like it's a subset of pdv_validcodes.code, you might need to do something extra to make this work, like maybe: a) include a code_type field in the target table with a fixed value of 'YNNA' that you can include in a foreign key constraint referencing pdv_validcodes, or, b) create a third table that contains only the 'YNNA' codes and reference this in the foreign key constraint on the target table. Aug 11, 2011 at 21:26

6 Answers 6

3

Michael,

I haven't had a chance to test this but as it's trigger code and therefore PL/SQL, something along the lines of this might work:

CREATE OR REPLACE TYPE "strarray" AS TABLE OF VARCHAR2 (255)
/


DECLARE
   validcodes strarray;
BEGIN
   SELECT code 
     BULK COLLECT INTO validcodes
     FROM pdv_validcodes
    WHERE code_type = 'YNNA'
   UNION
   SELECT 'NULL'
     FROM dual;

   IF  NVL(upper(:new.spec_1), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_2), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_3), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_4), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_5), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_6), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_7), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_8), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_9), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.spec_10), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.add_spec_1), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.add_spec_2), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.add_spec_3), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.add_spec_4), 'NULL') MEMBER OF validcodes
   AND NVL(upper(:new.add_spec_5), 'NULL') MEMBER OF validcodes
   THEN
      -- Business logic
   ELSE
      -- Business logic
   END IF;
END;
2
  • 1
    If this is part of a frequently used trigger you may want to store the nested table validcodes in a package and populate it in a before-table trigger (so the codes are only collected once per statement, not once per row). Also, you should probably remove the quotes around "strarray", or use an existing type.
    – Jon Heller
    Aug 12, 2011 at 4:25
  • @jonearles. I totally agree, i wrote it all in one go and it was quite late. Populating the nested table in a package once per transaction would be very beneficial depending on the volume of DML.
    – Ollie
    Aug 12, 2011 at 6:06
2

Depending on the Oracle version, you could use the WITH clause to factor out subqueries. I'm not sure that buys you too much in this case, but

with valid as (
  select code
    from pdv_validcodes
   where code_type = 'YNNA' )
select count(*)
into cnt
from pdv_validcodes c
where c.code_type = 'YNNA'
 and (upper(:new.spec_1) in
     (select * from valid) or
     :new.spec_1 is null)
 and (upper(:new.spec_2) in
     (select * from valid) or
     :new.spec_2 is null)
 and (upper(:new.spec_3) in
     (select * from valid) or
     :new.spec_3 is null)
...
1
  • Thanks, I thought of this before and was hoping I could get something that got me a little more, but it is certainly better than what I had. Aug 11, 2011 at 20:18
1

You can encapsulate the code in a VIEW. This won't help with performance, but it will help with readability and mistakes from improperly cut-n-pasting and should improve maintainability.

The best option would be to redesign that table though.

1

to check that all fields have values are in a list of valid codes

Something along these lines should do

select sys.dbms_debug_vc2coll( 
        :new.spec_1 , :new.spec_2 , :new.spec_3 , :new.spec_4 , :new.spec_5 ,
        :new.spec_6 , :new.spec_7 , :new.spec_8 , :new.spec_9 , :new.spec_10,
        :new.add_spec_1, :new.add_spec_2 , :new.add_spec_3 , :new.add_spec_4 )
       multiset except distinct
         (select cast(collect(code) as sys.dbms_debug_vc2coll)
          from pdv_validcodes where code_type = 'YNNA')
from dual;

I've used sys.dbms_debug_vc2coll but you can create your own collection type [CREATE TYPE tab_char AS TABLE OF VARCHAR2(20)]

If the query returns anything other than a set of one null value then those are the mismatched values.

Personally, I'd consider ignoring the check and make sure there's referential constraints on the DB and simply use DML error logging to handle any dodgy values.

1

I suggest a slightly different approach to the problem: materialize that list of :new. values as a result set. That would allow you to process it like you would a table.

This query below will return a count of the :new. values that are NOT NULL and do not match a code in the pdv_validcodes table.


SELECT COUNT(1)
  INTO cnt
  FROM (
         SELECT q.spec
           FROM ( SELECT :new.spec_1 AS spec FROM DUAL
                  UNION ALL SELECT :new.spec_2 FROM DUAL
                  UNION ALL SELECT :new.spec_3 FROM DUAL
                  UNION ALL SELECT :new.spec_4 FROM DUAL
                  UNION ALL SELECT :new.spec_5 FROM DUAL
                  UNION ALL SELECT :new.spec_6 FROM DUAL
                  UNION ALL SELECT :new.spec_7 FROM DUAL
                  UNION ALL SELECT :new.spec_8 FROM DUAL
                  UNION ALL SELECT :new.spec_9 FROM DUAL
                  UNION ALL SELECT :new.spec_10 FROM DUAL
                  UNION ALL SELECT :new.add_spec_1 FROM DUAL
                  UNION ALL SELECT :new.add_spec_2 FROM DUAL
                  UNION ALL SELECT :new.add_spec_3 FROM DUAL
                  UNION ALL SELECT :new.add_spec_4 FROM DUAL
                  UNION ALL SELECT :new.add_spec_5 FROM DUAL
               ) q WHERE q.spec IS NOT NULL
       ) p
  LEFT
  JOIN pdv_validcodes c
    ON c.code = UPPER(p.spec) AND c.code_type = 'YNNA'
 WHERE c.code IS NULL

Here's how it works:

First, we return the list of :new. values as a result set. (That's the inline view aliased as q.)

Next, we exclude any NULL values from that result set. (That's the inline view aliased as p.)

Next, we join that result set with the pdv_validcodes table. (We match only to the 'YNNA' code_type, and we do the matching as an OUTER join (LEFT JOIN) so that we return all the rows from the p result set, whether or not they match a code in the pdv_validcodes table.

As the final step, we exclude any rows for which we found a match (c.code will be NULL where rows from p did not have a match) , leaving us with a list of :new. values for which there was no match.


NOTES:

This query will return a count of zero when all the :new. values are matched, and will return a non-zero count if there are any :new. values for which a match is not found (which I think is inverted from the original)

This may not be the best way to perform the operation, but it does eliminate a lot of the redundant code that was in the original query.

The 'YNNA' literal is specified only once, and each of the :new. expressions is specified only once.

I am assuming that the datatypes of all of those :new. expressions are compatible (e.g. are all VARCHAR), since we do note that they are all being compared to the code column. If they aren't, then there's some implicit datatype conversion going on in the original query, which will probably need to be made explicit in this, so that the UNION ALL operation will work.)

A Common Table Expression would work as a replacement for the (old-school) inline view.

This code is not yet tested.

0

The relational operator you require is division, popularly known as "the supplier who supplies all parts".

Things to consider: exact division or division with remainder?; how to handle an empty divisior (e.g. if the parts list to supply is the empty set, logically all suppliers can supply it but is it more practical to evaluate to no suppliers).

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.