FWD » Core Development » Base Language

The third bullet point is a regression added in trunk/15456, recordOrShowError throws a DeferredLegacyErrorException which results in the com.goldencode.p2j.NumberedException: ** null error. Added Artur as a watcher as this is related to #8912 changes (8912h branch).

Committed 9396a/15583. This commit fixes all of my testcases related to #9396-2 table (except the 3rd bullet point).

• Added a new DynamicQueryException class.
• Stop dynamic query processing when 223 error is thrown (this will make the returned query null) so code like silent(() -> lok.assign(hq.unwrapQuery().prepare("for each tt9377 where l1 = 10"))); will correctly assign lok value.
• Support error 223 for cast functions (currently only DATETIME and DATETIME-TZ).

• Expand test cases that do not rely on DATETIME and DATETIME-TZ functions and fix any issues.
• Expand test cases to use INT64, DECIMAL, DATE, LOGICAL functions to reveal more incompatibilities and fix those as well.

This turns out to be more complicated for certain cast functions, for example you can have DATETIME(DATE(3,3,2001), 402) which will result in 4 literals to be extracted, then it is possible to have an invalid date format.

Found the following test:

    @Test.
    method public void test-logical-logical-func16():
        define query q for tt9377.
        open query q for each tt9377 where l1 = logical(1).

        finally:
            close query q.
        end finally.
    end method.

    [javac] /home/ddf/gcd/dataset/src/com/goldencode/dataset/unittests/Test9377Set13.java:464: error: no suitable constructor found for logical(int)
    [javac]             (P2JQuery.Parameter) () -> new logical(1)
    [javac]                                        ^
    [javac]     constructor logical.logical(BaseDataType) is not applicable
    [javac]       (argument mismatch; int cannot be converted to BaseDataType)
    [javac]     constructor logical.logical(boolean) is not applicable
    [javac]       (argument mismatch; int cannot be converted to boolean)
    [javac]     constructor logical.logical(logical) is not applicable
    [javac]       (argument mismatch; int cannot be converted to logical)
    [javac]     constructor logical.logical(String) is not applicable
    [javac]       (argument mismatch; int cannot be converted to String)
    [javac]     constructor logical.logical(character) is not applicable
    [javac]       (argument mismatch; int cannot be converted to character)
    [javac]     constructor logical.logical(Boolean) is not applicable
    [javac]       (argument mismatch; int cannot be converted to Boolean)

The converted query looks like this:

         query16.assign(new AdaptiveQuery().initialize(tt9377, "tt9377.l1 = ?", null, "tt9377.recid asc", new Object[]
         {
            (P2JQuery.Parameter) () -> new logical(1)
         }));

 (P2JQuery.Parameter) () -> new logical(decimal.fromLiteral("2.2")) 

The solution is to either add a logical(int) constructor or make conversion changes similar to the decimal (I prefer no conversion changes).

Review of 9396a

• The logical constructor can be condensed: assign(value != 0). I agree that the constructor should be added.
• I agree with the idea of a DynamicQueryException.

Before providing a green light, please better explain:

where the injected variable is actually a CHARACTER but the conversion should fail because of the datetime function. One possible solution would be to check if the parent is a cast function and then compare it with the sibling type, determining if the types are compatible.

At dynamic conversion, the literals are extracted as variables and embedded into the FQL. This way, the "convertor" will replace the variables (ex-literals) with substitution parameters. injectVariableHelper is doing this work - replacing literals with variables - but for doing so it needs to infer the data type of the variable. Instead of honoring the data type of the literal, it uses the data type of the field that is checked for equality - which in this case has a mismatch. Your attempt is to also honor the CASTs that are used when comparing that literal with the table field.

So for each tt where f1 = 1 is transformed into def var x as int. x = 1. for each tt where f1 = x. But if f1 was logical, the process would fail at injection time.

My questions:

• Is the literal extraction happening only in f1 = 1 kind is situations? Or f1 + 1 = f2 is also a valid case in which 1 is extracted?
• If only f1 = 1 is valid, where are the casts coming from? Or maybe f1 = LOGICAL(1) also a valid case in which the extraction is made? If so, can you point out the code in trunk that already bypasses the CASTS when considering a literal for extraction?

Is the literal extraction happening only in f1 = 1 kind is situations? Or f1 + 1 = f2 is also a valid case in which 1 is extracted?

