-1

Wrote the following PLSQL script to produce a report and am getting the error messages

Error at line 5
ORA-06550: line 61, column 18:
PLS-00103: Encountered the symbol "1" when expecting one of the following:
   (

I've been through the code many times and I cannot find the error. Any help would be greatly appreciated. I'm currently working in Oracle Database 11g Enterprise Edition Release 11.2.0.3.0 - 64bit Production

SET serveroutput ON size 1000000;

DECLARE
    TYPE TITLE_RECORD_TYPE IS RECORD
        (id                     number(19),
         gaid                   varchar2(20),
         artist_legal_name      varchar2(510),
         artist_display_title   varchar2(510),
         display_title          varchar2(510),
         category               varchar2(255),
         type                   varchar2(255),
         sub_type               varchar2(255));

    TITLE_RECORD            TITLE_RECORD_TYPE;

    v_title                     varchar2(510);
    v_artist                    varchar2(510);

    v_total_rows_error          number(20) := 0;
    v_row_count                 number(10) := 0;

    v_error_desc                varchar2(200) := NULL;
    v_error_code                number(19);

    CURSOR ARTIST_TITLE_CURSOR is
            select track_artist,track_title
            from asset_artist_title;

    CURSOR QUERY_CURSOR is
            select distinct g1.gaid,g2.legal_name,g1.artist_display_title,
                            g1.display_title,g1.category,g1.type,g1.sub_type
            from gcdm_app_rpt.rpt_asset g1,
                 gcdm_app_rpt.rpt_artist g2
            where g1.artist_id = g2.id
            and   g1.is_deleted <> 'Y'
            and   g1.is_core = 'Y'
            and   g2.is_core = 'Y'
            and   g1.title like v_title||'%'
            and   g1.artist_display_title like v_artist||'%';

BEGIN
    OPEN ARTIST_TITLE_CURSOR;

    LOOP
    FETCH ARTIST_TITLE_CURSOR into v_artist,v_title;
    EXIT WHEN ARTIST_TITLE_CURSOR%NOTFOUND or ARTIST_TITLE_CURSOR%NOTFOUND IS NULL;

    SELECT count(*)
    INTO   v_row_count
    FROM   gcdm_app_rpt.rpt_asset g1,
           gcdm_app_rpt.rpt_artist g2
    WHERE  g1.artist_id = g2.id
    AND    g1.is_core = 'Y'
    AND    g1.is_deleted <> 'Y'
    AND    g2.is_core = 'Y'
    AND    g1.title like v_title||'%'
    AND    g1.artist_display_title like v_artist||'%';


    IF v_row_count < 1 THEN
         v_error_desc := 'Matching Asset record for '||v_artist||' - '||v_title||' not found';
        DBMS_OUTPUT.PUT_LINE('Error: '||v_error_desc||'.');

         v_row_count := 0;
         v_total_rows_error := v_total_rows_error + 1;

    ELSE
        OPEN QUERY_CURSOR
        FOR i in 1..ARTIST_TITLE_CURSOR
        LOOP
        FETCH QUERY_CURSOR into TITLE_RECORD;
        EXIT WHEN QUERY_CURSOR%NOTFOUND or QUERY_CURSOR%NOTFOUND IS NULL;

        DBMS_OUTPUT.PUT_LINE(title_record.id,title_record.gaid,title_record.artist_legal_name,title_record.artist_display_name,
                             title_record.display_title,title_record.category,title_record.type,title_record.sub_type);

        END LOOP;
        CLOSE QUERY_CURSOR;

        v_row_count := 0;

    END IF;

    END LOOP;

    CLOSE ARTIST_TITLE_CURSOR;


    DBMS_OUTPUT.PUT_LINE(chr(0));

    IF v_total_rows_error > 0 THEN
        DBMS_OUTPUT.PUT_LINE('Total Rows in error: '||v_total_rows_error);

    END IF;

    DBMS_OUTPUT.PUT_LINE(CHR(0)); 

EXCEPTION
    WHEN OTHERS THEN
        v_error_desc := SQLERRM;
        v_error_code := SQLCODE;

    DBMS_OUTPUT.PUT_LINE('Error: '||v_error_desc||' - '||v_error_code);    

END;
peter.hrasko.sk
  • 4,043
  • 2
  • 19
  • 34
harrysolomon
  • 7
  • 1
  • 3
  • 1
    What is `FOR i in 1..ARTIST_TITLE_CURSOR` supposed to be doing? – Alex Poole Nov 11 '15 at 19:53
  • I originally had this coded as **FOR i in 1..v_row_count** but changed it prior to posting this based on some code examples I saw online. I have other working PLSQL scripts which are successfully using **FOR i in 1..v_row_count**.
    Basically what is supposed to happen is, the ARTIST_TITLE_CURSOR returns a Song Title and the name of the performer.
    QUERY_CURSOR is then supposed to take that Song Title and Performer name and check the database for additional records which the same Artist and Title.
    – harrysolomon Nov 11 '15 at 19:59
  • Sorry for the formatting of this answer. It's my first time on Stackoverflow and I'm still trying to get familiar with the formatting tags. – harrysolomon Nov 11 '15 at 20:09
  • OK, but why would you run that second cursor more than once, based on the row count? Seems like you're mixing up two approaches, perhaps. Anyway, you'd still have an error with that for the same reason, because of the missing semi-colon... – Alex Poole Nov 11 '15 at 20:11
  • Re. formatting, there's a lot of useful info in [the help center](http://stackoverflow.com/help), includiing formatting tips. – Alex Poole Nov 11 '15 at 20:17
  • The ARTIST_TITLE_CURSOR should generally be returning more than a single record for the given Title and Artist (v_row_count > 1). So QUERY_CURSOR then runs and pulls the relevant information for all the matching records. – harrysolomon Nov 11 '15 at 20:23
  • But you will get that many rows from QUERY_CURSOR anyway, since the cursor query is the same as the count query. All you are potentially doing is fetching exactly that many times, so you don't quite hit the (oddly formed) `exit when ... notfound` - you don't do the next loop iteration that would hit that. (I wouldn't have a separate count as it's repetitive; I'd increment a counter within the cursor loop, and afterwards check if it was non-zero.) – Alex Poole Nov 12 '15 at 09:30
  • I see what you're saying and I'm definitely going to give it try. I'm obviously no expert at this but always willing to learn. Thanks for the tip. – harrysolomon Nov 12 '15 at 14:26

1 Answers1

3

It's line 67 in what you've posted, not 61, but still; this line is not right:

FOR i in 1..ARTIST_TITLE_CURSOR

You're trying to loop over a range of numbers - perhaps you wanted the number of records returned by the cursor, which you can't get - but your end 'number' is a cursor, so not legal in that context.

But it seems to be completely out of place anyway as you're looping over the QUERY_CURSOR records, so I wouldn't think the ARTIST_TITLE_CURSOR is relevant at this point. And you aren't attempting to use i. It looks like you can just remove that line.

More importantly, the previous line is missing a semi-colon:

OPEN QUERY_CURSOR;

Because it doesn't have one it's seeing the FOR and expecting a cursor query.


Following up on comments about why you have that FOR 1..v_row_count, it's still a bit redundant. You're limiting the number of fetches you do to match the count you got previously, from essentially the same query as you have in the cursor, which means you don't quite ever hit the EXIT WHEN QUERYCURSOR%NOTFOUND condition - that would come from the v_row_count+1 loop iteration. Normally you wouldn't know how many rows you expect to see before you loop over a cursor.

You don't really need to know here. The count query is repetitive - you're querying the same data you then have to hit again for the cursor, and you have to maintain the query logic in two places. It would be simpler to forget the count step, and instead keep a counter as you loop over the cursor; then handle the zero-rows condition after the loop. For example:

DECLARE
...
BEGIN
  OPEN ARTIST_TITLE_CURSOR;
  LOOP
    FETCH ARTIST_TITLE_CURSOR into v_artist,v_title;
    EXIT WHEN ARTIST_TITLE_CURSOR%NOTFOUND;

    -- initialise counter for each ARTIST_TITLE
    v_row_count := 0;

    OPEN QUERY_CURSOR;
    LOOP
      FETCH QUERY_CURSOR into TITLE_RECORD;
      EXIT WHEN QUERY_CURSOR%NOTFOUND;

      -- increment 'found' counter
      v_row_count := v_row_count + 1; 

      DBMS_OUTPUT.PUT_LINE(title_record.id
        ||','|| title_record.gaid
        ||','|| title_record.artist_legal_name
        ||','|| title_record
        ||','|| artist_display_name
        ||','|| title_record.display_title
        ||','|| title_record.category
        ||','|| title_record.type
        ||','|| title_record.sub_type);

    END LOOP;
    CLOSE QUERY_CURSOR;

    -- now check if we found anything in the QUERY_CURSOR loop
    IF v_row_count < 1 THEN
      v_error_desc := 'Matching Asset record for '||v_artist||' - '||v_title||' not found';
      DBMS_OUTPUT.PUT_LINE('Error: Matching Asset record for '
        || v_artist || ' - ' || v_title || ' not found.');

      v_total_rows_error := v_total_rows_error + 1;
    END IF;
  END LOOP;
  CLOSE ARTIST_TITLE_CURSOR;

  --DBMS_OUTPUT.PUT_LINE(chr(0));
  -- presumably this was meant to put out a blank line; use this instead
  DBMS_OUTPUT.NEW_LINE;

  IF v_total_rows_error > 0 THEN
    DBMS_OUTPUT.PUT_LINE('Total Rows in error: '||v_total_rows_error);
  END IF;

  --DBMS_OUTPUT.PUT_LINE(CHR(0)); 
  DBMS_OUTPUT.NEW_LINE;
END;

I've also taken out the exception handler because it isn't really adding anything; you'd see the code and message without it, even if you didn't have server output on; and catching WHEN OTHERS is a bad habit to get into.

You also don't need to declare your record type. You could use an implicit cursor anyway and avoid the type and variable completely, but even with the cursor definition you have, you could put this afterwards instead:

TITLE_RECORD  QUERY_CURSOR%ROWTYPE;

There are various ways to open and loop over cursors, and you're using one of the more explicit ones - which isn't a bad thing for learning about them, but be aware of the options too.

Alex Poole
  • 183,384
  • 11
  • 179
  • 318
  • Yep I think the missing semicolon after QUERY_CURSOR is what was causing the problem. Added that and I'm not getting the **PLS-00103** error message any more. Still getting other errors but I think I know how to fix them. Thanks so much for your help. – harrysolomon Nov 11 '15 at 20:14
  • This definitely did fix the issue and the code is running as expected now. Thanks again for your help. – harrysolomon Nov 11 '15 at 21:07
  • Thanks for this greatly improved code, very interesting. I knew there was probably a better more efficient way to handle this task. – harrysolomon Nov 12 '15 at 14:28