Project

General

Profile

Bug #10360

FOR loop WHERE condition evaluation issue

Added by Teodor Gorghe 12 months ago. Updated 11 months ago.

Status:
Test
Priority:
Normal
Assignee:
Teodor Gorghe
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
Alexandru Lungu, Constantin Asofiei
production:
No
env_name:
topics:

Related issues

Related to Database - Bug #9133: Array subscript is out of range stops the evaluation of other expressions in the where clause Internal Test

History

#2 Updated by Teodor Gorghe 12 months ago

We have the following test case:

First two tests should not throw any error and the last test should show the following error message: ** Incompatible data types in expression or assignment. (223).

The following error, which makes all three tests to fail on FWD is:

Caused by: java.lang.IllegalArgumentException: Argument type class com.goldencode.testcases.tests.TestForLoopUnknown$$Lambda$831/0x00007f934498ec78 is not known.

#3 Updated by Constantin Asofiei 12 months ago

This one is weird, not in terms that it fails, but how 4GL behaves. In FWD, we evaluate all arguments before executing the query; in 4GL, they are evaluated 'for each record', when filtering via the scan index. And things get even weirder if an index is added:

DEFINE TEMP-TABLE tt1 FIELD f1 AS INTEGER.
DEFINE TEMP-TABLE tt2 FIELD f1 AS INTEGER FIELD f2 AS INTEGER index ix2 f2.
DEFINE VARIABLE hdl AS HANDLE NO-UNDO.

CREATE tt2.
tt2.f1 = 1.
tt2.f2 = 2.
release tt2.
CREATE tt2.
tt2.f1 = 1.
tt2.f2 = 2.
release tt2.

FOR EACH tt2 WHERE tt2.f1 = 1 and tt2.f2 = hdl::invalid:  // no error is thrown here...
   message tt2.f2. 
END.

What exact case is the customer application's using?

An approach would be to convert the WHERE clause to a client-side where, but I don't recall exactly the rules (I think is a 'all or nothing approach', which means the FOR EACH gets all records and filters them on the FWD server - the 'db client side'). So we can't just convert every case where the expression in a WHERE clause has a handle reference, as this will affect performance.

Ovidiu: do you recall some example where we emit a client-side WHERE in FWD, in conversion?

#4 Updated by Constantin Asofiei 12 months ago

Teodor, please also add a test where hdl::f3 is a valid field and hdl is a valid handle.

#5 Updated by Teodor Gorghe 12 months ago

I have updated the Test code from the #10360-2.

#6 Updated by Constantin Asofiei 12 months ago

For now, lets fix it so that FWD doesn't abend - I assume the resolver/lambda? for this argument is not properly executed when preparing the arguments.

#7 Updated by Ovidiu Maxiniuc 12 months ago

Constantin Asofiei wrote:

Ovidiu: do you recall some example where we emit a client-side WHERE in FWD, in conversion?

First: your testcase converts successfully for me and also runs without producing any output. I did not check the data in tables, but it seems that it does not matter. The converted code is:

            query0.initialize(tt2, "tt2.f1 = 1 and tt2.f2 = ?", null, "tt2.f2 asc", new Object[]
            {
               (P2JQuery.Parameter) () -> new integer(hdl.unwrapDereferenceable().dereference("invalid"))
            });
The easiest way to force the conversion to create a client-side component is to inject a can-find from a different database. Your example is based on _temp so I can use the permanent table book as:
FOR EACH tt2 WHERE tt2.f1 = 1 AND 
      CAN-FIND(book WHERE book.isbn = hdl::invalid):  // no error is thrown here...
   MESSAGE tt2.f2. 
END.
Now the code we obtain is:
            query0.initialize(tt2, "tt2.f1 = 1", () -> new FindQuery(book, "upper(book.isbn) = ?", null, "book.bookId asc", new Object[]
            {
               toUpperCase(hdl.unwrapDereferenceable().dereference("invalid"))
            }, LockType.NONE).hasOne(), "tt2.f2 asc");
Hope this helps.

#8 Updated by Teodor Gorghe 12 months ago

Constantin Asofiei wrote:

For now, lets fix it so that FWD doesn't abend - I assume the resolver/lambda? for this argument is not properly executed when preparing the arguments.

Committed revision 16084 on task branch 10360:
- QueryComponent.resolveArgs: unprocessed lambda/resolver are replaced with unknown value.

#9 Updated by Teodor Gorghe 12 months ago

  • Status changed from New to WIP
  • Assignee set to Teodor Gorghe

#10 Updated by Teodor Gorghe 12 months ago

  • Status changed from WIP to Review
  • % Done changed from 0 to 100
  • reviewer Constantin Asofiei added