What I meant by this is if the data type checking is done only for very specific patterns like a = b or FWD has a full implementation for any arbitrary expression (e.g. TRUNCATE(LENGTH(CONCAT("a", "b") - 5, 2) = c). Note how literals are character, but the whole left-side expression is an integer after the evaluation of functions (which are not casts). The bottom-line is if you attempt to fix this for a niche (casts) or for a full-set of problems I am not seeing.

The example will extract "a" and "b" as character variables, replacing the literals.

I might have not expressed myself correctly, I am not talking about implicit casts but explicit ones (using functions like INT64, LOGICAL, DATETIME, DATE, DATETIME-TZ, DECIMAL).

I am not sure I follow. The cast seems to be implicit in 4GL, so it is converted to an explicit one in FWD using extractParams. Therefore, casts seem to be fabricated by FWD. The bottom line of this statement is that maybe CASTS shouldn't be handled specifically for injectVariable, but rather fabricated with more care.

• If LENGTH case works because it comes from the converted code, it means that a cast should work exactly the same, without extra special handling. Thus the problem may reside in the synthetic casts from extractParams.
• If LENGTH case doesn't work, it means this is a more general problem in which literals are incorrectly extracted. In this case, I would rather avoid extracting the literal altogether or find a unified solution to fix them all.

Alexandru, you are right. The problem also happens with other functions, the example:

silent(() -> lok.assign(hq.unwrapQuery().prepare("for each tt9377 where c1 = LENGTH('abc')")));

I'll have to work on another solution, I expect a lot of changes from this...

The results are based on the assumption that in DynamicConversionHelper.injectVariable(), in the second step where the parent is checked that it will try to check the sibling when no construct is available. When a construct is used, then it will not check the sibling (as the parent should be an operator). The last table row is represented by cases where the sibling is checked but it is either null or the conditions are not fulfilled.

Can you check what the construct is in that case? I expect to see UPPER) kind of combination in 99% of that cases.

Alexandru Lungu wrote:

Can you check what the construct is in that case? I expect to see UPPER) kind of combination in 99% of that cases.

• 528 - 4359 times (ProgressParserTokenTypes.KW_AND)
• 152 - 62 times (ProgressParserTokenTypes.EXPRESSION)
• 544 - 3 times (ProgressParserTokenTypes.KW_BEGINS)
• 377 - 2 times (ProgressParserTokenTypes.FUNC_POLY)

528 - 4359 times (ProgressParserTokenTypes.KW_AND)

I wonder if this is something line tt.f1 = 1 and true kind of construct ... doesn't make much sense to extract anyway. I think it is safe to go ahead with the plan of omitting the extraction for such literals.

Alexandru Lungu wrote:

528 - 4359 times (ProgressParserTokenTypes.KW_AND)

I wonder if this is something line tt.f1 = 1 and true kind of construct ... doesn't make much sense to extract anyway. I think it is safe to go ahead with the plan of omitting the extraction for such literals.

Committed the changes to 9396a/15587, the test plan remains #9396-8.

Another commit will follow to remove unnecessary changes from previous commits that are not hit anymore.

Alexandru Lungu wrote:

Ovidiu, can you provide an insight for this matter?

First, a bit of wisdom :) from my experience working with dynamic queries: when dealing with issues in dynamic queries, run the static variant of the query firstly. It will help you see more easily how the query looks under-the-hood. (Of course, it can be done directly from DynamicQueryHelper, but debug code must be activated.)

For a long time, FWD was developed with the following paradigm: the customer MUST use valid 4GL code for conversion. This seems logical, because the source code is assumed to have already been used on a 4GL native box. Otherwise: garbage in - garbage out. As result, a lot of validations and data type checks were ignored during conversion assuming the 4GL compiler have already identified these issues and the programmers resolved them.

However, things got complicated with the introduction of dynamic support for temp-tables, find procedures and queries. We were grad when we took the static conversion and reuse it directly, but as noted above, it lacks the validations and data type checks which the 4GL compiler does in both cases (static and dynamic). The FWD runtime (for both compiled static and interpreted dynamic) is actually unchanged and the type from this PoV is undifferentiable. It assumes the code was semantically checked and executes it identically in both cases.

The problem is not only with the literals. We can add a lot of heuristic filters for them but we will not cover all the cases. There are constructs which do not involve the presence of literals. Take the following example:

DEFINE TEMP-TABLE tt003 
   FIELD l1 AS LOGICAL
   FIELD c1 AS CHARACTER
   FIELD c2 AS CHARACTER.
[...]
h:QUERY-PREPARE("for each tt003 where c1 = length(c2)") // dynamic query
[...]
FOR EACH tt003 WHERE c1 = LENGTH(c2):   // static query, non compilable in 4GL, but convertible by FWD
    [...]
END.

My conclusion is that we need to add the full type checking during standard conversion with error reporting if we want to have a fully compatible runtime for dynamic queries (and not only).

• static conversion will generate tt.f1 = 1 where clause.
• dynamic conversion will generate tt.f1 = ? where clause and a fabricated x will be used as substitution parameter.

