FWD » Core Development » Database

Dynamically defined queries are parsed and converted at runtime by P2J, then compiled into discrete Java classes using the InMemoryCompiler. Buffers are set into public instance variables of such a newly compiled class, and in the end, an instance of P2JQuery is extracted from the class via an accessor method. This instance is the P2J representation of the dynamically defined query, which is then used to access database records.

This approach has been a great first step in our dynamic database support and while it appears to work consistently, it unfortunately has some problems. Firstly, its performance is very poor. In code which makes heavy use of dynamically defined queries, the compilation step in particular makes the performance of this solution unacceptable. Profiling (sampling) has shown that most of the CPU time spent during customer testing of such code is spent in InMemoryCompiler.compile and InMemoryFileManager.list (really, in the J2SE classes used by these methods).

Secondly, the implementation of the J2SE compiler which is used under the covers (via javax.tools) causes quite a serious memory leak through its heavy use of soft references and an ever-growing, shared array of characters. This problem can quickly overwhelm the P2J server using default garbage collection settings. To alleviate this leak, we effectively have disabled soft references as a stop-gap measure, but this cannot relied upon as a long term approach for production use.

Thirdly, even with soft references disabled, we still have a huge number of dynamically compiled classes filling up PermGen memory. While each is relatively small, the number of these grows continually into the hundreds of thousands, since the same queries are re-converted and compiled into new classes all the time. This is not a sustainable approach for a long-running server.

This task is about replacing that runtime compilation step with a more performant approach which does not generate any new Java classes at runtime, but which instead analyzes the ASTs generated by runtime conversion, and instantiates queries and supporting objects from standard classes.

I understand the principle of this approach.

At first sight, it looks rather easy, the where clause and sorting are already computed as strings. The reflection should be able to detect the appropriate constructor for FindQuery using the arguments stored in an Object[] array.

The difficulty appears when there are another buffers (and aliases) and even more, when some client-side where clause has to be constructed also. We should do some tests to detect the maximum complexity of these and find a solution to instantiate this object.

The java/TRLP debate. It's clear that TRPL has the some advantages, but I think that we should prefer java processing at least because of the additional speed gained because the code is already compiled and the direct manipulation of objects.

Ovidiu Maxiniuc wrote:

The difficulty appears when there are another buffers (and aliases)...

Please explain your concerns in detail on both these points.

...and even more, when some client-side where clause has to be constructed also. We should do some tests to detect the maximum complexity of these and find a solution to instantiate this object.

Yes, we have to design for the most complex circumstances possible, since if it is possible, someone will have done it. I do agree that the client-side where clause processing is somewhat problematic, due to it being a callback and the open-ended nature of the expressions that can be represented there.

At the core of this approach, I see something like a Runnable interface, but which returns the object instance we care about. Perhaps something like this:

public interface Interpretable<T>
{
   public T interpret(Aast jast);
}

The idea being that T is the type of object you want returned, like a P2JQuery or a WhereExpression, and the implementation of interpret(Aast) is where the walk happens.

This idea is not fully formed yet, so I welcome feedback.

I'm not sure how well an interpreted WhereExpression would work. We may still have to fall back to a compiled approach for these, since the expressions they represent can be quite flexible, though I'd like to avoid that if possible. The good news, however, is that they should be relatively rare; at least, that's what we've found with static conversion.

Ovidiu Maxiniuc wrote:

The java/TRLP debate. It's clear that TRPL has the some advantages, but I think that we should prefer java processing at least because of the additional speed gained because the code is already compiled and the direct manipulation of objects.

I tend to agree in terms of the final walk of the JAST, where we actually invoke the constructors and methods using reflection. Although TRPL is designed for walking ASTs, I think in this case the advantages of using Java for this stage of the process outweigh those of TRPL.

However, I think all of the other steps up to that point to create the JAST should continue to be in TRPL, with callouts to helper methods from JavaPatternWorker (or a new pattern worker if they're extensive) for the messy parts (e.g., figuring out which c'tor or method variant is the right one for a particular JAST node, etc.).

I'm not sure how well an interpreted WhereExpression would work. We may still have to fall back to a compiled approach for these, since the expressions they represent can be quite flexible, though I'd like to avoid that if possible. The good news, however, is that they should be relatively rare; at least, that's what we've found with static conversion.

In #2355 I fixed a defect with these expressions, I am trying now to determine how complex these might be on the customer server project (as a minimum requirement for now).
On the other hand, the QUERY-PREPARE only accept only quoted constants or an unabbreviated, unambiguous buffer/field reference for buffers known to query otherwise error 7328 will occur. The predicate of FIND-FIRST/LAST methods has a fixed syntax that might reduce the complexity.

However, I think all of the other steps up to that point to create the JAST should continue to be in TRPL, with callouts to helper methods from JavaPatternWorker (or a new pattern worker if they're extensive) for the messy parts (e.g., figuring out which c'tor or method variant is the right one for a particular JAST node, etc.).

True. In the end, if this step is still a bottleneck, creating the JAST tree is the possible subject for further optimization.

I think the following would give us a reasonably complex WhereExpression:

find first <permanent table buffer>
where
  <permanent table buffer>.<integer field> = <integer constant>
and
  if <local logical variable>
     then true
     else can-find(first <temp-table buffer> where <temp-table buffer>.<integer field> = <integer constant>)

I haven't actually tried this; is it possible to do a dynamic find-first() that is the equivalent of this construct, or would this trigger a Progress error?

Looking at find-first() method in manual, the predicate expression is very simple, just

[ WHERE [ logical-expression ] ] [ USE-INDEX index-name ]

OK, so no calls to UDFs, either? That's another way to trigger client-side where clause processing.

If not, it would seem it is not possible to generate a client-side WhereExpression from a find-*() method. That's good news.

Is it possible to do either of these things in a query-prepare() predicate expression? The documentation makes it seem like those are more open.

Yes, query-prepare() is the big issue. I believe there are no constrains here and any number of buffers can be added using set/add-buffer() methods.
I will try to construct an example based on your note #8 above.
I good idea is to log the encountered queries from the server itself while running the set of tests. That way we should get an idea of what we should expect.

Ovidiu Maxiniuc wrote:

Yes, query-prepare() is the big issue. I believe there are no constrains here and any number of buffers can be added using set/add-buffer() methods.
I will try to construct an example based on your note #8 above.

Also try defining a Progress function and invoking it from a where clause.

I good idea is to log the encountered queries from the server itself while running the set of tests. That way we should get an idea of what we should expect.

I've done this; see #1868, note 24.

Of course, we can't limit our solution to ignore client-side where clause considerations, even if they're not represented in these debug dumps. Also, consider that this output may not be correct, if we don't already handle this case with the existing solution -- I'm not sure we do. Please test this, if you manage to get such a test case to work in Progress.

I started implementing a small 'interpreting' engine, targeting only the required node/items. In fact at this time, the interpreter is able to process the simpler FIND-FIRST / FindQuery methods.
However, when the JAST gotten more complex, in the case of OPEN QUERY / AdaptiveQuery, I am getting the following error in printed in message area:

    ** Logical dbname dictdb must be connected in order to add alias dictdb. (1660)

The line that causes it is equivalent to:

    ConnectionManager.createAlias("dictdb", "p2j_test");

I saw that there are some recent changes regarding this alias created for the first connected database but I did not went too deep on it.

Should I ignore the node:

               ConnectionManager.createAlias [STATIC_METHOD_CALL] @0:0
                  dictdb [STRING] @0:0
                  p2j_test [STRING] @0:0

Ovidiu Maxiniuc wrote:

I started implementing a small 'interpreting' engine, targeting only the required node/items. In fact at this time, the interpreter is able to process the simpler FIND-FIRST / FindQuery methods.

Very cool! I am very interested to review when you have something you can share.

However, when the JAST gotten more complex, in the case of OPEN QUERY / AdaptiveQuery, I am getting the following error in printed in message area:

** Logical dbname dictdb must be connected in order to add alias dictdb. (1660)

when I process the jast code.

The line that causes it is equivalent to:

ConnectionManager.createAlias("dictdb", "p2j_test");

in the 'execute' method before calling the constructor fro the query.

The text of the error message suggests you are passing the same parameter (dictdb) for both the alias and ldbName. Are you sure you are passing p2j_test as the ldbName in the interpreted createAlias call?

While I am excited that you've begun implementing a solution, I'm curious whether you've found the answers to the questions we documented in notes 7-12 above. Most importantly, is it possible/legal in Progress to construct a dynamic query which will trigger client-side where clause processing?

Related to the createAlias topic, here's an email exchange I had with Constantin recently:

Eric,

The problem here is that at conversion-time the SchemaDictionary (IIRC) needs to be aware of any explicitly created DB aliases... otherwise, when that alias is used in the query, it will not be recognized properly.

Thanks,
Constantin

On 12/15/2014 1:32 AM, Eric Faulhaber wrote:

Hi Constantin,

Do you recall why this bit of code from DynamicQueryHelper.prepareTempTable(SchemaDictionary dict, StringBuilder pcode) is necessary?

       ...
       // custom database aliases...
       Map<String, String> aliasesByLDB = ConnectionManager.get().getAliasesByLDB();

       for (String ldb : ldbs)
       {
          String alias = aliasesByLDB.get(ldb.toLowerCase());
          if (alias != null)
          {
             String stmt = "CREATE ALIAS %s FOR DATABASE %s.%n";

             pcode.append(String.format(stmt, alias, ldb));
          }
       }
       ...