#11 Updated by Alexandru Lungu 12 months ago

  • Related to Bug #9133: Array subscript is out of range stops the evaluation of other expressions in the where clause added

#13 Updated by Alexandru Lungu 12 months ago

IMHO, there are multiple issues here:
  • if no records actually "hit" the faulty clause, no error is thrown.
    • if the table is empty, there are really no errors thrown when parsing the WHERE clause. This is quite new to me.
    • if the first clause tt2.f1 = 1 doesn't hold, then AND won't evaluate the second parameter, so the faulty clause won't be reached. In your unit tests, if you create elements that do not have f1 equal to 1, then there won't be any error. (#9133). We do not fixed this as it presumed moving the whole clause as a client-side where. We have only logged that!
  • even if the clause is hit, there are some cases in which a faulty operand is evaluated silently to unknown. Please check #7982-23.
    • if the query is dynamic, relational and equality operands can have different types! For instance, character types will be cast implicitly to integer if = (which is not correct in static cases).
    • if you use dereference operator, then the query will be "partially dynamic". The WHERE clause won't have its correctness ensured at compile time, so you may end up with some dynamic things going on.
FOR EACH tt1 WHERE tt1.f1 = hdl:scrollable:
    message "found".
END.

This test will fail at compile time if f1 is not logical, because scrollable is a logical attribute. However, if f1 is logical, then hdl:scrollable won't throw invalid handle error, but it will match the records that have f1 set on unknown.

def var x as int extent 5.
def var y as int initial 10.

FOR EACH tt1 WHERE tt1.f1 = x[y]:
    message "found".
END.

This will also match only tt1.f1 that is ?.

In FWD, we evaluate all arguments before executing the query; in 4GL, they are evaluated 'for each record', when filtering via the scan index.

This is mostly correct from FWD's POV. Arguments are not affected by run-time changes in 4GL. If you alter hdl inside the FOR loop, then the query won't change.
This means that the arguments are evaluated only once at the beginning based on the state before the first loop iteration. All other rows are using that arguments.

PS: Please also test FOR EACH tt1 WHERE tt1.f1 = 1 AND hdl::invalid-handle. I think that hdl::invalid-handle will evaluate to ? and the AND conjunction will end up being ? or FALSE - either way nothing should be found.

#14 Updated by Teodor Gorghe 12 months ago

On FWD, without the 10360a rev 16084 fix, the test cases always fails when the handle is invalid, including FOR EACH tt1 WHERE tt1.f1 = 1 AND hdl::invalid-handle.
After the fix, all test cases passes now.

The last test case should not pass, as it fails on OE (and that type of error can't be caught into a catch block). On FWD this passes.
I have modified the last test case with this:

    @Test.
    METHOD PUBLIC VOID TestForLoopUnknownHandleDoesThrowWhenReach():
        FOR EACH tt2 WHERE tt2.f1 = 1 AND tt2.f2 = hdl::INVALID-HANDLE:
            Assert:IsTrue(FALSE).
        END.
    END METHOD.

This still fails on OE, but on FWD, with the 10360a rev 16084 fix, tt2.f1 = 1 evaluates to TRUE and tt2.f2 = hdl::INVALID-HANDLE to FALSE. Assert:IsTrue(FALSE) is not executed.

#15 Updated by Alexandru Lungu 12 months ago

  • Project changed from Base Language to Database

#16 Updated by Constantin Asofiei 12 months ago

  • Status changed from Review to WIP

Teodor, the fix is not OK. The root cause is the block-level on error undo, throw. clause in FWD and an additional CATCH - this leaves the evaluation of this lambda, in AbstractQuery.preprocessSubstitutionArguments:

 parm = () -> args[idx] = unwrapProxy.apply(((Parameter) next).resolve());

in an inconsistent state - args[idx] remains set to the original Parameter lambda, and doesn't get a chance to be set even to unknown.

More, this scenario shows that a STOP condition is being raised:

do on stop undo, leave:
   FOR EACH tt2 WHERE tt2.f1 = 1 and tt2.f2 = hdl::invalid:  // no error is thrown here...
      message tt2.f2. 
   END.

   catch ex as progress.lang.error:
      message ex.
   end.
end.

message "finish".

More, the catch block is mandatory to duplicate this. This is a good case to show that we need first to duplicate the problem as simple as possible (in a standalone .p) to understand it, and after that move ti OEUnit.

#17 Updated by Teodor Gorghe 12 months ago

  • Status changed from WIP to Feedback

Ok. I have understood.
The ErrorConditionException thrown by deference operator is eaten by silentWorker, because the force flag is set to false.
I am thinking about having an additional check of wa.legacyError on preprocessSubstitutionArguments and just set args[idx] to unknown:

  • AbstractQuery.java
                   if (!success || errHlp.hasAnyError())
               {
                  errHlp.clearPending();

                  args[idx] = new unknown();
               }
    *
  • ErrorManager.java
          public boolean hasAnyError() {
         return wa.legacyError != null;
      }

Regarding with the STOP condition, I don't have any solution.

#18 Updated by Constantin Asofiei 12 months ago

Hm... why is success true in this case?

#19 Updated by Teodor Gorghe 12 months ago

Success is true in this case because the silentWorker catches the ErrorConditionException, thrown by deference operator with ErrorManager.displayError.

ErrorManager.displayError throws the ErrorConditionException when on error undo, throw is used. This exception is then caught by the silentWorker, which ignores it. The success value is true in this case.

When on error undo, throw is not used, then ErrorManager.displayError just displays the error and marks it as error. The success value is false in this case.

#20 Updated by Teodor Gorghe 12 months ago

  • Status changed from Feedback to WIP
  • reviewer Alexandru Lungu added

Committed revision 16085 on task branch 10360a:
- Reverted 16084. Replaced displayError with recordOrThrowError on handle.invalidAttribute method.

I have created the following test case, which failed before this change, because of ERROR-STATUS:ERROR flag set to false:

I have also tested this on procedure and it seems OK.
I have one question which I've didn't found the answer: Why ErrorManager.displayError is used, since ErrorManager.recordOrThrowError basically does the same thing?

#21 Updated by Teodor Gorghe 12 months ago

  • Status changed from WIP to Review

#22 Updated by Alexandru Lungu 12 months ago

  • Status changed from Review to WIP
  • % Done changed from 100 to 90

Teodor Gorghe wrote:

I have one question which I've didn't found the answer: Why ErrorManager.displayError is used, since ErrorManager.recordOrThrowError basically does the same thing?

There are multiple levels of "error-handling", mostly depending on client mode:
  • batch mode is not interactive, so you can't "see" an error. There is no point is displaying it.
  • ChUI vs GUI mode may differ in error handling, but both are interactive and most probably display a message on screen.
  • redirected mode is when you use OUTPUT TO and so errors may not be displayed on screen, but rather written to a file.
All of the above are only about client mode. Now you also have to consider whether:
  • the error is also throwing a legacy exception (so you can "catch" it)
  • the error sets the ERROR-STATUS flags
  • how the error behaves in a statement with NO-ERROR
  • how the error behaves with block-level on error undo, throw.

I usually think that displayError just displays an alert message and does not update ERROR-STATUS or is able to be "catch". It looks mostly like a notification pop-up :)

I reviewed 10360a and I am not sure the change is right. Test:

def var h as handle.

message h::f1.
message error-status:error. // no

I see the error being displayed, but the execution is not stopped so that no is printed. It tells me that displayError is correct, as there is no throw, record or execution halt. Even with a catch, nothing happens.

#23 Updated by Alexandru Lungu 12 months ago

  • Status changed from WIP to Internal Test

Nevermind. I think the code in #10360 is a super weird edge case.

def var h as handle.
def var a as int initial 0.

a = h::f1.

catch b as Progress.Lang.SysError:
    message a.
    message error-status:error.

This shows that the error is thrown.

def var h as handle.
def var a as int initial 0.

a = h::f1 no-error.
message error-status:error. // yes

This shows that the error is recorded.

I think it is right to replace ErrorManager.displayError with ErrorManager.recordOrThrowError. But please do a bzr blame for the author of the displayError and backtrack to a task where that was motivated. I am concerned that the display seem quite peculiar there like someone had a test-case exactly for it. I don't want to break that.

#24 Updated by Teodor Gorghe 11 months ago

The trunk revision which changes the recordOrThrowError to displayError is 11301.1.51 #3761.
This also modifies the BufferImpl.dereference to use displayError. From this revision until the current trunk revision, the implementation was changed back to use recordOrThrowError.
I have asked the author about the test case and he told to me that he does not remember about a testcase for the handle dereference operator.

I am starting to do internal testing and I will tell if there is a problem.

#25 Updated by Alexandru Lungu 11 months ago

I am starting to do internal testing and I will tell if there is a problem.

I am OK with the plan.

#26 Updated by Constantin Asofiei 11 months ago

  • Status changed from Internal Test to Merge Pending

10360a can be merged now.

#27 Updated by Teodor Gorghe 11 months ago

  • Status changed from Merge Pending to Test
  • % Done changed from 90 to 100

Branch 10360a was merged to trunk rev 16120 and archived.

Also available in: Atom PDF