• as DynamicQueryHelper has this literal extraction routine which already does the data type checking and little effort can be done to set this straight, lets go with such quick win.
• for general case as you mentioned, a real type checking system is required. Unfortunately, some things can go as POLY and we can't perfectly state if the query is OK or not until the execution time. Anyway, for this case, maybe we can catch an invalid syntax from SQL and presume that the query was simply prepared in a bad way. Of course, we can LOG SEVERE and turn up all the red lights, but as a safety net, we can at least provide a minimal syntax checking through the SQL parser.
  • In a near future, we will need that type checking system.

Review of 8396a

• While the approach should fix the evident problem, I think it is too relaxed. Mind that 1 + 1 = 2 is also a valid syntax, but the two 1 values will be considered complex, because their parent is +. This is not wrong per-se, but please mind injectVariable implmenetation:

      int var4glType = literalType;
      switch (litAst.getParent().getType())
      {
         case ProgressParserTokenTypes.KW_EQ:
         case ProgressParserTokenTypes.EQUALS:
         case ProgressParserTokenTypes.KW_NE:
         case ProgressParserTokenTypes.NOT_EQ:
         case ProgressParserTokenTypes.KW_LT:
         case ProgressParserTokenTypes.LT:
         case ProgressParserTokenTypes.KW_LTE:
         case ProgressParserTokenTypes.LTE:
         case ProgressParserTokenTypes.KW_GT:
         case ProgressParserTokenTypes.GT:
         case ProgressParserTokenTypes.KW_GTE:
         case ProgressParserTokenTypes.GTE:
            ProgressAst sibling = getSibling(litAst);
            if (sibling != null)
            {
               if (sibling.getType() > ProgressParserTokenTypes.BEGIN_FIELDTYPES &&
                   sibling.getType() < ProgressParserTokenTypes.END_FIELDTYPES)
               {
                  // only fields are taken into consideration to avoid database CASTing issues
                  var4glType = sibling.getType();
               }
               else if (sibling.getType() == ProgressParserTokenTypes.FUNC_RECID)
               {
                  // we want our operand to be bigint to match the record.id field in db
                  var4glType = ProgressParserTokenTypes.NUM_LITERAL;
               }
            }
            break;
      }

• it is recid, so the variable type will be num.
• it is compared with a field type (aka tt.f1 kind of syntax).

In all other cases, literalType and var4glType will match. So I suggest having this switch-case from injectVariable fine tuned.

Please investigate why tt.f1 = LENGTH('abc') is hitting the switch-case, making var4glType different from literalType.

Alexandru Lungu wrote:

Ovidiu, the work-flow of static and dynamic queries in FWD differs a bit, especially through DynamicQueryHelper. for each tt where tt.f1 = 1 has different Java outputs in static conversion vs dynamic conversion.

• static conversion will generate tt.f1 = 1 where clause.
• dynamic conversion will generate tt.f1 = ? where clause and a fabricated x will be used as substitution parameter.

• stop the static conversion (or at least log a serious warning) or
• be caught by dynamic query helper and raise a runtime error condition and return false.

The _poly case, by its nature, is resolved at runtime in 4GL as well. We will do this likewise.

This is the ideal/general way to fix the problem. As a temporary short-cut we can check the compatibility of operand types in a quick pass for a set of operators. In my testcase, for = operator, since type(c1) differs from type(length(...)) error 223 should be raised, regardless of the parameters of length. In fact, this is just a fragment of the final solution, where the types of functions/methods/object methods will have to be checked based on their signature. Yet, IIRC, I think we are doing this at some point during conversion, but probably the algorithm is not covering all the cases.

My conclusion is that we need to add the full type checking during standard conversion with error reporting if we want to have a fully compatible runtime for dynamic queries (and not only).

We will indeed be doing this, but not in this task. See #3882 which is itself dependent upon other rework like #1757.

Alexandru Lungu wrote:

Please investigate why tt.f1 = LENGTH('abc') is hitting the switch-case, making var4glType different from literalType.

• 9396a/15582: Reaches DynamicConversionHelper.injectVariable(), in step 2 the var4glType is assigned a STRING and the switch case is not hit.
• 9396a/15588: Reaches DynamicConversionHelper.isComplexVariableInjection(), the ast.getType() is STRING and does not change.

Danut, I am trying to drag the discussion to the point where proving that the error occurs only when var4glType and literalType are different. You stated in #9396-13 that the example with LENGTH should fail - this means that var4glType and literalType are different, right?

Alexandru Lungu wrote:

Danut, I am trying to drag the discussion to the point where proving that the error occurs only when var4glType and literalType are different. You stated in #9396-13 that the example with LENGTH should fail - this means that var4glType and literalType are different, right?

var4glType and literalType should be different indeed.

I thought about this and found that we have a map storing function definitions in SignatureHelper (map_func), this can get the return type needed to check against the other side of the operation. But a better idea is to check this in DynamicQueryHelper.extractParams() when iterating the literals by taking a look at the parent and checking if it's a function available in the map_func.