Wouldn't the dynamic query code automatically pick up all aliases in the context in which it is run without this?

Thanks,
Eric

Ovidiu Maxiniuc wrote:

I did some researches on windev01. Progress is not very permissive. The static statements can be much more complex than the and dynamic calls using methods/handles.

Does that mean that neither of the two ways we discussed above of creating a client-side where clause condition are allowed in Progress? This would be very good news.

I also downloaded and I had a look over the logs in #1868, note 24. There are no WhereExpression used in any of the queries.

Hopefully, that means there shouldn't be any, and we're doing things correctly. I guess at this point, since we're passing most of the tests and we aren't getting any errors in the server.log that suggest a client-side related, dynamic query problem, we can be pretty optimistic that this is the case.

Instead I noticed occurrences of CompoundQuery s that will complicate the implementation.

Yes, though once we have a generic way of identifying and invoking methods and constructors, this will just represent more of the same.

If you have the chance to re-run it, please add the string predicate before and, to simplify the files, only log the 'final' JAST/java source as feed to im-memory compiler. The version before the post-processing does not contain really important info.

Sure, I can do that, but are you certain you want to use the current, final version of the JAST as a starting point? I thought the step to get to this version does a lot of things we no longer want to do (e.g., adding a getQuery method, adjusting instance variables, etc.). I had assumed we would be starting with the first version and modifying it quite differently to get to the final version we now need. However, I haven't looked at your alpha yet, so maybe your intent will become more clear once I do...

I attached an 'alpha' version. It certainly needs to be restructured.

Excellent, I will have a look.

Eric Faulhaber wrote:

Sure, I can do that, but are you certain you want to use the current, final version of the JAST as a starting point? I thought the step to get to this version does a lot of things we no longer want to do (e.g., adding a getQuery method, adjusting instance variables, etc.). I had assumed we would be starting with the first version and modifying it quite differently to get to the final version we now need. However, I haven't looked at your alpha yet, so maybe your intent will become more clear once I do...

My request was only to have a more insight of what are we dealing with. When possible. At this moment I prefer the post-processed JAST just because is simpler once the fix_ RULES were applied. In the final solution, this step is not needed and the access to different parts of the JAST to be done using direct method calls, building objects and calling methods by reflection, without extra processing in the JAST tree. Maybe I see this too simple, but the target is to build dynamically a P2JQuery object using JAST as a map for the pieces.

I've taken a look at the code so far and it is a great start! Yes, I agree that the final tree is simpler to deal with for the POC. I think the approach you are taking makes a lot of sense for the alpha, however I don't want to take the implementation much further inside DynamicQueryHelper.interpret, once you're satisfied the concept works.

Much of what you are doing in the new DynamicQueryHelper.interpret method is what I think we should do in TRPL, possibly in reworked versions of convert/fix_in_mem_find_query.xml and convert/fix_in_mem_query.xml (btw, it probably makes sense to move these from rules/convert to a new rules/runtime directory, so their purpose is clear).

This is where we should be letting TRPL help us with the walk. For example, I'm thinking of something like this:

<variable name="parameters"   type="java.util.List" />
<variable name="methodRef"    type="java.lang.reflect.Method" />
<variable name="ctorRef"      type="java.lang.reflect.Constructor" />
<variable name="classRef"     type="java.lang.Class" />
...
<walk-rules>
   ...
   <rule>type == java.method_call or type == java.constructor      
      <action>classRef = this.getAnnotation("java-class")</action>
      <!-- handle no-arg methods and c'tors immediately; there will be no descent -->
      <rule>this.numImmediateChildren == 0
         <rule>type == java.method_call
            <action>methodRef = classRef.getDeclaredMethod(text)</action>
            <!-- TODO: add error checking for null return -->
            <action>putNote("method", methodRef)</action>
         </rule>
         <rule>type == java.constructor
            <action>ctorRef = classRef.getDeclaredConstructor()</action>
            <!-- TODO: add error checking for null return -->
            <action>putNote("ctor", ctorRef)</action>
         <rule>
      </rule>
   </rule>

   <rule>
      parameters != null and
      (parent.type == java.method_call or parent.type == java.constructor)
      <action>parmTypeRef = java.getClassForAst(this)</action>
      <!-- TODO: add error checking for null return -->
      <action>parameters.add(parmTypeRef)</action>
   </rule>
   ...
</walk-rules>

<descent-rules>
   ...
   <rule>type == java.method_call or type == java.constructor
      <action>parameters = create("java.util.ArrayList")</action>
   </rule>
   ...
</descent-rules>

<ascent-rules>
   ...
   <rule>type == java.method_call or type == java.constructor
      <rule>type == java.method_call
         <action>methodRef = java.findMatchingMethod(classRef, parameters)</action>
         <!-- TODO: add error checking for null return for method -->
         <action>putNote("method", methodRef)</action>
      </rule>
      <rule>type == java.constructor
         <action>ctorRef = java.findMatchingConstructor(classRef, parameters)</action>
         <!-- TODO: add error checking for null return for constructor -->
         <action>putNote("ctor", ctorRef)</action>
      </rule>
      <action>parameters = null</action>
   </rule>
   ...
</ascent-rules>

As today, the source AST for this rule set is the one that comes out of the first-pass JAST processing. That step would need to be enhanced to store some annotations that we rely on here (like java-class, the java.lang.Class object that is the target of a method call or constructor call). It makes sense to do it in that earlier stage, because it is at that point that we have the most information about why we are storing a particular method or constructor in the JAST.

This rule-set would have additional rules to do the refactoring that we want (removal of all the unnecessary nodes, etc.). Or we may find that it makes more sense to do this refactoring in an intermediate rule-set that feeds into this one. In fact, we may have to, if the refactoring still needs to be different for FIND-*() and PREPARE-QUERY() JASTs.

You will note also some calls to helper functions that don't exist yet (e.g., findMatchingConstructor, findMatchingMethod, getClassForAst). These should be added to the JavaPatternWorker$JavaAstHelper inner class. It looks like you have made a good start on findMatchingMethod already.

This rule-set is very generic, and really doesn't have any domain knowledge of the methods, arguments, constructors, etc. it is walking. It's just collecting information about types, signatures, methods, constructors, and so on, and storing this in annotations which will be used in the final interpreter walk in Java code, which we want to be as easy as possible.

I know it seems like a lot of extra effort splitting the interpreter into two phases like this, but there is a method to my madness. Once this improvement is implemented, I want to be able to implement smart caching of the JASTs. To make this worthwhile, they should contain the results of as much of the expensive work as we can do up front as possible (e.g., refactoring and minimizing the JAST, collecting signatures, matching methods and constructors). The final walk to actually invoke everything via reflection should be as "thin" as possible, just doing the minimum to gather the arguments that aren't available until runtime. That way, we amortize the cost of the runtime conversion as much as possible.

• com.goldencode.p2j.pattern.CommonAstSupport$Library.create
• com.goldencode.p2j.pattern.CommonAstSupport.matchConstructor
• com.goldencode.expr.SymbolResolver.matchTargetMethod
• com.goldencode.expr.SymbolResolver.resolveFunction
• com.goldencode.expr.SymbolResolver.introspectFunction

