FWD » Core Development » Database

create test_table.
test_table.f1[1] = 10.
test_table.f1[2] = 20.
test_table.f1[3] = 30.
release test_table.

def buffer buf1 for test_table.
def var i1 as int initial 0.
def var i2 as int initial 2.

find first buf1 where i2 = 2 or buf1.f1[i1] ne 0 no-error.
if available buf1 then do:
   message "found".
end.

in 4gl found is printed.
in fwd there is nothing printed on the screen.

f1[i1] throws error but it is silent and because the other expression (i2 = 2) is true, find first is able to find the record.
In fwd the execution of find first stops when the error is encountered (the same happens for both persistent tables and temp-tables).

This is actually happening in the case of one of our customer, so it needs to be fixed. See #7143-1204.

The (anonymized) generated FQL is: "miTable.sField = false and (? or miTable.dField[?] = true)". The parameters are: isEqual(i, 0) and minus((i), 1). At the moment of execution, i is 0. Even more, the query is a FOR EACH, not even in NO-ERROR mode.

The HQLAst is:

and [AND]
   = [EQUALS]
      miTable [ALIAS]
         sField [PROPERTY]
      false [BOOL_FALSE]
   ( [LPARENS]
      or [OR]                    // <---- the OR to be short-cut
         ? [SUBST]               // <---- the first operand with YES/TRUE value
         = [EQUALS]              // <---- this node and its subtree should NOT be processed
            miTable [ALIAS]
               dField [PROPERTY]
                  [ [LBRACKET]
                     ? [SUBST]
            true [BOOL_TRUE]

Because the first parameter, isEqual(i, 0) is evaluated to true, the second operand of or should not be evaluated (this is why the first operand of or, (i eq 0) is guarding) since the value of the or node is already known. The problem is that FQLPreprocessor is not taking this into consideration and will blindly attempt to process that subtree.

Probably, a similar thing happens for AND(FALSE, T).

Lorian, please put this change in a branch and move this into Review.

Lorian please, do such experiment and post the Java conversion of buf1.f1[i1] vs buf1.f1[buf1.f2]. Are these processed differently as query parameters vs inlined FQL?

Thanks Alex for the investigation. My implementation was too trivial and for sure did not solve all the edge cases. The issue seems more complex than i thought it was. I tested a bit the scenario you mentioned and this is the java converted code:

query0.initialize(buf1, ((String) null), () -> or(isEqual(buf1.getF2(), 0), () -> isNotEqual(buf1.getF1(minus((buf1.getF2()), 1)), 0)), "buf1.recid asc");

• The extent access is extracted onto a client where and the output from FWD is the same as in 4GL.

query1.initialize(buf1, "buf1.f2 = 0 or buf1.f1[?] != 0", null, "buf1.recid asc", new Object[]
            {
               minus((i1), 1)
            });

• The extent access is extracted onto inline FQL and the issues arrise.

Probably, as you said, the solution would be most of the time to extract the extent access into a client where and avoid FQLPreprocessor.
I will look into the rules and find a way of changing the extent access.

I really fear the impact of client where. For the first query you posted: if that query would run over a persistent database, it would basically fetch all records in the memory. This may be huge. Also, the client-where for such extent usage would be unbounded. While previously was only about dynamic indexes, using this now for static indexes would make all extents be evaluated at server-side - yikes!

Also, using throw_error is a bad idea due to https://www.postgresql.org/docs/current/sql-expressions.html#SYNTAX-EXPRESS-EVAL This means that SQL is not short-circuiting OR from left-to-right necessary. So the core principle is flawed.

• Maybe we can implement a system that will transform an FQL to a client where at run-time. This way we generate the slow query due to invalid extent index only when we detect such case.

I think I have the question for why before 9095a the incorrect FQL (containing references of form <tablename>.null) was not a problem.

The response can be found in AdaptiveComponent.prepareHQLPreprocessor(). At that point, there are two FQLPreprocessor constructed, using different parameter sets. The call from super will invoke FQLPreprocessor.get() with true as the 7th parameter (inline), while if fallback and the first FQLPreprocessor was inlined, a new dynamic version is constructed which should NOT permit inlining of arguments.

• if the invalid index is detected, the process will stop with error;
• if not, the invalid column name will be used, meaning a broken FQL string.

In the customer application, the second preprocessor, even if it is created, it is never actually used. This is why it worked. Have it been used, the incorrect FQL would have been converted to an incorrect SQL and the invalid syntax error would have been issued when the latter is executed.

• if the inline parameter is false for the dynamic processor, is it normal to do the 'inlining' for the name of the expanded fields? The dynamic version should NOT take into consideration the current parameters. It is not correct to ignore the value of i2 in left but to use i1 on the right side of OR (in testcase #9133-1);
• why are we so eager to construct the AdaptiveComponent.dynamicFQLPreprocessor if it will NOT be used? The process (assuming it is corrected) is time consuming (the FQL string is parsed, HQLAst tree constructed and walked multiple times).

In the customer application, the second preprocessor, even if it is created, it is never actually used. This is why it worked. Have it been used, the incorrect FQL would have been converted to an incorrect SQL and the invalid syntax error would have been issued when the latter is executed.

This seems like a "it works by chance" case in the customer application. I would rely on it as before, but we should hurry up on fixing it in the near future. This includes the two questions asked - I don't have an answer right now; it needs specialized investigation and testing.

I think this is similar to item 4. udf.throw_error(26) is even better than original null because this is the actual way to thrown the error when needed. The only problem I see here is writing throw_error method to return a _poly value. Even if will not actually return anything, it must honour the SQL syntax and match the expected type.

I think #9133-11 should dismiss this approach. If the official documentation states that it has non-deterministic results, I won't rely on that then. Even with index('tt', 'f1', sqlExpr), I think it falls into the same dilema. Note that this is only for PostgreSQL; SQL standard doesn't guarantee such evaluation order. I don't know more about MariaDB or other dialects. Anyway, this is a solution that is flawed right from the start, so lets abandon it.

You can have more complex expressions like for each tt where true or tt.f1[i0] + 1 = 2. This will result in false = 2 according to your changes (which is a syntax error)

The processing is done with the full expression parsed. If the simplification is applied, the full tt.f1[i0] + 1 = 2 expression is dropped.
the initial solution I proposed for this task, was to identify the left-side of the or as being a constant and evaluate the whole tree to a specific value using simplification rules. For example:

Yeah, I was rather thinking of more complex cases like for each tt where tt.f2 = 1 or tt.f1[i0] + 1 = 2, in which we can't do any reduction. For this case, tt.f1[i0] is still not converting properly to SQL.

On a more general scale, for x or y in SQL, FWD should generate or(<expr1>, () -> <expr2>) in SQL. The problem is that there is no lazy evaluation possibility in SQL (i.e. () -> <expr2>), the OR UDF should guard the evaluation of the second operand and this might be very slow anyway. Using the native SQL OR doesn't guarantee any evaluation order.

I strongly agree here. We will probably be forced to do that, but let's try to limit the cases where the correctness cannot be achieved otherwise. And, ad Eric mentioned in daily meet, these events MUST be reported at conversion time.

1) Do you have some rules in mind. From my POV, only a static correct index can emit FQL in this scenario (i.e. tt.f1[1]). This means we need to infer the data-type of that, check the extent, and ensure it is in bounds. In all other cases, we should emit a client-where. From my POV, all other cases seem like 99% of the cases :) I am afraid of the performance draw-back in the end.

the second approach was to detect the tablename.extField[id] cases and instead of generating invalid column name, just to replace the node with a null (unknown) value.

2) I really like this! It will generate a correct SQL and in most cases it will return an accurate set of results. The flaw is indeed that using an invalid index will not generate an error - it will simply bypass that. Functionally, it is not perfect as it probably returns more results than intended. But, we can do a SEVERE log for that - when id is 0 in tablename.extField[id]?

Solution 1) is functionally perfect, but very slow in 99% of the cases. Solution 2) is fast, but not functionally correct in some edge cases. Any both cases the customer will be recommended to rewrite the code (due to conversion warning or run-time SEVERE log). I think we are left only with these two cases.

I am inclined to go with 2) because none of the edge cases of 2) were found in a customer application. With 1), the performance impact will affect most of the cases in the customer application.

Lorian, please attempt 2) in 9133a. It means replacing invalid tablename.extField[id] with ? and that is all. Put it in the branch and more to Review.

Alexandru Lungu wrote:

Lorian, please attempt 2) in 9133a. It means replacing invalid tablename.extField[id] with ? and that is all. Put it in the branch and more to Review.

I replaced tablename.extField[id] with null since i was not able to find UNKNOWN type for nodes:

Original where clause : buf1.f2 = 0 and buf1.f1[?] != 0
Generated fql : buf1.f2 = 0 and (null != 0 or null is null) (null is null is an augmentation that is added before inlineDenormalizedField() is called)
Generated SQL :

select 
    testtable_0_.recid as col0_0_ 
from
    test_table testtable_0_ 
where
    testtable_0_.f2 = 0 and (0 is not null or null is null)
order by
    testtable_0_.recid asc

I'll try to find if there is a way to skip adding that null is null condition, because that will always be true and therefore the whole expression in parenthesis will be evaluated as true which is not correct.