Eric Faulhaber wrote (from Constantin's email):

The problem here is that at conversion-time the SchemaDictionary (IIRC) needs to be aware of any explicitly created DB aliases... otherwise, when that alias is used in the query, it will not be recognized properly.

This suggests that the CREATE ALIAS statement(s) currently added before the query are there specifically for parsing (schema name resolution) purposes. If that's the case, these nodes should be removed from the Progress AST as early as possible/practical after parsing, or at least before the core conversion phase, when we generate the JAST. They should not be converted into the JAST, since they shouldn't be executed at runtime (the runtime state already includes the aliases).

Ovidiu Maxiniuc wrote:

If you have the chance to re-run it, please add the string predicate before and, to simplify the files, only log the 'final' JAST/java source as feed to im-memory compiler.

Attached to #1868, note 27.

Ovidiu Maxiniuc wrote:

Attached you can find a first version of 'runtime/interpret_find_query.xml'. It only handles the simple find-query cases.

This is good, closer to what I am looking for. I think you've proven the concept can work. Let's focus on a robust, generic implementation now.

One thing I'd like to refactor is to defer the invocation of methods or constructors. We don't want to do this at this stage, unless the result of the invocation is something static that will be used across multiple executions of the same query (i.e., in the not-too-distant future, when we're caching these JASTs). The TRPL work should just be about annotating information and refactoring the JAST to make it as small and fast as possible. That will allow us to figure out as little as possible on a final walk in Java code, so we can just retrieve those annotations, references to buffers, contents of variables, and then use all this to execute.

The basic flow I'm trying to get to is:

1. JAVA: prepare-query request comes into DynamicQueryHelper; the buffers should have been added/set by this time.
2. [FUTURE JAVA: we have the buffers and predicate; it should be possible to do a cache check here; if we find an existing JAST for this combination of predicate and buffer types, skip to step 7].
3. JAVA: parse the predicate; drive TRPL runtime rules.
4. TRPL: convert up to the first pass JAST we have today, but with modifications (see below).
5. TRPL: refactor JAST to create minimalist version (only the absolute minimum nodes we need for interpret/invoke walk); we want this to have as few nodes as possible, both for speed of the final walk, and to reduce memory footprint when cached; also determine and annotate c'tors, methods, other needed info.
6. [FUTURE JAVA: cache the JAST using a cache key comprised of the buffer types (probably class name is sufficient) and original predicate].
7. JAVA: do a final walk of the JAST; extract annotations; look up the appropriate buffer when we hit a buffer reference node that we marked earlier; store the P2JQuery object when we invoke the c'tor that we marked earlier.

At that point, the rest of the code path is largely the same as it is today.

• When we store a CONSTRUCTOR node for a particular query class (e.g., FindQuery), we know at this moment which query class we want to use, so we should store a "java-class" (or similar) annotation with the com.goldencode.p2j.persist.FindQuery class in that node. At this point, we don't know which variant of the c'tor we will use, but it's OK to figure that out later, in step 5.
• Also when we store this query CONSTRUCTOR node, mark it with a special annotation, so we can identify it during the final walk as the node which creates the P2JQuery we need. This is the replacement for the getQuery method we have today.
• As we add the REFERENCE node for a buffer parameter in the query c'tor or an addComponent method, mark it with a special annotation. This will tell us during the final walk that we need to retrieve the buffer which needs to be passed for this parameter. This is the replacement for today's mechanism of using reflection to store the buffers into the dynamically compiled class' public instance variables.
• When we add a CAST node to satisfy a particular method or c'tor signature (like when casting a null HQL where clause to String), we should add a "java-class" annotation for the specific class -- this will help us identify the correct method or c'tor in step 5.
• A reference to a static, final variable like LockType.NONE is essentially a constant. This currently is represented as a REFERENCE node with text LockType.NONE. We should add a "constant" annotation which stores an actual reference to a LockType.NONE object. Such an annotation can be used later, both in step 5 to determine a c'tor or method signature, as well as in step 7 as a ready-to-pass argument.

As we're implementing step 4, it is also a good time to make any improvements to the structure of the JAST which require domain-specific knowledge, to make downstream processing easier. For example, when in runtime conversion mode, we would avoid creating the STATIC_METHOD_CALL nodes for ConnectionManager.createAlias calls at this step.

Up until now, I've been thinking about step 5 being forked into different rule-sets for FIND and OPEN-QUERY features. However, I think if we do a good enough job annotating in step 4, we should be able to make this a very generic program implemented in a single XML file. Something like:

<rule-set>
   strip out all unneeded nodes here:
      package statement
      all import statements
      compile unit node
      class node
      all CS_* nodes
      BlockManager-related nodes
</rule-set>

<rule-set>
   add generic annotations needed by interpret/invoke walk:
      constructors
      methods
      other?
</rule-set>

However, during the weekend I worked on my own approach to the runtime interpreter, in pure java. It is part of the same archive, RuntimeJastInterpreter. It has a little more functionality than the alpha POC from Friday. The idea was to have a fast runtime engine that is able to interpret pieces of the code and extract results stored in variables :
[...]
I won't continue to work on this code.

Don't abandon it just yet...it will not need to do as much as it does now, but this is a good candidate to morph into the "thin" walk in step 7.

Don't abandon it just yet...it will not need to do as much as it does now, but this is a good candidate to morph into the "thin" walk in step 7.

Indeed, No matter how much object I can annotate, there will be things that cannot be processed until runtime. For example:
COMPILE_UNIT/KW_CLASS/CS_INSTANCE_METHODS/METHOD_DEF/BLOCK/ASSIGN/CONSTRUCTOR/EXPRESSION/INITIALIZER/EXPRESSION/STATIC_METHOD_CALL/CONSTRUCTOR/STRING/
corresponds to
AdaptiveQuery//new Object///isEqual/abs/decimal/1.3
that was generated from java open-query: "FOR EACH b-3 WHERE (b-3.book-id NE ", valueOf(1001), ") AND (ABS(100000000000000) eq 1.3) AND (UPPER('a') eq 'A')"
The AND -s are really constants and we should process them at conversion time.

Another idea. I think we should implement the caching soon. As you mentioned, the true constructor cannot be evaluated in TRPL, but at runtime we have the parameter types. At this moment the matching Constructor is detected and can be stored back into the JAST node as annotation. If the cache will indicate that the already built JAST can be used, the effort to detect the constructor again is eliminated. The same for methods, static methods, and even for constants or constant expressions. The first evaluation will take the "hit" but the next calls will be much faster.

Certainly static expressions will be more efficient to convert, execute, and store at conversion. I'm pretty sure we don't do this during conversion today. If we're going to do this, we should do it for static conversion as well as runtime. I believe these expressions will convert inline in HQL currently, as long as they use only regular operators and built-in functions, which will perform reasonably well already. In any case, I think this is likely to be a larger effort with limited real-world uses (except possibly for unanticipated preprocessor output), and as such it is outside the scope of this task.

I agree that we implement the caching very soon: immediately after this task, unless we believe it will be truly trivial. If the combination of fully qualified buffer type names and query predicate string (assuming we no longer prepend a unique query name) form a reliable cache key, this should be pretty easy to implement. But again, unless the effort is just an hour or two, I don't want the scope of this task to creep.

This is true, we don't do the constant expression evaluation at this moment neither in static conversion but it would be nice. Before the final brew, traverse the EXPRESSION tree and if constant, or parts of it, cut respective branch and replace with the computed value. However this should be deferred to another task.

My thoughts were that we should annotate more than P2JQueries. Even if my example contained toy-expression, math operations and other operators are very likely to appear in where expressions. At such a later stage (final java evaluation of even final post-processing in step 5) it is difficult (time consuming) to detect which method/constructor should be applied for abs, isEquals or decimal used in my example. We should do the runtime annotations in earlier stages (annotation/variable_definitions and convert/operators) but even at that point, the exact method is not known because the children type is not evaluated or their evaluation cannot be done at that moment. The static call is emitted, relying on java compiler to resolve the actual method to be invoked looking into the list of the ones we overload.
In consequence this final step before a node evaluation must be done after the children evaluation and because of the cost, it must be cached as annotation to the respective node for later calls and the whole TRPL tree cached as you described in steps 2 & 6.

On the other hand, if we do to many semantic annotations, the tree will grow - and the management will require additional CPU and memory, even if in the final step it will be cut, throwing away unneeded already annotated nodes. Another thing that will increase the CPU usage is that before doing the actual annotations, we should always check if we are processing in 'runtime' mode. I already created a function that does this before doing these annotations to minimize the increase of the complexity of the TRPL script.

Ovidiu Maxiniuc wrote:

The static call is emitted, relying on java compiler to resolve the actual method to be invoked looking into the list of the ones we overload.

Don't we convert these kinds of expressions into HQL today, using PL/Java UDFs to represent the built-ins being called (e.g., ABS, UPPER)?

Ovidiu Maxiniuc wrote:

On the other hand, if we do to many semantic annotations, the tree will grow - and the management will require additional CPU and memory, even if in the final step it will be cut, throwing away unneeded already annotated nodes. Another thing that will increase the CPU usage is that before doing the actual annotations, we should always check if we are processing in 'runtime' mode. I already created a function that does this before doing these annotations to minimize the increase of the complexity of the TRPL script.

Maybe a JAST is not the best data structure to store in memory for a cache (for one thing, there will be a LinkedHashMap at every annotated node, which can chew up memory quickly). Can you think of a flatter/leaner data structure which is better optimized for caching and perhaps for interpreter performance as well?

Eric Faulhaber wrote:

Don't we convert these kinds of expressions into HQL today, using PL/Java UDFs to represent the built-ins being called (e.g., ABS, UPPER)?

In this case no, P2J decided to evaluate on client the expressions and pass the result as parameters:

      query0 = new AdaptiveQuery(b3, "b3.bookId != 1001 and ? and ?", null, "b3.id asc", new Object[]
      {
         isGreaterThan(abs(100000000000000L), new decimal("1.3")),
         isEqual(toUpperCase("a"), "A")
      });

After I got satisfactory results with simpler queries, I started to increase the complexity of the dynamic queries in order to catch new types of nodes that have to be handled/interpreted. I find in #1868 queries similar to: OPEN QUERY inMemQuery16 FOR EACH b-3 WHERE b-3.isbn <> 'Blue' AND b-3.isbn <> 'BLUE', EACH tmp WHERE tmp.af = b-3.book-id.
This is parsed to following JAST (simplified):

    [ASSIGN] @0:0
      query0 [REFERENCE] @0:0
      CompoundQuery [CONSTRUCTOR] @0:0
          [BOOL_TRUE] @0:0
          [BOOL_FALSE] @0:0
   addComponent [METHOD_CALL] @0:0
      query0 [REFERENCE] @0:0
      AdaptiveQuery [CONSTRUCTOR] @0:0
         b3 [REFERENCE] @0:0
         upper(b3.isbn) != 'BLUE' and upper(b3.isbn) != 'BLUE' [STRING] @0:0
          [NULL_LITERAL] @0:0
         b3.id asc [STRING] @0:0
   RecordBuffer.prepare [STATIC_METHOD_CALL] @0:0
      tmp [REFERENCE] @0:0
   addComponent [METHOD_CALL] @0:0
      query0 [REFERENCE] @0:0
      AdaptiveQuery [CONSTRUCTOR] @0:0
         tmp [REFERENCE] @0:0
         tmp.af = ? [STRING] @0:0
          [NULL_LITERAL] @0:0
         tmp.id asc [STRING] @0:0
          [EXPRESSION] @0:0
            new Object [REFERENCE_DEF] @0:0
             [INITIALIZER] @0:0
               FieldReference [CONSTRUCTOR] @0:0
                  b3 [REFERENCE] @0:0
                  bookId [STRING] @0:0 

      query0 = new CompoundQuery(true, false);
      query0.addComponent(new AdaptiveQuery(b3, "upper(b3.isbn) != 'BLUE' and upper(b3.isbn) != 'BLUE'", null, "b3.id asc"));
      RecordBuffer.prepare(tmp);
      query0.addComponent(new AdaptiveQuery(tmp, "tmp.af = ?", null, "tmp.id asc", new Object[]
      {
         new FieldReference(b3, "bookId")
      }));

• there are ~5500 FIND FIRST of which ~4900 are various queries on "ttfiltercriteria", apparently on same field, "FieldName", only the value searched differs. The best long-run of queries is on FieldName = "cde": ~600, but otherwise they are rather different as keys for a cache so we would need ~2500 (estimated) cache entries.
• I am thinking of caching the already processed queries in a more intelligent way. What if we analyze the following queries:
  

      FIND FIRST ttfiltercriteria WHERE FieldName = "aBatchSubmitTreeNodeID" NO-LOCK.
      FIND FIRST ttfiltercriteria WHERE FieldName = "abatjob-num" NO-LOCK.
      FIND FIRST ttfiltercriteria WHERE FieldName = "active" NO-LOCK.
  

  
  and realize that they are the same ?
• We just need to be able to identify the strings / integers / other-possible parameterizable values and replace them with some kind of a token that can be later identified in the processed JAST. This way, all ~4900 occurrences would benefit from a single execution of the runtime conversion and, after the 1st evaluation (that will fill-in the missing annotations).
• Using this technique the cache will drop to size of tens or maybe few hundred which is acceptable to keep the JAST in memory.
• Implementation:
  1. parameter detection
  - string detection from the query should be simple, we just need to look after pairs of " and '
  - integer, logical detection should be rather simple, after tokenizing the query (need more thoughts on these types on implemntation, however)
  2. replace them with indexed tokens like $1, $2, etc. This way, the above queries will be grouped in same 'class':
  

  FIND FIRST ttfiltercriteria WHERE FieldName = "$1" NO-LOCK.
  

  
  along with other 4900 similar queries.
  3. if such JAST is not already in cache, process it and save it with (parametrized-form and ttfiltercriteria) as cache key
  4. set the $1 = "aBatchSubmitTreeNodeID" (actual value) in the java interpreter before calling evaluation method
  5. when a string is encountered it is scanned for indexed tokens, if they are found, they are replaced with actual values before returning

• this works fine for for string values because the converted will consider them as string token, and it ignores their content, for other datatypes, I cannot think of a solution at this moment that would work naturally, without many changes in the conversion rules.

and realize that they are the same ?

Don't forget that in the Progress AST the tree structure is in fact:

• The same tree, the parameter in this case would be a STRING node. Only the text of that node would differ.
• Already properly parsed into the proper form.

I recommend detecting such cases using the Progress AST, it will be MUCH easier and MUCH more reliable. Then choose a different conversion for those nodes that must now be substitution parms. I think we may even have an annotation for that, right?

Not sure I understand. Of course, the trees will be the except for the string value at some point. But later, during the conversion, the string will be merged back to the where expression so the node will look similar to:

FindQuery [CONSTRUCTOR] @0:0
    ttfiltercriteria [REFERENCE] @0:0
    "upper(ttfiltercriteria.fieldname) = 'ACTIVE'" [STRING] @0:0
    null [NULL_LITERAL] @0:0
    "ttfiltercriteria.fieldname asc" [STRING] @0:0
    LockType.NONE [REFERENCE] @0:0

My first idea is to avoid building any AST tree a second time. It's faster but I must admit, it's a more like heuristic approach that may fail.
I guess that the solution that combines the speed and accuracy looks like:

ProgressParser parser = new ProgressParser(lexer, sym);
parser.external_proc();
ProgressAst progAst = (ProgressAst) parser.getAST();
progAst.brainwash(wa.dynQueryFile, true);

// new:
List<String> params = extractParameters(progAst); // replace string (int/decimal ?) constants with $1, $2, etc
String cacheKey  = getCacheKey(progAst); // gets a unique representation of the query with constants replaced by parameters
qast = check-cache(cacheKey, buffers)
if qast == null
      ConversionPool.runTasks();
      processor.postprocessJavaAst(jcode, false);
      store qast in cache
build interpreter
interpreter.setReplacements(params)
interpreter.evaluate()

A few thoughts on the caching:

I agree with Greg that the Progress AST will be much more reliable. However, it is another AST to store in a cache, and most likely much larger than the minimal JAST we are caching. This will more than double the memory requirement. I already was wondering whether there is a flatter/leaner data structure we could use for the JASTs. Is the JAST the most "distilled" form of information, or is there something better which could be the product of this process? Maybe even just a specialized Aast implementation which uses a leaner approach for the annotations and drops the instance variables in AnnotatedAst which we don't need. Now we would have to consider how to make the cache key much more compact as well.

I think using the buffer names (like ttfiltercriteria) as part of the key is error-prone. It happens to be done consistently in the application data we've reviewed, but I could easily write a test case which uses the same buffer name for different tables. We need to use the fully qualified name of the DMO instead, plus the predicate string. Perhaps in a follow-up, smarter implementation, we would parameterize the name of the buffer in the predicate string, so that different buffer names/aliases used for the same backing table would be recognized as the same.

Let's take a step back at this point, though. It is good to be discussing more sophisticated approaches, but I don't want to make the caching implementation particularly complicated in our first pass. It will take longer and is more likely to introduce bugs. My main goal at this point is to understand how much caching will improve performance, then we can refine the approach and perhaps give back a little of that gain to have a much smarter, leaner implementation. For now, we can achieve this early goal with a very simple implementation.

So, the approach I want to take is to start with a relatively "dumb" design, which just uses the fully qualified DMO class names and the original predicate string (without prepending any unique query names), without being overly concerned about minimizing duplicates yet. Maybe we lowercase the predicate in the key to at least eliminate duplicates caused by case differences. We should be able to implement that in just a few hours, I would think. Let's see what that buys us, and we can refine it from there.

When I refer to the Progress AST, I mean the original AST that is first created for the provided expression in a QUERY-PREPARE(). Don't we create a regular AST using the progress.g? That is the place to analyze and potentially mark things as substitution parms which normally would convert as inline HQL text.

I don't mean for an extra parsing step to occur. The best time to make this a parameter is during the original conversion.

Of course, we would only do this for runtime queries.

Please keep in mind that I haven't reviewed your solution, so perhaps I am misunderstanding how things work.

Greg, you are right.

We do parse the initial query using the progress.g (ProgressParser from the code in note 36). And this is part of the current implementation. This is already compiled code so it must be fast. Yet, I have to study what the best solution for these parameters.

I will delay the exact implementation as Eric suggested.

Thank you for your valuable input.

Before I write up the code review, I need a definitive answer on the client-side where clause processing question in note 18. You wrote that Progress was not very permissive for the dynamic FINDs and that you didn't find any instances of WhereExpression in the logs. But we need to know for sure if there is any way for this to happen. If so, it will be a big hole in the current implementation.

Code review 0129a:

I really am excited about this update. Nice work overall, a great first pass. Some implementation nits to pick, nonetheless:

• I'm not crazy about adding an unused boolean parameter to Aast.putAnnotation, to keep the name but change the signature for the compiler. Please drop the boolean and just change the name to putAnnotationObject.
• Why is there a deprecated tag in a new class (JastInterpreter) for the LockType processing? This is duplicative with the code in database_access.rules, right? If it's truly dead, let's remove it. If not and it's covering some limitation in the TRPL change, can we fix it now in database_access.rules or add a TODO there?
• I think we can still do a better job of merging the FIND and OPEN QUERY fix-up TRPL into a single rule-set, but let's defer that for now. We can work incrementally toward that.
• In ConversionProfile, please remove the deprecated items. We're not moving back to the old way; we'll have to make this implementation work one way are another.
• Can the deprecated template in java_templates.tpl be removed? Again, this is a one way trip.
• Let's dump the old code in DynamicQueryHelper, we can get back to it in version control if we need to review.
• Please bump the cache size up to 2048 and put a TODO in about making its size configurable from the directory.
• Please make the debug and performance logging optional (currently going to stdout), with different levels of verbosity if necessary. We may want to output this to a separate log file.
• We don't still need to prefix a unique query name to the OPEN QUERY statements, do we?
• Why are we doing so much parsing work before checking the JAST cache (all the WorkArea updates, setting up the schema dictionary, preparing the temp-table AST, parsing the predicate)? Is this in anticipation of the smarter cache that requires a parsing pass?
• Please split the big synchronized block into two: one synchronized cache check as early in the method as possible, followed by the (unsynchronized) conversion if a cache miss occurs; then a second, synchronized block containing a cache check and cache put if the check misses again. This probably will require refactoring the try-catch-finally blocks somewhat. There may occasionally be race conditions and some wasted conversion work done, but I think the benefit of minimizing contention in this area is more important. The conversion times are very long relative to the execution times; we don't want to block other threads for tens or hundreds of milliseconds at a time.
• The JAST still seems to me to have too much class structure and method cruft. I know you key off some of these nodes in the interpreter, but I had expected you to use annotations for that instead. Not sure which is the better approach; the JAST looks bigger with the extra nodes, but in practice, dropping those nodes and using the extra annotations may actually increase the memory footprint. Let's leave it alone for now, and keep thinking about ways to make these thinner.

1. answer any questions posed above;
2. make all the changes noted above, EXCEPT:
• refining the FIND and OPEN QUERY TRPL rules;
• stripping more of the JAST nodes; and
• moving the cache check ahead of the other prep work, assuming you have a good reason for doing the cache check as late as it is.

BTW, I've run very large sets of tests using this implementation (with the larger cache size), and it's performed quite well. In the best cases, I'm seeing up to a 70% speed improvement! I've also found a few errors: some queries it can't deal with (probably would have failed with the old implementation, too), and at least one case of the JAST interpreter losing its mind on a method. I'll post these as separate Redmine issues.

Code review 0205a:

• Please put back the indent at DynamicQueryHelper:101.
• I think the logging levels used in DynamicQueryHelper are a little off. With normal use, we shouldn't see every cache action in the log. Please set logging for the cache activity (puts and gets) to FINE, as well as the elapsed time entries. Please log the race condition at INFO, and make the message text less alarming -- customers may worry unnecessarily with the current text. OTOH, the failure to interpret the JAST (line 228) represents a serious situation that requires action on our part; it should be logged as SEVERE, since that represents a broken query which likely will result in a problem at the application level. Notification that the cache is full should be at INFO level; it is a normal operation for the cache to expire elements, but it is good to know for purposes of tuning the cache size. When you obtain the logger, use LogHelper.getLogger(DynamicQueryHelper.class.getName()) instead of LogHelper.getLogger(DynamicQueryHelper.class). This will allow us to change the log level for this class in particular. The latter is scoped to the package for some reason.
• In DynamicQueryHelper.buildIndex, why did you make a change to hard-code the index annotations UNIQUE, PRIMARY, and WORD to true? That looks wrong.
• Why did you indent build.xml:265?
• What is the TODO at BufferImpl:2210 about?
• Why was DatabaseManager included in the update? It has no changes.

Otherwise, everything looks good.

Please post a new update addressing these items and run it through a conversion regression test (I don't need to review again, unless you decide to make other changes). The changes to the runtime classes aren't used in the regression test environment, so runtime regression testing won't be necessary. However, I'll run that update through a representative set of customer unit API tests to make sure it doesn't regress anything there. If that goes well, we can check it in.

Thanks, Ovidiu. Please post your conversion results when that's done. I'll run my tests later today or overnight.

Eric,
The last updates fail to convert the majic sources. The reason is the putAnnotation() method. Although I used cast in my code to guide the compiler to the correct variant of the overloaded method, converting the Majic code revealed other place (possible other may exist) where P2J is unable to detect the right method. The result is that conversion stops with following exception:

     [java] Caused by: com.goldencode.expr.AmbiguousSymbolException: com.goldencode.ast.Aast.putAnnotation
     [java]     at com.goldencode.expr.SymbolResolver.matchTargetMethod(SymbolResolver.java:1465)
     [java]     at com.goldencode.expr.SymbolResolver.introspectFunction(SymbolResolver.java:1336)
     [java]     at com.goldencode.expr.SymbolResolver.resolveFunction(SymbolResolver.java:1179)
     [java]     at com.goldencode.expr.SymbolResolver.resolveFunction(SymbolResolver.java:1120)
     [java]     at com.goldencode.expr.ExpressionParser.method(ExpressionParser.java:2042)
[...]

The best alternative is to use your solution from note 43 and rename the new method to putAnnotationObject() so the rest of TRPL remain untouched.

Ok, please do this and run the test again.

The regression test was successful. Please commit and distribute the update.

Eric,

I tried such constructs on windev01.

CAN-FIND: P2J parses and successfully converts them in java code, but P4GL is more restrictive, even in static mode:

CAN-FIND is invalid within an OPEN QUERY. (3541)
**  Could not understand line 8. (196)

Variables: In static case, references to variables are allowed:

DEFINE VARIABLE kkk AS INTEGER init 100.
open query q3 FOR EACH book where book.book-id <> kkk SHARE-LOCK.

kkk must be a quoted constant or an unabbreviated, unambiguous buffer/field reference for buffers known to query . (7328)

DEFINE VARIABLE my-isbn AS INTEGER init '111-111-111-111-1'.
hq:query-prepare("FOR EACH book where book.isbn <> " + quoter(my-sibn)).

Internal functions: are allowed. If they are applied to buffers/fields. P2J wires our UDFs in place.

User defined functions: are only allowed in static queries as in your example.
The following construct

hq:query-prepare("FOR EACH book where book.book-id eq udf(100)").

       ** Unable to understand after -- ".book-id eq udf". (247)
PREPARE syntax is: {FOR | PRESELECT} EACH  OF.. WHERE ... etc". (7324)

However, I also think that is good to support this kind of JAST trees. Otherwise we could get uncovered by some exotic case we could not think of for the moment.

Eric Faulhaber wrote:

The result of this evaluation would be fed into an adapter subclass of the WhereExpression abstract class.
There's more to this implementation, in that the WhereExpression class also has the getSubstitutions() method, an override for which is generated with certain client-side queries. IIRC, this would be emitted if, for instance, the can-find's where clause in the q1 query had its own query substitution parameters.

That's the idea I'm working with. The problem is that the evaluate() method can be called multiple times, and each time, the result must be a new object. We cannot store that value in the adapter. The adapter must be dynamic, to rebuild the result from the 'environment' at the time of the respective call. This will happen at a different time that the interpretation of the 'main' execute and building of the query. So a reference to the interpreter is kept in the adapter. I don't think that this event will collide with our cache, otherwise some complex synchronization will have to be implemented.

Another issue at this moment, if we decide to support UDFs (P4GL doesn't, see my previous note-53), how will they be accessed. It could go naturally is the class that declare it is imported statically.

Regarding the user-defined functions. I was thinking too ahead. At this moment the ProgressParser cannot handle UDF-s because they are not 'visible' in the parsed string ("WHERE udf(book-id) eq udf(100)"):

line 1:23: unexpected token: udf
    at com.goldencode.p2j.uast.ProgressParser.lvalue(ProgressParser.java:12680)
    [...]
    at com.goldencode.p2j.uast.ProgressParser.statement(ProgressParser.java:5712)
    at com.goldencode.p2j.uast.ProgressParser.single_block(ProgressParser.java:4568)
    at com.goldencode.p2j.uast.ProgressParser.block(ProgressParser.java:4327)
    at com.goldencode.p2j.uast.ProgressParser.external_proc(ProgressParser.java:4253)
    at com.goldencode.p2j.persist.DynamicQueryHelper.generateJavaTree(DynamicQueryHelper.java:328)
    at com.goldencode.p2j.persist.DynamicQueryHelper.parse(DynamicQueryHelper.java:180)
    at com.goldencode.p2j.persist.DynamicQueryHelper.parseFindQuery(DynamicQueryHelper.java:674)
    [...]

I guess this is the case for P4GL too, because of the compile error when a bad function name is encountered in a static open query is similar to the runtime warning issued when encountering any UDF in dynamic mode.

If we cannot come up with a single test case which would result in client-side where clause processing (all so far seem disallowed in the Progress environment), I hesitate to spend any more time working on this feature. How would we test the implementation (if it's even necessary)?

Stanislav, I don't fully understand the following comment, from DynamicQueryHelper:763:

// Dynamic buffers have unique DMO aliases, so "some-buffer" name may be converted to the
// alias like "someTable__1", while aliases in predicate will be converted using standard
// P2J rules, which will result to the converted alias "someTable". So below we explicitly
// specify converted DMO alias like "someTable__1" for the conversion rules using
// "force_dmo_alias" annotation.

You are the original author of this comment AFAICT. Maybe you can explain it to me. I'm seeing dynamic queries which are otherwise identical, except for the DMO alias suffix, like __1, __2, etc. We have implemented a cache for dynamic queries, based in part on the buffer name. There are cases where the buffer type and predicate of separate queries are identical, but the buffer names differ by this suffix only. It seems like these are basically the same queries in Progress, and this differentiating suffix is applied by P2J. As a result, the queries are each deemed to be unique by the caching logic, so each time we encounter one, a cache miss occurs. Thus, we add many copies of what is otherwise the same query to the cache.

Any insight into the purpose of this differentiation would be appreciated.

Eric, are you basically asking why suffixes are added to the names of dynamic buffers?

Stanislav Lomany wrote:

Eric, are you basically asking why suffixes are added to the names of dynamic buffers?

Yes, why and where/how? I want to figure out if this is still relevant/necessary with the new implementation.

IIRC the point is that there can be multiple dynamic buffers with the same name in the same scope and some P2J structures use buffer name as a key.

Eric Faulhaber wrote:

If we cannot come up with a single test case which would result in client-side where clause processing (all so far seem disallowed in the Progress environment), I hesitate to spend any more time working on this feature. How would we test the implementation (if it's even necessary)?

I finally have found such cases (thanks to Constantin for help):

DEFINE TEMP-TABLE tt
   FIELD f1 AS INTEGER.

hQuery:SET-BUFFERS(BUFFER book:HANDLE, BUFFER tt:HANDLE).

hQuery:QUERY-PREPARE("FOR EACH book, FIRST tt WHERE BUFFER tt:NAME = book.isbn").

hQuery:QUERY-PREPARE("FOR EACH book, EACH tt WHERE BUFFER tt::f1 = book.book-id").

These are probably not the single cases, but they are the proof that we have to handle the WhereExpression in interpreter. I am still looking for a test case that need overriding the getSubstitutions() method.

• the equality test in the where clause is never yes (for example: WHERE tt.f2 = buffer book:name, even if tt.f2 is a char initialized with "book")
• WHERE BUFFER tt::f1 <> book.book-id will result in a ** Incompatible data types in expression or assignment. (223)

Eric,
I almost finished implementing support for 'extending' and interpreting the client-side WhereExpression but I this moment (among other issues) I encountered the following error in the dynamic conversion:
Consider the query to prepare: "FOR EACH book, EACH tt WHERE BUFFER tt:NAME = book.isbn".
The converted JAST dumped to console looks like:

OPEN QUERY DynGenQuery FOR EACH book, EACH tt WHERE BUFFER tt:NAME = book.isbn
compilation unit [COMPILE_UNIT] @0:0
   com.goldencode.p2j.util.* [KW_IMPORT] @0:0
   com.goldencode.p2j.util.BlockManager.* [STATIC_IMPORT] @0:0
   DynGenQuery [KW_CLASS] @0:0
       [CS_INSTANCE_VARS] @0:0
         = [ASSIGN] @0:0
            WhereExpression [REFERENCE_DEF] @0:0
            WhereExpression [ANON_CTOR] @0:0
                [CS_INSTANCE_METHODS] @0:0
                  evaluate [METHOD_DEF] @0:0
                      [LPARENS] @0:0
                        BaseDataType[] [REFERENCE_DEF] @0:0
                     block [BLOCK] @0:0
                         [KW_RETURN] @0:0
                           isEqual [STATIC_METHOD_CALL] @0:0
                              tt [REFERENCE] @0:0
                              getIsbn [METHOD_CALL] @0:0
                                 book [REFERENCE] @0:0
         CompoundQuery [REFERENCE_DEF] @0:0
       [CS_INSTANCE_METHODS] @0:0
         execute [METHOD_DEF] @0:0
         [...]

Is it possible that the simplified runtime conversion is missing processing step? Is this something related to temp-tables maybe ?

At a glance, yes, that looks wrong. Have you converted the static form of the same query? This will tell you if the problem is related to runtime conversion specifically, as opposed to a general conversion problem.

Eric Faulhaber wrote:

At a glance, yes, that looks wrong. Have you converted the static form of the same query? This will tell you if the problem is related to runtime conversion specifically, as opposed to a general conversion problem.

Yes, the statically converted query looks fine:

   WhereExpression whereExpr0 = new WhereExpression()
   {
      public logical evaluate(final BaseDataType[] args)
      {
         return isEqual(tt.name(), book.getIsbn());
      }
   };

The problem reported in note-62 is fixed by adding the methods_attributes.rules to runtime conversion. With this occasion I removed the brewing rules that are not used any more.

I discovered yet another case of bad conversion. Constructs like

OPEN QUERY q1 FOR EACH book, EACH tt WHERE book.book-id < SESSION:TIMEZONE AND NOT buffer tt:AVAILABLE.

   WhereExpression whereExpr0 = new WhereExpression() {
      public logical evaluate(final BaseDataType[] args) {
         return isLessThan(book.getBookId(), SessionUtils.getSessionTimeZone());
      }
   };
   query2.addComponent(new AdaptiveQuery(tt, (String) null, whereExpr0, "tt.id asc", new Object[] {
               and(isLessThan(book.getBookId(), SessionUtils.getSessionTimeZone()), new LogicalExpression() {
                  public logical execute() {
                     return not(tt.available());
                  }
               })
            }));

Code review 0210a:

The version of DynamicQueryHelper in the update shows no difference from the latest version in bzr. Was this intentional? The rest looks good to me.

• Improved preparePredicate() for OPEN QUERY predicates to
  - allow one level nested quotes
  - allow handle methods as valid constructs: buffer book:name
  - allow dereference operator (:: punctuation): buffer book::isbn
  - drop garbage after the COLON (:) or DOT (.) when they are terminators (followed by some white space).
  See #2401.
• Added support for dynamic calling of methods from super classes
• Fixed dereference operator when applied directly to Buffer.
• Prevented a NPE when listeners is empty in ChangeBroker.
• Added support for method and dereference in dynamic conversions.

Code review 0218a:

Where did the need to improve the predicate prep come from? What is the rationale behind the cutoff point at which this level of sanity checking should end and the rest be left up to the Progress parser?

If you remove the brew-related rules from the "runtime-rules" pattern set in build.xml, isn't that going to disable some of the debug output capability in DynamicQueryHelper? For example, I don't run the P2J server from within eclipse; I use the jar files. This change will disable that debug capability for me.

In core_conversion.xml, please consolidate your header entries into one. There generally should be one header entry per check-in of a file.

Everything else looks good to me.

If the above are the only changes you make, please go ahead with a conversion regression test (since some of the changed files are used in static conversion) and commit the update when it passes. The change to ChangeBroker is in a method that should never be invoked by the regression test environment, and the changes to the uast classes are additive only, so runtime testing is not necessary.

Eric Faulhaber wrote:

Where did the need to improve the predicate prep come from? What is the rationale behind the cutoff point at which this level of sanity checking should end and the rest be left up to the Progress parser?

• it was parsing wrongly the string literals (a literal started with first of ' or " and ended at second ' or " so a mix of quotes was validated), leading into truncation of queries like: "for each book where isbn = 'abc~". abc'" to "for each book where isbn = 'abc~""
• it wasn't allowing the ":" and "::" punctuation at all. Predicates like "for each book where timezone(book.timestamp) eq session:timezone" were incorrectly reported as being syntactically incorrect.
  Normally the ., :, and :: operators are followed (without any blank in between) by a field / method / dereference. At the moment of this scan, we cannot tell if the right-hand of the punctuation is correct, we need the true ProgressParser but it will act at a later stage. However, if a blank character (one of \r\n\t ) occurs after the . or : Progress just stop parsing the string, ignoring the rest of the characters.

If you remove the brew-related rules from the "runtime-rules" pattern set in build.xml, isn't that going to disable some of the debug output capability in DynamicQueryHelper? For example, I don't run the P2J server from within eclipse; I use the jar files. This change will disable that debug capability for me.

This is true. But the JASTs are still available. At least that's all I use. This is the reason also for the small additions in Aast / DumpTree. I think it's a good tool for debugging: to optionally see the annotations for AST nodes. Nevertheless, seeing the true java code is important, too, so I will add those files back to jar.

If the above are the only changes you make, please go ahead with a conversion regression test (since some of the changed files are used in static conversion) and commit the update when it passes. The change to ChangeBroker is in a method that should never be invoked by the regression test environment, and the changes to the uast classes are additive only, so runtime testing is not necessary.

OK.

Ovidiu Maxiniuc wrote:

The generated code is unchanged for majic. The attached update passed the conversion test.

Looks good. Please commit and distribute.

I found a test case that fails to convert properly.

DEF TEMP-TABLE tt0 
   FIELD f1 AS INT. 
DEFINE VARIABLE t3 AS HANDLE.
CREATE BUFFER t3 FOR TABLE "tt0" BUFFER-NAME "t3h".
t3:FIND-FIRST().

At runtime the last line generate the following in-memory dynamic code:

DEFINE BUFFER t3h FOR TEMP-TABLE tt0.
FIND FIRST t3h  SHARE-LOCK.

It will then be processed to JAST that, before applying post-processing step, look like this (annotation included):

compilation unit [COMPILE_UNIT] @0:0
   com.goldencode.p2j.persist.dynquery [KW_PACKAGE] @0:0
   com.goldencode.p2j.util.* [KW_IMPORT] @0:0
   com.goldencode.p2j.util.BlockManager.* [STATIC_IMPORT] @0:0
   DynGenQuery [KW_CLASS] @0:0
   (access=public, pkgname=com.goldencode.p2j.persist.dynquery)
       [CS_CONSTANTS] @0:0
       [CS_STATIC_VARS] @0:0
       [CS_STATIC_INITS] @0:0
       [CS_INSTANCE_VARS] @0:0
         = [ASSIGN] @0:0
            TempRecord1.Buf [REFERENCE_DEF] @0:0
            (name=t3h)
            TemporaryBuffer.define [STATIC_METHOD_CALL] @0:0
               TempRecord1.Buf.class [REFERENCE] @0:0
               t3h [STRING] @0:0
               tt0 [STRING] @0:0
                [BOOL_FALSE] @0:0
         = [ASSIGN] @0:0
            TempRecord1.Buf [REFERENCE_DEF] @0:0
            (name=t3hBuf2)
            TemporaryBuffer.define [STATIC_METHOD_CALL] @0:0
               t3h [REFERENCE] @0:0
               t3hBuf2 [STRING] @0:0
               t3h [STRING] @0:0
       [CS_CONSTRUCTORS] @0:0
       [CS_STATIC_METHODS] @0:0
       [CS_INSTANCE_METHODS] @0:0
         execute [METHOD_DEF] @0:0
         (peerid=25769803777)
             [BLOCK] @0:0
               externalProcedure [STATIC_METHOD_CALL] @0:0
                  Block [ANON_CTOR] @0:0
                      [CS_INSTANCE_VARS] @0:0
                      [CS_INSTANCE_METHODS] @0:0
                        body [METHOD_DEF] @0:0
                        (access=public, rettype=void)
                           block [BLOCK] @0:0
                           (peerid=25769803777)
                              buffer_anchor [BOGUS] @0:0
                                 RecordBuffer.openScope [STATIC_METHOD_CALL] @0:0
                                 (peerid=25769803807)
                                    t3hBuf2 [REFERENCE] @0:0
                                    t3h [REFERENCE] @0:0
                              first [METHOD_CALL] @0:0
                              (peerid=25769803791)
                                 FindQuery [CONSTRUCTOR] @0:0
                                 (peerid=25769803797, runtime-class=class com.goldencode.p2j.persist.FindQuery)
                                    t3hBuf2 [REFERENCE] @0:0
                                    (peerid=25769803798)
                                    String [CAST] @0:0
                                        [NULL_LITERAL] @0:0
                                     [NULL_LITERAL] @0:0
                                    t3hBuf2.id asc [STRING] @0:0
                                    LockType.SHARE [REFERENCE] @0:0
                                    (runtime-value=SHARE)
       [CS_INNER_CLASSES] @0:0
   com.goldencode.p2j.persist.* [KW_IMPORT] @0:0
   com.goldencode.testcases.dmo._temp.* [KW_IMPORT] @0:0
   com.goldencode.p2j.persist.lock.* [KW_IMPORT] @0:0

Note that there are two TempRecord1.Buf defined: t3h and t3hBuf2. Unfortunately, the FindQuery statement will use the second one, the original t3h remains unused. The problem is that, when interpreting this JAST, the t3hBuf2 is unknown, the caller only supplies informations about the original t3 / t3h buffer: t3h = com.goldencode.p2j.persist.$__Proxy1@727e748c. Of course, the dynamic query fails with following messages:

(com.goldencode.p2j.persist.DynamicQueryHelper:SEVERE) Failed to interpret JAST for QueryCacheKey{query='', bufferNames=[t3h], bufferDMOs=[class com.goldencode.testcases.dmo._temp.impl.TempRecord1Impl]}

com.goldencode.p2j.persist.RuntimeJastInterpreter$InterpreterException: No REFERENCE named (t3hBuf2)

(ErrorManager:SEVERE) {00000001:00000008:bogus} Invalid WHERE clause in FIND method for buffer tt1. (10041)

Code review 0306a:

Looks good. I'll run tests with this update and let you know how it goes.

Eric Faulhaber wrote:

Code review 0306a:

Looks good. I'll run tests with this update and let you know how it goes.

Sorry, forgot to get back to you. Testing went well with this update, please commit and distribute.

Code review 0318a:

Looks good, noticed one typo on line 1121: "...some king of expression..."

What testing have you done with this update applied?

Eric Faulhaber wrote:

Code review 0318a:

Looks good, noticed one typo on line 1121: "...some king of expression..."

Thanks!

What testing have you done with this update applied?

See #2535, my note 10. There are IntegerExpression and DateExpression.
The integer variant occurs from first note (at first I thought that was the issue in #2535). But then I reproduced the bug in static mode. With the update applied, the IntegerExpression is not generated any more for that testcase. But, to be sure we don't get held in the future, I think it's better to add support for int and date expressions, at least.

Ovidiu Maxiniuc wrote:

What testing have you done with this update applied?

See #2535, my note 10. There are IntegerExpression and DateExpression.
The integer variant occurs from first note (at first I thought that was the issue in #2535). But then I reproduced the bug in static mode. With the update applied, the IntegerExpression is not generated any more for that testcase. But, to be sure we don't get held in the future, I think it's better to add support for int and date expressions, at least.

Go ahead and commit and distribute this update, but we also should add support for all the data wrapper types in a follow-up update.

The update om_upd20150318a was committed to bzr as revno 10824 and distributed by mail.

Is there any work left to do on the initial implementation, or any known problems outstanding? If not, I will close this task, and we can handle any new bugs as separate issues.

Eric Faulhaber wrote:

Is there any work left to do on the initial implementation, or any known problems outstanding? If not, I will close this task, and we can handle any new bugs as separate issues.

The RuntimeJastInterpreter still lacks the support for some client-side where __Expression-s (currently only Logical, Integer and Date were detected as required and supported). We don't know if the other datatypes are really needed.

Some improvements in the caching can be implemented. The current 'dumb' solution will duplicate the efforts for queries like "FIND FIRST book WHERE isbn EQ 'isbn1'" and "FIND FIRST book WHERE isbn EQ 'isbn2'". They are the same in fact, they only differ by a string literal that can be detected and extracted as a SQL parameter.

Code review 0512a:

The change looks good. If it fixes your problem locally, please commit and distribute, since regression testing won't cover this functionality.

Eric Faulhaber wrote:

Code review 0512a:

The change looks good. If it fixes your problem locally, please commit and distribute, since regression testing won't cover this functionality.

Committed revision 10859. Distributed my mail.

Please investigate the following error from the P2J server.log:

com.goldencode.p2j.persist.RuntimeJastInterpreter$InterpreterException: Failed to execute static method
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.callStaticMethod(RuntimeJastInterpreter.java:844)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:965)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.execMethod(RuntimeJastInterpreter.java:388)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.access$000(RuntimeJastInterpreter.java:73)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter$WhereExpressionAdapter.evaluate(RuntimeJastInterpreter.java:1457)
        at com.goldencode.p2j.persist.WhereExpression.evaluate(WhereExpression.java:206)
        at com.goldencode.p2j.persist.RandomAccessQuery.execute(RandomAccessQuery.java:2578)
        at com.goldencode.p2j.persist.RandomAccessQuery.first(RandomAccessQuery.java:1167)
        at com.goldencode.p2j.persist.RandomAccessQuery.first(RandomAccessQuery.java:1113)
        at com.goldencode.p2j.persist.CompoundQuery.processComponent(CompoundQuery.java:2228)
        at com.goldencode.p2j.persist.CompoundQuery.retrieveImpl(CompoundQuery.java:1975)
        at com.goldencode.p2j.persist.CompoundQuery.retrieve(CompoundQuery.java:1552)
        at com.goldencode.p2j.persist.CompoundQuery.retrieve(CompoundQuery.java:1438)
        at com.goldencode.p2j.persist.CompoundQuery.next(CompoundQuery.java:877)
        at com.goldencode.p2j.persist.CompoundQuery.next(CompoundQuery.java:823)
        at com.goldencode.p2j.persist.AbstractQuery.wrapNext(AbstractQuery.java:2199)
        at com.goldencode.p2j.persist.AbstractQuery.getNext(AbstractQuery.java:1242)
        at com.goldencode.p2j.persist.QueryWrapper.getNext(QueryWrapper.java:3668)
        ...

I was able to duplicate the issue. One occurrence is in test[Entity=|+TNCY_TENANT| TestType=|SEARCH| TestSubType=|SEARCHEABLE_ATTRIBUTE| xmlSuffix=|26|].
There is an incorrect JAST tree for a LogicalExpression implementation. The execute looks like this:

execute [METHOD_DEF] @0:0 (access=public, rettype=logical)
   block [BLOCK] @0:0
       [KW_RETURN] @0:0
         isLessThan [STATIC_METHOD_CALL] @0:0
            getTermDat [METHOD_CALL] @0:0(runtime-class=class com.goldencode.p2j.persist.$__Proxy72, runtime-method=public com.goldencode.p2j.util.date com.goldencode.p2j.persist.$__Proxy72.getTermDat())
               tncy [REFERENCE] @0:0

The P4GL converted query is:

OPEN QUERY DynGenQuery  FOR EACH prhst , FIRST tncy WHERE tncy.prhst-occ-num = prhst.occ-num AND ((tncy.term-dat = ? and prhst.end-dte < TODAY) OR tncy.term-dat < TODAY) NO-LOCK

package com.goldencode.p2j.persist.dynquery;

import com.goldencode.p2j.util.*;
import com.goldencode.p2j.persist.*;
import com.something.server.dmo.<db_name>.*;
import com.goldencode.p2j.persist.lock.*;

import static com.goldencode.p2j.util.BlockManager.*;
import static com.goldencode.p2j.util.CompareOps.*;
import static com.goldencode.p2j.util.logical.*;

public class DynGenQuery
{
   Tncy.Buf tncy = RecordBuffer.define(Tncy.Buf.class, "<db_name>", "tncy", "tncy");

   Prhst.Buf prhst = RecordBuffer.define(Prhst.Buf.class, "<db_name>", "prhst", "prhst");

   WhereExpression whereExpr0 = new WhereExpression()
   {
      public Object[] getSubstitutions()
      {
         return new Object[]
         {
            new DateExpression()
            {
                              public date execute()
               {
                  return date.today();
               }
            }
         };
      }

      public logical evaluate(final BaseDataType[] args)
      {
         return or(and(isUnknown(tncy.getTermDat()), new LogicalExpression()
         {
            public logical execute()
            {
*               return isLessThan(prhst.getEndDte());
            }
         }), new LogicalExpression()
         {
            public logical execute()
            {
*               return isLessThan(tncy.getTermDat());
            }
         });
      }
   };

   public void execute()
   {
      externalProcedure(new Block()
      {
         CompoundQuery query0 = null;

         public void body()
         {
            RecordBuffer.openScope(tncy, prhst);
            query0 = new CompoundQuery(true, false);
            query0.addComponent(new AdaptiveQuery(prhst, (String) null, null, "prhst.prSeqNo asc, prhst.seqNo asc, prhst.perNum asc", "WHOLE-INDEX,idx-1"));
            RecordBuffer.prepare(tncy);
            query0.addComponent(new RandomAccessQuery(tncy, "tncy.prhstOccNum = ?", whereExpr0, "tncy.prhstOccNum asc", "occ-num", new Object[]
            {
               new FieldReference(prhst, "occNum")
            }, LockType.NONE), QueryConstants.FIRST);
            query0.open();
         }
      });
   }
}

I will create a static testcase and go further from there. I don't think that this issue is related to this task.

LE: GES removed the customer's name and customer's database name from this entry.

Code review 0610a:

Looks good. Assuming your local testing confirms it fixes the problem, please run conversion regression testing only (the changes to the conversion/build are very minor, but just to be safe...).

Note there are some commented out settings added (used while testing, perhaps?) in the aspectj target of build.xml. Please remove, then commit/distribute when testing passes.

(Note: there are a number of fixes in the later months that require new rules files to be added to runtime bundle. The size of them is not a problem, but processing them at runtime could add a performance hit because of the extra tests (even if nothing is changes in the AST trees, the rather consuming functions are evaluated). I wonder if these required pieces of TRPL code could be extracted in separate files somehow?)

Can you identify specific areas/tests/functions that concern you? Have you profiled/measured to confirm your concerns, or is it more of a gut feeling based on reviewing the code? TRPL code in general is pretty fast once it has been compiled, though if most of it is unnecessary, clearly it will be faster to avoid running that code at all.

I tested the 0610a update with the full set of search unit tests. I can confirm that it fixed the problematic test and did not regress others.

Please commit and distribute 0611a. Thanks.

Ovidiu, I need some help extending RuntimeJastInterpreter to deal with a new way of handling query substitution parameters and to support a new Java token type: LAMBDA.

In order to fix some outstanding bugs which require the deferred evaluation of some query substitution parameters, I implemented the conversion of those query substitution parameters as lambda expressions. A typical case would look like this:

new FindQuery(foo, "foo.bar = ?", ..., new Object[]
{
   (P2JQuery.Parameter) () -> toUpperCase(bar.getFoo())
}, LockType.NONE).first();

new FindQuery(foo, "foo.bar = ?", null, "foo.bar asc", new Object[]
{
   (P2JQuery.Parameter) () -> userDefinedFunction(bar.getFoo())
}, LockType.NONE).first();

...
FindQuery [CONSTRUCTOR] @0:0
   foo [REFERENCE] @0:0
   foo.bar = ?) [STRING] @0:0
    [NULL_LITERAL] @0:0
   foo.bar asc [STRING] @0:0
   idx-1 [STRING] @0:0
    [EXPRESSION] @0:0
      new Object [REFERENCE_DEF] @0:0
       [INITIALIZER] @0:0
         P2JQuery.Parameter [CAST] @0:0
             [LAMBDA] @0:0
                [LPARENS] @0:0
               character.toUpperCase [STATIC_METHOD_CALL] @0:0
                  getFoo [METHOD_CALL] @0:0
                     bar [REFERENCE] @0:0
   LockType.NONE [REFERENCE] @0:0
QueryConstants.FIRST [REFERENCE] @0:0
...

com.goldencode.p2j.persist.RuntimeJastInterpreter$InterpreterException: Unknown expression type
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:1135)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:936)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:996)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:951)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.collectParameters(RuntimeJastInterpreter.java:622)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.callCtor(RuntimeJastInterpreter.java:552)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.evalExpression(RuntimeJastInterpreter.java:924)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.collectParameters(RuntimeJastInterpreter.java:622)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.callMethod(RuntimeJastInterpreter.java:702)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.execMethod(RuntimeJastInterpreter.java:433)
        at com.goldencode.p2j.persist.RuntimeJastInterpreter.interpret(RuntimeJastInterpreter.java:230)
        at com.goldencode.p2j.persist.DynamicQueryHelper.parse(DynamicQueryHelper.java:241)
        at com.goldencode.p2j.persist.DynamicQueryHelper.parseQuery(DynamicQueryHelper.java:602)
        at com.goldencode.p2j.persist.QueryWrapper.prepare(QueryWrapper.java:3437)
        ...

The updates are in branch 2650a/10936, which is based on trunk 10930. Could you please take a look and add this support? When you have a fix, please check it into that branch.

You will need to build P2J with Java 8, so you'll have to make that your default environment (java and javac) temporarily. Don't worry about getting the native code working -- it will build, but it may not run appserver properly. That isn't necessary for this work and it caused problems for me, so please don't spend any time on it. I've gotten around the issues, but I haven't documented how yet. Thanks.

Ovidiu Maxiniuc wrote:

Eric Faulhaber wrote:

Is there any work left to do on the initial implementation, or any known problems outstanding? If not, I will close this task, and we can handle any new bugs as separate issues.

The RuntimeJastInterpreter still lacks the support for some client-side where __Expression-s (currently only Logical, Integer and Date were detected as required and supported). We don't know if the other datatypes are really needed.

How much effort do you estimate is necessary to implement the remaining datatypes?

Some improvements in the caching can be implemented. The current 'dumb' solution will duplicate the efforts for queries like "FIND FIRST book WHERE isbn EQ 'isbn1'" and "FIND FIRST book WHERE isbn EQ 'isbn2'". They are the same in fact, they only differ by a string literal that can be detected and extracted as a SQL parameter.

This can be moved into a separate issue as an idea for a performance or efficiency enhancement. It is not a functional requirement.

Eric Faulhaber wrote:

Ovidiu Maxiniuc wrote:

The RuntimeJastInterpreter still lacks the support for some client-side where __Expression-s (currently only Logical, Integer and Date were detected as required and supported). We don't know if the other datatypes are really needed.

How much effort do you estimate is necessary to implement the remaining datatypes?

This is a matter of minutes, to duplicate classes/methods and replacing the new datatypes.

Some improvements in the caching can be implemented. The current 'dumb' solution will duplicate the efforts for queries like "FIND FIRST book WHERE isbn EQ 'isbn1'" and "FIND FIRST book WHERE isbn EQ 'isbn2'". They are the same in fact, they only differ by a string literal that can be detected and extracted as a SQL parameter.

This can be moved into a separate issue as an idea for a performance or efficiency enhancement. It is not a functional requirement.

OK, I will create a new task to track this as a feature/enhancement.

Ovidiu Maxiniuc wrote:

Eric Faulhaber wrote:

Ovidiu Maxiniuc wrote:

The RuntimeJastInterpreter still lacks the support for some client-side where __Expression-s (currently only Logical, Integer and Date were detected as required and supported). We don't know if the other datatypes are really needed.

How much effort do you estimate is necessary to implement the remaining datatypes?

This is a matter of minutes, to duplicate classes/methods and replacing the new datatypes.

OK, please add this to your next task branch (after 2975a). Better we spend the minutes now than potentially hours tracking down a bug from this later. Then we can close this task.

Support for interpreting remaining datatypes __Expressions in RuntimeJastInterpreter was added as rev. 10969 in task branch 2975a.

Ovidiu Maxiniuc wrote:

Support for interpreting remaining datatypes __Expressions in RuntimeJastInterpreter was added as rev. 10969 in task branch 2975a.

Did you find that task branch 2975a causes converted code changes in Majic or the customer server project, such that we now need to do runtime regression testing? I would have expected not, since otherwise we should have seen compile failures in those projects before the 2975a/10968 fix.

If there were no conversion differences in Majic or customer server project, please back out the __Expression changes (i.e., rev 10969) from 2975a and place those changes instead into the next task branch you create for working on a different issue, after #2975. Don't uncommit the change, but please put the old versions of the runtime classes back as they were and/or remove any new classes, then commit that as 2975a/10970 (should be the same as 2975a/10968).

2975a should only contain the conversion changes to fix the regression covered by #2975. Otherwise, we have to do runtime regression testing for 2975a. Also, I don't want to put this in a dedicated task branch, which would cause a dedicated round of regression testing just for this change.

Thanks.

Eric, I reverted 2975a back to rev 10968 = 10970. Sorry, I misread your note 100.

The updated was committed to task branch 2794a as revision 10970.

Ovidiu Maxiniuc wrote:

The updated was committed to task branch 2794a as revision 10970.

The changes are good, thanks.

I think we'll want to parameterize the expression class hierarchy at some point, since there's a lot of duplication (unless there's some functional or performance concern in doing so). The base classes (and conversion) originally were written before Java had generics. I know this wasn't feasible here, since the classes you inherit from are emitted in converted code, but we'll want to change that with a future code improvement.

Eric Faulhaber wrote:

I think we'll want to parameterize the expression class hierarchy at some point, since there's a lot of duplication (unless there's some functional or performance concern in doing so). The base classes (and conversion) originally were written before Java had generics. I know this wasn't feasible here, since the classes you inherit from are emitted in converted code, but we'll want to change that with a future code improvement.

This is true, I also thought about doing it now, but there are some architectural issues here. All the classes involved share the same ancestor (Resolvable) but the class hierarchy split too early into specialized classes. Once the generics are added to base/intermediary classes we will replace all these utilities with a generic one.