I looked into FQLPreprocessor.mainWalk() and found out how we handle the cases tablename.extField[?] , where the index has unknown value.

// Possibly inline certain query substitution parameters.
            case SUBST:
               parent = (HQLAst) next.getParent();
               if (parent != null)
               {
                  int index = ((Long) next.getAnnotation("index")).intValue();
                  Object param = parameters[index];

                  [....]
                  // check for unknown value
                  if (param instanceof BaseDataType && 

I tried to force the cases where we have an index < 0 to behave similar as it had unknown value.
The problem is that these type of cases are resolved like this : they are removed from the fql at all they are simplified (simplifyUnknowns())

 // get rid of unknowns
         if (unknowns != null)
         {
            Set<HQLAst> createdBooleans = simplifyUnknowns(unknowns);
            if (createdBooleans != null)
            {
               if (needSimplification == null)
               {
                  needSimplification = new HashSet<>();
               }
               needSimplification.addAll(createdBooleans);
            }
         }

I will investigate deeper to see if this case is simplified as BOOL_FALSE

Lorian Sandu wrote:

I will investigate deeper to see if this case is simplified as BOOL_FALSE

Actually, this is simplified as BOOL_TRUE and therefore will be removed from the fql.
Our case is something like this ? != 0 :

 // we have a simple "? <comparison operator> {CONST|SUBST}" statement
               if (pType == EQUALS)
               {
                  disposition = BOOL_FALSE;
               }
               else if (pType == NOT_EQ)
               {
---->             disposition = BOOL_TRUE;
               }

Hmm I see. This is because it will end up as null ne 0 and this is basically true. This you also happen for cases where we compare the extent field with null (tablename.extField[id] = ?) - it will evaluate to true.
This gets trickier. I suppose this is in fact an edge case of the functionality. So I think we are back on the drawing board with #9133-14 (?).

Alexandru Lungu wrote:

Hmm I see. This is because it will end up as null ne 0 and this is basically true. This you also happen for cases where we compare the extent field with null (tablename.extField[id] = ?) - it will evaluate to true.
This gets trickier. I suppose this is in fact an edge case of the functionality. So I think we are back on the drawing board with #9133-14 (?).

• replace invalid tablename.extField[id] with ? in mainWalk() because the expression will be evaluated and simplified as BOOL_TRUE (which is incorrect, #9133-17)
• change tablename.extField[id] to be a null node in the AST (in inlineDenormalizedField()) because the fql will also be incorrect (#9133-15)

So this means that converting to client where is the only way to go here? I suppose so.

I put some thinking into this again, but (again) I can't see a clear implementation for this. Can we have a log SEVERE for this case in trunk until a proper solution is found?
If the extent parameter is out-of-bounds, log that severe message. This way, we can troubleshoot the issue better when we will encounter it.

Because the extents are expanded, this makes it even harder to support.

Short term: I would document this as an unsupported functionality of expanded extents. FWD will simply preprocess the FQL and will fail if a bad extent is used, even if in an OR clause. It is like attempting to generate an invalid SQL, but rely that "the bad part of the SQL will not get reached due to OR short-circuits". This is simply not possible due to the strong typing of SQL and the non-deterministic order of processing the OR clauses.

Long term: I suppose we can convert the FQL to a Java client where at run-time and interpret it. This way, we don't generate the client where for all queries with extents at conversion time. However, this will be a new functionality of FWD: converting only an FQL to a Java expression (client-side where) and interpreted dynamically.

       if (index == null || index < 0)
       {
          LOG.severe(String.format("Failed to denormalize %s.%s[%d]. Index is out of bounds or is null.",
-                 recBuf.getLegacyName(),
-                 property,
-                 index));
+                                  recBuf.getLegacyName(),
+                                  getLegacyFieldName(recBuf, property),
+                                  index != null ? index + 1 : null));
          return null;
       }
       DmoMeta dmoInfo = recBuf.getDmoInfo();

• changed the field name that is logged to match exactly the name in the original 4GL code
• incremented the logged index to match the indexing value from 4GL

If the 4GL code looks like buf1.f1_ext[0] , the FWD log is :
| SEVERE | com.goldencode.p2j.persist.TableMapper | .... | Failed to denormalize buf1.f1_ext[0]. Index is out of bounds or is null..

It would be then easier to find the problematic 4GL code when debugging if we know how it looks like from the log.

Serban, can you test the scenario from #9189-1 with 9133b , please?

I expect to see some SEVERE logs of this format :
| SEVERE | com.goldencode.p2j.persist.TableMapper | .... | Failed to denormalize buf1.f1_ext[0]. Index is out of bounds or is null..