FWD » Core Development » Base Language

• added .rules changes and in various files (like ExpressionConversion etc) for int64 support (more changes addes as exceptions to general rules are discovered)
• simple samples are converted and run fine
• I am building a complex testcase that will include integer, int64, decimal types (and later chars and datetime datatypes).

Another error I found. The following sample code extracted from OpenEdge® Development: ABL Reference (page 55) fails to compile on OpenEdge windev01:

DEFINE VARIABLE fx AS DECIMAL NO-UNDO LABEL "X".
DEFINE VARIABLE fabs-x AS DECIMAL NO-UNDO LABEL "ABS(X)".
REPEAT:
  SET fx.
  IF fx < 0 THEN
    fabs-x = -fx.
  ELSE
    fabs-x = fx.
  DISPLAY fabs-x.
END.

with the following compiler message:

** Unknown Field or Variable name - -fx. (201)
**  Could not understand line 6. (196)

The P2J implementation does not have this defect for both integer and newer int64.

• date = the base class (relative to this issue, of course it will inherit BaseDataType), having year, month and day.
• datetime = extends date, new fields: hour, min, sec, msec.
• datetime-tz = extends datetime, new field: tz.
  The date functions would work directly, no overloading of methods would be necessary.

This plan sounds good.

My only feedback is that it is probably best to encode the time value as a number of millis since midnight, instead of separate values for hour, min, sec and msec. All of these can be easily derived from the single value. We do the same thing with the main date class, where we derive year, month and day off the julian day number. The advantage of this is that it may decrease the overall size of the data stored for this class, which may help keep memory requirements down since there could be millions of these instantiated on the server side at once.

I think it is best to keep this value separate from the julian day number in the main date class. Although it is tempting to potentially use just a single long to encode the date and time values (as millis since the epoch), I think it has too much potential to break the date class, which is already complex enough. Keeping the implementation close to how the 4GL needs the result seems to be the best plan.

Go ahead with this approach.

The highest priorities for the DATETIME implementation:

NOW
MTIME
ADD-INTERVAL
DATETIME

SESSION:TIME-SOURCE (stub this only, no implementation)

all operators that can operate on DATETIME

Lower priority:

INTERVAL
ISO-DATE
SESSION:YEAR-OFFSET

Additional requirements:

• define a DATETIME variable
• define a DATETIME field in a temp-table
• assign a DATETIME value from another DATETIME instance
• assign unknown value into a DATETIME
• formatting a DATETIME into a character using STRING with a format phrase (e.g., "99/99/99 HH:MM", "99/99/9999 HH:MM")

DATETIME-TZ is not needed at this time; implementation of this class should be minimal (mostly as a placeholder for future development).

Code review 20130117a:

• Nice functional addition! A bit more to go for Milestone 4 (e.g., defining DATETIME in a temp-table).
• ExpressionConversionWorker: was debugBreak() meant to be a permanent addition to the API?
• date: this class now deals with time values instead of just with dates. I notice your comment about OOP standards where date code tests if an object is an instance of the datetime subclass, which I agree with. But it seems the design problem is broader than just the implementation within that method, since many of the new methods are about DATETIME and DATETIMETZ concepts, and as such don't seem to belong in the parent class. Why is this necessary?
• builtin_functions.rules: same design questions as previous bullet, since this is where the converted calls to the new date methods are generated.
• operators.rules: rogue blank line at the bottom of the file header.

Please note that some of the files in this update conflict with changes checked into bazaar today, and will need to be merged.

Ovidiu: I am going to make the final modifications on the latest code you have posted. I know you are busy with other tasks and we need to get these changes in ASAP.

If you have anything modified that was not yet posted, please post it here.

Note 19 above discusses datetime and datetime-tz support. But the code updates also have a significant number of added features for int64 as well. The following are known to be things still to do for int64:

1. All of the new 4GL features that compile differently in 10.x should be implemented in 2 versions (the old integer return value and the new int64 return value). For example, the CURRENT-VALUE should have 2 versions (SequenceManager.currentValue() and SequenceManager.currentValue64() or something like that). Then we would have an optional conversion setting (in p2j.cfg.xml) that specifies 64-bit versions or not. This would be the equivalent to the -noint64 startup parameter in the 4GL.

2. All operators and built-in functions need to be checked for properly handling int64 in their operations. See the Progress document entitled "ABL Data Types in OpenEdge Release 10" which describes the new types (and their issues) in reasonable detail.

3. Many P2J runtime APIs that have versions that explicitly take an integer or decimal need to be examined for potentially moving to a NumberType, otherwise int64 cannot be passed. AND we don't want to explode the signatures unless there is a very good reason.

4. Should integer and int64 have a common base class that inherits from NumberType?

5. Does ui/client/format/NumberFormat.java properly support int64? Enhance it if needed.

6. Search int64.java for TODOs.

I inadvertently missed a change in my update zip. The following diff is from com/goldencode/p2j/persist/type/DateUserType.java:

bzr diff src/com/goldencode/p2j/persist/type/DateUserType.java
=== modified file 'src/com/goldencode/p2j/persist/type/DateUserType.java'
--- src/com/goldencode/p2j/persist/type/DateUserType.java       2009-03-31 17:18:21 +0000
+++ src/com/goldencode/p2j/persist/type/DateUserType.java       2013-01-26 22:13:41 +0000
@@ -155,12 +155,11 @@
    throws SQLException
    {
       // Need a java.sql.Date;  date class deals in java.util.Date.  The Date
-      // must be constructed using the JVM's default timezone, to ensure there
-      // is no drift to the previous date, due to timezone or Daylight Savings
-      // offsets.
+      // is constructed using the same timezone used for the datetime to
+      // ensure there is no drift to the previous date, due to timezone or
+      // daylight savings offsets.
       date d = (date) value;
-      long localMillis = d.dateValue(null).getTime();
-      Date sqlDate = new Date(localMillis);
+      Date sqlDate = new Date(d.dateValue().getTime());

       ps.setDate(index, sqlDate);
    }

After applying this, the bindNonNull() method looks like this:

   /**
    * Set a Date value into the given prepared statement,
    * based upon the date instance value.
    *
    * @param   ps
    *          Prepared statement into which we will set our data.
    * @param   value
    *          An integer instance.
    * @param   index
    *          Index in the statement at which to write the data.
    *
    * @throws  SQLException
    *          if an error occurs setting data into the statement.
    */
   protected void bindNonNull(PreparedStatement ps,
                              BaseDataType value,
                              int index)
   throws SQLException
   {
      // Need a java.sql.Date;  date class deals in java.util.Date.  The Date
      // is constructed using the same timezone used for the datetime to
      // ensure there is no drift to the previous date, due to timezone or
      // daylight savings offsets.
      date d = (date) value;
      Date sqlDate = new Date(d.dateValue().getTime());

      ps.setDate(index, sqlDate);
   }

I believe this missing change is the true cause of the regressions in staging. It certainly explains how date values could be different in the database than in the JVM.

After I neutered the timezone changes in date.java, everything did end up passing testing. So this change will have to be added when the timezone processing is re-introduced in date.java.

I am checking in my code to bzr and distributing the update (as is, without the DateUserType update since that has not passed testing yet).

Ovidiu: your next priority should be completing the runtime work on this task. The first priority is to get the int64 work done. This is simpler and smaller than the datetime/datetime-tz work. Once int64 is there, then work the date stuff.

Please note that I have made many changes in this area since you last worked there. I did try to leave behind TODO markers in the code. Please review those and make a list here of the work to be done.

In addition, please read the document entitled "ABL Data Types in OpenEdge Release 10" from Progress. It has some errors, but it generally does give a good idea of the support that is needed. Please augment the list of todos using knowledge gained from this document. Once the full list of features/changes is documented here, we will discuss any implementation issues as needed.

Code Feedback:

1. The instantiateUnknownRaw() method should be positioned after the constructors in the raw.java.

2. NumberType.getBits() should not return this when isUnknown() is true. We always return a new instance when we return unknown, otherwise a caller might modify the state of the instance thinking it was safe, resulting in an unexpected change to the original. this.instantiateUnknown() will do the job.

3. I would like the NumberType.get/setBits() variants to share a common implementation rather than duplicating the code. Just wrap the primitives and call the getBits(NumberType, NumberType)/setBits(NumberType, NumberType, NumberType) version.

4. The javadoc for all the get/setBits[Worker]() methods has this: "The number of bytes to take into consideration" but it should be "bits" not "bytes".

5. The integer.setBitsWorker() implementation looks wrong to me. It looks like any high order bits (outside pos + len) in data will be turned on in the destination. In other words, the data being copied in needs to be masked off on both sides too. The comment also mentions using long, but you are really using int. I also don't understand what would happen in the 4GL if the pos + len == 33. For which cases have you tested it?

6. int64.setBitsWorker() has the same logic problems as integer.setBitsWorker().

7. What is the purpose of the changes to BlockManager.functionBlock()?

I would like to get the int64/integer stuff done and then work on the datetime stuff. Let's try to get this done in smaller batches rather than waiting for everything to be done on all data types.

Have you read the data types document that I mentioned in note 24? There is definitely some behavior in there for int64/integer that is not yet implemented. For example, it describes several things like internal math being done in 64-bit as well as the ability to use int64/integer interchangably (even in user function calls?)... we don't duplicate these features.

I still want to see the full list of all TODOs and your estimate of the effort to complete these. Please make that your top priority. Put those details in this task, as previously requested. Then focus on the int64/integer stuff and lets work to get that completely done. The datetime/datetime-tz stuff would come next as a separate update.

Greg,
I have integrated your feedback into the code for notes 1, 2, 3 and 4.
5 and 6 work fine, I double checked. If pos + len is 33/65 means that the bits involved are the last len bits from the left.
I have tested with various values like:

DEF VAR dest2 AS INT64 NO-UNDO FORMAT ">>>,>>>,>>>,>>>,>>>,>>>,>>9" INIT 1234567890123456789.
DEF VAR src2  AS INT64 NO-UNDO FORMAT ">>>,>>>,>>>,>>>,>>>,>>>,>>9" INIT 513.

PUT-BITS(dest2, 40, 10) = src2.
DISPLAY "PUT-BITS(dest2, 40, 10) = src2 --> " dest2 + 0 FORMAT ">>>,>>>,>>>,>>>,>>>,>>>,>>9" SKIP. /* should be 1234831772914123029 */

The point 7 is an intermediary fix for bug #2105. I have found it when assigning values received form dynamic functions.

The implementing the 64 bit computing in Progress way is a vast change. I did not knew exactly what are the problems that can arise during implementation I start, during last couple of days I have start coding and approached internal 64 bit computing in two ways:

• Using an intermediary 64-bit integer that is derived from NumberType and base class for both integer and int64. This class had the NumberType interface and most of the common implementation. Of course, on 64 bit storage. All operators were returning this class which simplified a lot MathOps / CompareOps of overloaded methods. Only assignments to integer are checked for overflow. I had problems with client/server communication and much more complicated since the ExpressionConversionWorker and maybe lot of TRPL should accommodate the new type.

• The second approach is to alter the class hierarchy so as the int64 is the base for integer. This would normally give a 64 bit background for entire platform and has all the advantage of the previous point, but without adding extra details for a new class. I have tested this approach and it is working quite well.

The problem I got now is with functions and procedures. They are searched using the exact signature. So, mixing integer and int64 does not work with current implementation. I saw that there is a note in ControlFlowOps about narrowing/widening of character/longchar, and this would also be the case for the integers too. On the other hand (while writing this) I found a possible solution by converting:

PROCEDURE someproc:
    DEF INPUT PARAMETER p1 AS INTEGER.
END.

   public void someproc(final int64 _p1)
   {
      internalProcedure(new Block()
      {
         integer p1 = new integer(_p1);
         ...

There could be a 3rd solution that will solve this issue and further simplified the code: merging the two classes and only separate them by a flag at declaration time. This would be used for emitting warnings/errors in case of 32 bit overflows and other internal management, but this approach is not very object-oriented (using if instead of overriding methods).

What is still to do is duplicate special functions that normally return 64 bit results and at conversion time, if the -noint64 is specified in p2j.cfg.xml the 32 bit implementation to be wired from TRPL.

One big disadvantage with all implementation (I believe) is that regression testing will not be very useful as there will be lots of small changes in the generated code.

• INPUT only:
  integer and int64 can be mixed.
  You cannot call a function using a longchar as argument for a character parameter.
• OUTPUT only:
  integer and int64 can be mixed.
  You cannot call a function using a character as argument for a longchar parameter.
• INPUT-OUTPUT:
  All parameters MUST receive the arguments of the exact type, they behave just like a strong type language.

The static calls will give compile-time errors, while calling them using dynamic-function will give runtime warnings.

At this moment P2J is less restrictive from this point of view. The conversion is successful for all kind of combinations and the overflow int64-to-integer are normally emitted when needed.

5 and 6 work fine, I double checked. If pos + len is 33/65 means that the bits involved are the last len bits from the left.

Make sure to put comments into the implementations so that readers understand why the code was written that way.

The second approach is to alter the class hierarchy so as the int64 is the base for integer. This would normally give a 64 bit background for entire platform and has all the advantage of the previous point, but without adding extra details for a new class. I have tested this approach and it is working quite well.

This seems to be the best approach, but I need to see the code to really understand the implications. Please post a newer version of the update for review (even if it is not done).

What is still to do is duplicate special functions that normally return 64 bit results and at conversion time, if the -noint64 is specified in p2j.cfg.xml the 32 bit implementation to be wired from TRPL.

Are there any other open issues with the int64 work? Please list all TODOs/issues/questions.

I will be reviewing the 0406 updates shortly.

I assume you are now working on the datetime/datetime_tz changes. Please make a list of the things that need to be implemented or fixed AND make an estimate of the time (in hours) that you will require to get that done (including time working with testcases, doing unit testing and regression testing).

From my research, the -noint64 parameter does not affect in any way the current implementation.

I think you are right about this. Any app that we convert is by definition being "re-compiled" and so long as we map everything consistently, it should not be a problem. I think this feature was added for a case where older code was being used with a newer runtime.

Code Review:

1. The update is missing your BlockManager change.

2. DisplayFormat.instanceOfType(String dataType, String value) is missing support for int64. Have you tested common int64 cases in the UI (DISPLAY, SET, UPDATE are the most important)? Make sure these are working.

3. As far as I know, there is no such thing as a "RAW literal". Please show a 4GL example that would trigger the code added to literals.rules.

4. I am a little nervous with the change to ExpressionConversionWorker.resolveNumericType(), which can no longer ever return "integer". The documentation on the new int64 support states that "operators (such as the * Multiplication and the + Addition operators) are still compiled as INTEGER if both arguments are INTEGER.". This change does not honor this statement.

5. In ExpressionConversionWorker.expressionType() for MW_MOD, there is this code:

            // both operands should be numbers
            jcls = resolveNumericType(ftype, stype);

            if (jcls == null)
            {
               // at least one operand is not numeric
               errmsg = "MODULO cannot have op1 (" + ftype + ") and op2 (" + stype + ").";
            } 
            else 
            {
               // if decimal rounding will occur, modulo always returns int64
               jcls = "int64";
            }

So long as both operands are numeric, this always returns "int64". Is that correct and is that intended?

6. I think this code in BinaryData will cause infinite recursion (then stack overflow):

   public integer getLong(NumberType pos)
   {
      if (pos == null || pos.isUnknown())
      {
         return new integer();
      }
      return getLong(new integer(pos));
   }

7. FYI, the changes in BinaryData are otherwise quite good. Please do note that the worker methods will be changed later to properly handle the host byte order as an option. But you don't need to handle that now.

8. I am still reviewing the code in integer, int64, MathOps and CompareOps. Since those changes are so extensive, it is going to take me a little more time to complete the code review for those portions.

More Code Review:

9. CompareOps is fine.

10. This MathOps method has a typo:

   public static int64 random(NumberType low, NumberType high)
   {
      if (low.isUnknown() || high.isUnknown())
      {
         return new int64();
      }

      return random(low.longValue(), low.longValue());  <--- 2nd parm should be high.longValue()
   }

11. I am worried about your removal of this code in MathOps:

    * If the left operand is greater than zero, then the result will be
    * as expected from normal integral arithmetic (the remainder after
    * integer division will be returned). However, if the left operand is
    * not greater than zero, then the result will depend on the value of
    * the remainder after the absolute value of the left operand is integrally
    * divided into the base.  Zero will be returned if the remainder in this
    * case is zero, otherwise the base minus the remainder will be returned.
    * Warning: the Progress documentation only suggests that this is an
    * invalid case which should not be relied upon. However, client
    * application code has been found to be reliant upon this behavior.
    * <p>
    * If either operand is the unknown value, the result will
    * be the unknown value.
    *
    * @param    op1
    *           The left operand (number to divide).
    * @param    op2
    *           The right operand (the divisor or modulo base).
    *
    * @return   The remainder result.
    */
   public static int64 modulo(NumberType op1, NumberType op2)
   {
      if (op1.isUnknown() || op2.isUnknown())
         return new int64();

      if (op2.longValue() < 1)
      {
         return genModuloError(op2.doubleValue());
      }

      // common case;  much faster than BigDecimal
      if ((op1 instanceof integer || op1 instanceof int64) && 
          (op2 instanceof integer || op2 instanceof int64))
      {
         return modulo(op1.longValue(), op2.longValue());
      }

      BigDecimal d1 = (op1 instanceof decimal
                       ? ((decimal) op1).toBigDecimal()
                       : new decimal(op1.longValue()).toBigDecimal());
      BigDecimal d2 = (op2 instanceof decimal
                       ? ((decimal) op2).toBigDecimal()
                       : new decimal(op2.longValue()).toBigDecimal());

      BigDecimal modVal = d1.abs().remainder(d2);

      // common case
      if (d1.compareTo(decimal.ZERO) >= 1)
           return (new int64(modVal));

      // handle non-positive input
      if (modVal.compareTo(decimal.ZERO) == 0)
         return new int64(0);

      return new int64(d2.subtract(modVal));
   }

Are you sure that the modulo(long, long) implementation is compatible? We know that the behavior is actually used.

I am now looking at the integer and int64 classes.

More Code Review:

12. What is this code in int64?

   /** 
    * This field is used only in conjunction with output procedure parameters. The argument to 
    * procedure/function is saved here and when leaving the block the new value is transfered 
    * back to argument object.
    * It can be a decimal (or an int64 for integer<code>). 
    */
   protected NumberType outputParameterReference = null;
</pre>

I don't want procedure/function processing to have implementation code here.  This is not a place someone would look to find/maintain parameter processing code.  In addition, the wrapper classes are meant to have a very limited amount of data and to be used in the client as well as the server.  It is even expected to use these wrappers in the database server (via pljava) and in remote applications that integrate with P2J.  None of these other use cases need or want the parameter processing code.  

If there is some problem with output parameters, I would like to understand and fix it elsewhere.  I also would not expect there to be special output parameter behavior just for numbers, so that suggests a general flaw in our processing that would be fixed elsewhere.  Anyway, I need to understand more about this.

There is other code in the int64 and integer classes that is likewise associated with this (e.g. the special constructor...).

13. In regard to this question about integer.hashCode():

<pre>
// TODO: should all integer-type objects return same hash for same value ??
</pre>

As far as I understand it, you have made equals() consistent with the values, no matter if it is int64 or integer.  So long as equals() and hashCode() return consistent results when the same values are used, then we are OK.  Do a final check for consistency, fix any problems that you find and then remove the TODO from hashCode().

14. Looking back at MathOps, I am wondering about whether we should have versions like this:

integer plus(integer, integer)
integer minus(integer, integer)
integer multiply(integer, integer)
integer negate(integer)
...

As noted above, the Progress documentation suggests that when all the operands are integer, that the result will be integer.

I'm done with the code review.  The work is really good!  Let's clear these 14 items and get this into testing ASAP.  We will need to regression test this both for conversion and runtime.  I want to test this independently of other updates.

Greg Shah wrote:

Code Review:

1. The update is missing your BlockManager change.
2. DisplayFormat.instanceOfType(String dataType, String value) is missing support for int64. Have you tested common int64 cases in the UI (DISPLAY, SET, UPDATE are the most important)? Make sure these are working.

For an unknown reason, I forgot to add these two files to update package.

3. As far as I know, there is no such thing as a "RAW literal". Please show a 4GL example that would trigger the code added to literals.rules.

Perhaps I did not document that well. It's about using ? raw literal. Consider the following code:

FUNCTION get-unknown2   RETURNS RAW:         RETURN ?.                  END FUNCTION.
DEFINE VARIABLE r AS RAW.
r = ?.
r = get-unknown2().

   raw r = new raw();
   r.setUnknown();
   r.assign(getUnknown2());

   public raw getUnknown2()
   {
      return function(raw.class, new Block()
      {
         public void body()
         {
            returnNormal(raw.instantiateUnknownRaw());
         }
      });
   }

4. I am a little nervous with the change to ExpressionConversionWorker.resolveNumericType(), which can no longer ever return "integer". The documentation on the new int64 support states that "operators (such as the * Multiplication and the + Addition operators) are still compiled as INTEGER if both arguments are INTEGER.". This change does not honor this statement.

I am convinced that this time, the documentation is wrong, too. Consider the following code that shows an int64 overflow (it will print -9223372036854775804):

DEF VAR ii AS INT64 INIT 9223372036854775807.
MESSAGE ii + 5.

DEF VAR i AS INT INIT 20000000.
DEF VAR j AS INT INIT 20000000.
MESSAGE i + j.

5. In ExpressionConversionWorker.expressionType() for MW_MOD, there is this code:
[...]
So long as both operands are numeric, this always returns "int64". Is that correct and is that intended?

Yes. From my research this is the way 4GL computes MOD: convert both operands to their int64 and then does the Modulo operation. For example:

DEFINE VAR d AS DECIMAL INIT 9223372036854775809.
MESSAGE d MOD 8589934591.5.

6. I think this code in BinaryData will cause infinite recursion (then stack overflow):
[...]

You are right about this. It should be returning getLong(pos.doubleValue()). I did not had a test-case for these forms. These functions were not in my scope so I did not invest too much time except for wiring the new functionality that was already available for int64. On the other hand, with the passing to 64 bit architecture I was expecting that 4GL to support 64-bit offset in memory maps/streams/files. However, this does not happen, in my tests, I could not set the size of a memptr to anything that do not fit into a 32-bit integer. Probably they will implement this in a future version or in true 64-bit build (which can access lots of memory)?

Greg Shah wrote:

More Code Review:

10. This MathOps method has a typo:
[...]

Thanks, because of the random function this was a little hard to see in testcases.

11. I am worried about your removal of this code in MathOps:
[...]
Are you sure that the modulo(long, long) implementation is compatible? We know that the behavior is actually used.

Please see my response on sub-issue 5 from my previous note.

12. What is this code in int64?
[...]

I don't want procedure/function processing to have implementation code here. This is not a place someone would look to find/maintain parameter processing code. In addition, the wrapper classes are meant to have a very limited amount of data and to be used in the client as well as the server. It is even expected to use these wrappers in the database server (via pljava) and in remote applications that integrate with P2J. None of these other use cases need or want the parameter processing code.

If there is some problem with output parameters, I would like to understand and fix it elsewhere. I also would not expect there to be special output parameter behavior just for numbers, so that suggests a general flaw in our processing that would be fixed elsewhere. Anyway, I need to understand more about this.

There is other code in the int64 and integer classes that is likewise associated with this (e.g. the special constructor...).

This is the same workaround as the one used in Text classes public _c'tor_(Text value, boolean output). This allows that the new value computes for OUTPUT parameters to be transferred back to the argument variable. I duplicated the implementation from there and went a little step further by defining the reference variable. I did not found any documentation on how this OUTPUT set-reference-value-on-procedure-exit will be implemented and i thought that this is the right place; I will contact Constantin about more details on this.

13. In regard to this question about integer.hashCode():
[...]
As far as I understand it, you have made equals() consistent with the values, no matter if it is int64 or integer. So long as equals() and hashCode() return consistent results when the same values are used, then we are OK. Do a final check for consistency, fix any problems that you find and then remove the TODO from hashCode().

OK.

14. Looking back at MathOps, I am wondering about whether we should have versions like this:
integer plus(integer, integer)
integer minus(integer, integer)
integer multiply(integer, integer)
integer negate(integer)
...
As noted above, the Progress documentation suggests that when all the operands are integer, that the result will be integer.

Yes, it does. However, I was not able to find any code sample that upholds that, in fact all my test proved that the only integer - feeling in 4GL is when you try to assign a value to a variable or field and you get a notification that it does not fit (see my response on sub-issue 3, above).

I'm done with the code review. The work is really good! Let's clear these 14 items and get this into testing ASAP. We will need to regression test this both for conversion and runtime. I want to test this independently of other updates.

Thanks. There are a lot of changes in the code and most of my test-cases are rather "localized". A full application-size test is welcomed.

Greg,
today I have investigated another point-of view. You mentioned the DisplayFormat, and starting to double check the user input/output routines from NumberFormat. I added the missing support here for placing the current value into a int64 variable, and attempted to run the testcases. Consider the following code:

DEFINE TEMP-TABLE tt
    FIELD f1 AS INTEGER FORMAT "->>>>>>>>>>9" 
    FIELD f2 AS INTEGER FORMAT "->>>>>>>>>>9" 
CREATE tt.
UPDATE tt.
MESSAGE tt.f1 tt.f2.

• when a very large number is typed into a integer/int64 fillin widget, the value is not checked for bounds anywhere. In fact, when computing the value to be stored in variable, the code:
  

  res = res * 10 + digits[i];
  

  
  causes repeated silent overflows, depending on the type of the re (long/int). For example, if user enters 9999999999 for an integer, the computed value will be trimmed to 1410065407.
• I tried to fix the first issue by checking for possible overflow and using ErrorManager.recordOrThrowError to signalize the exception case. As I understood from Constantin, and checked against 4GL, this check is a little to early, but the current implementation of P2J the content of the screen buffer cannot be transferred to server except contained wrapping temporary variable.
  Example:
  

  def var i as int format ">>>>>>>>>>>>>>>>>>>>>>.99".
  prompt-for i.
  message "read var: " input i i.
  assign input i.
  message i.
  

  
  I the user enters: 99.99, he gets on 4GL:
  

  read var:  99.99 0
  100
  

  
  but, on P2J:
  

  read var:  100 0
  0
  

  
  More strage, when you use the 4GL GUI on Windows you cannot enter values larger than the variable in cause nor enter decimals as the entered value is automatically rounded to an integer. This proves that the GUI version has a new layer of validation of user-input that is not available in ChUI mode.
• After adding the validation code (on client side, but this is not important now), something strange happen in the update if the user does the following steps:
  1. enters two large values for both f1 and f2, on GO, P2J will complain about the value too large and restart editing.
  2. lowers the f1 to an acceptable value, but lets f2 in place. P2J will apparently accept BOTH inputs and will display f1 's value 0 for f2.
  Looking into code, it seems to be an "early optimisation"-bug, the f2 's widget is marked as "clean" when f1 validation fails and user is prompted again but he only changes the first screen-field.

Perhaps I did not document that well. It's about using ? raw literal. Consider the following code:

OK, go with it.

If such an expression would be compiled using integers, the code:

DEF VAR i AS INT INIT 20000000.
DEF VAR j AS INT INIT 20000000.
MESSAGE i + j.

would print 1105788928. Instead it prints 400000000000000, which is a 64-bit number.

I don't understand this part. When I test this code, it prints 40000000. One can't really tell from this if the result is 32-bit or 64-bit. In addition, the message statement tends to do some implicit conversion to string, so it is a bit unreliable for use in testing this case.

So long as both operands are numeric, this always returns "int64". Is that correct and is that intended?

Yes. From my research this is the way 4GL computes MOD: convert both operands to their int64 and then does the Modulo operation. For example:

DEFINE VAR d AS DECIMAL INIT 9223372036854775809.
MESSAGE d MOD 8589934591.5.

Will give error 78 since the decimal value is a little too large to fit in an int64. If you decrease the d 's initial value to 9223372036854775807, it will print 8589934591, which is, of course not representable on 32-bit data type.

One concern here is whether you can ever get an integer back from modulo.

def var i as int init 14.
def var j as int init 3.
message i modulo j.

This prints 2 but as noted above, this is not a reliable way to check such things.

I guess what this all comes down to is this: can we devise a way to detect if something returns a 32-bit integer or not? If we can't devise a way to do that, then it may not matter because the application cannot be coded to detect the difference.

I did try this:

def var i as int init 2147483647.
def var j as int init 3.
def var k as int.

k = i + 1.
k = i + j.

Both of the assignments to k fail with an error like this:

Value 2147483648 too large to fit in INTEGER.

Please consider alternate approaches to detect the difference. If you can't think of any, then we will accept that the documentation seems wrong.

If this is the case, we will at least need to document (in MathOps for the operators and in the ECW) that contrary to the Progress documentation, we cannot find any cases where the application can detect any operators actually returning integer even when all operands are integer.

On the other hand, with the passing to 64 bit architecture I was expecting that 4GL to support 64-bit offset in memory maps/streams/files. However, this does not happen, in my tests, I could not set the size of a memptr to anything that do not fit into a 32-bit integer. Probably they will implement this in a future version or in true 64-bit build (which can access lots of memory)?

Possibly. They do at least support pointers that are 64-bit, but it is not too surprising that the memory objects are more limited. Actually, it is a bit of a relief, since allocating gigabytes of memory using 4GL code would be dangerous to say the least.

I will contact Constantin about more details on this.

Good. I look forward to some more discussion about this.

You mentioned the DisplayFormat, and starting to double check the user input/output routines from NumberFormat. I added the missing support here for placing the current value into a int64 variable, and attempted to run the testcases.

...

I found some issues (not all of them will probably fixed here, but they needed to be written down somewhere):

Please fix the current update first and let's get that reviewed/tested. Then the display issues can be fixed in a 2nd update.

Sorry, my mistake in the sample above, it should be multiplication, not addition:

DEF VAR i AS INT INIT 20000000.
DEF VAR j AS INT INIT 20000000.
MESSAGE i * j STRING(i * j).
DISPLAY i * j FORMAT ">>>>>>>>>>>>>>>>>>9" STRING(i * j) FORMAT "x(25)".

i = 2000000000.
j = 2000000000.
i = i + j NO-ERROR. /* this will fail with 15747 code */
MESSAGE i + j STRING(i + j).
DISPLAY i + j FORMAT ">>>>>>>>>>>>>>>>>>9" STRING(i + j) FORMAT "x(25)".

I don't know if I mentioned explicitly, but the int64 literals are also accepted in -noint64 mode:

i = 400000000000000 / j.

Code Review:

1. The modulo solution is still too simplistic. The 4GL has some behavior that doesn't exist in Java. For example:

message -10 modulo 3.

This results in:

2

Java's modulo results in: -1

Please examine the javadoc carefully. I think some additional logic is needed to duplicate the boundary conditions in the 4GL.

2. In BlockManager.functionBlock(), these changes are not ready for production:

//          ret[i] = (T) (retVal == null ? 
//            BaseDataType.generateUnknown(retType) : 
//               retVal[i].duplicate());

         if (retVal == null)
         {
            ret[i] = (T) BaseDataType.generateUnknown(retType);
         }
         else
         {
            try
            {
               BaseDataType newVal = retType.getConstructor(BaseDataType.class).newInstance(retVal[i]);
               ret[i] = (T) newVal;
            }
            catch (Exception e)
            {
               e.printStackTrace();
            }
         }
      }

Why is this needed? The BDT.duplicate() method is meant for this exact case.

3. In int64.setValue(NumberType), you have added a comment:

      double doubleValue = newVal.doubleValue();
      if (doubleValue > (double)Long.MAX_VALUE || doubleValue < (double)Long.MIN_VALUE)
      {
         // first check for very large numbers
         ErrorManager.recordOrThrowError(78, "Value too large for integer");

         // TODO: for some reasons, java internal architecture or something, this 
         // test fails to detect some large numbers like 9223372036854775809 that are do not fit 
         // into the 64-bit long. I believe that the precision is lost when converting to 
         // Long.MIN/MAX_VALUE to double.  It sees it as 9223372036854776329 from my observations,
         // so probably all integers in this interval are seen as 64-bit values.
      }

Why not use BigDecimal instead of double? Using floating point is quite a mess and can be a cause of issues like this. But BigDecimal will be precise and will not have such problems.

4. I see you have partially removed the output parameter stuff. I have not seen any discussion of it. Please summarize the discussion you and Constantin had and what conclusion you came to. Also: if it is not needed, let's remove the extra constructor too (in both int64 and integer).

5. In regard to the "when both operands are integer the result is integer" issue, I am fine with leaving this as all int64. But we must explain (in MathOps) that we are deliberately deviating from the Progress documentation. The reasoning is that our approach of making integer a subclass of int64 greatly reduces the number of potential conflicts AND we cannot find any way to actually prove that the 4GL will emit integer as in practice. Note that any overflow will silently yield an int64 and we cannot find any use case where integer is required and int64 makes things fail. Place elaborate on this and put it in the class-level MathOps javadoc. The list of supported operators also needs to be updated as it all references integer versions.

Why is this needed? The BDT.duplicate() method is meant for this exact case.

I will remove the old code. The BDT.duplicate() is not the "short form" for the the new code.

I am also concerned with the e.printStackTrace() in the catch block. That is not the way we can leave the code.

From our discussion, I understood that a new class that will implement Scopable will be implemented. It will take the reference to output variable in constructor and will be registered with TransactionManager.registerScopeable. In scopeFinished the reference will be updated.
I don't know if this task is the place for this implementation, maybe #1920 ? I won't do any changes here, the code is very similar to Text objects, when this new class will be implemented will update all affected classes.

Please remove all code related to this output parameter case (including the extra constructors). Also make sure that you or Constantin put the suitable TODOs into #1920 so we don't forget this.

Interesting finding.

If we make this change:

FUNCTION f RETURNS INTEGER: RETURN INT64(9223372036854775807). END.
MESSAGE f().
def var i as int.
i = f().
message i.

Then we get this output:

9223372036854775807
Value 9223372036854775807 too large to fit in INTEGER.
** Unable to update  Field. (142)

This suggests that you can sometimes create an INTEGER that contains a 64-bit value (using this "loophole" in function return processing) and that they have only implemented the 32-bit overflow check in certain places like assignment.

This version:

FUNCTION f RETURNS INTEGER: RETURN INT64(9223372036854775807). END.
FUNCTION x RETURNS INTEGER (num as INTEGER) : RETURN num. END.
MESSAGE x(f()).

results in this:

Value 9223372036854775807 too large to fit in INTEGER. Line 0 in x /home/gregs/p50596_Untitled2.ped. (15747)
User-defined Function could not build input runtime parameters from the stack. (6348)
?

Which is similar to this (but not exactly the same):

FUNCTION x RETURNS INTEGER (num as INTEGER) : RETURN num. END.
MESSAGE x(9223372036854775807).

which results in:

Value 9223372036854775807 too large to fit in INTEGER. Line 0 in x /home/gregs/p50596_Untitled2.ped. (15747)
Routine /home/gregs/p50596_Untitled2.ped sent called routine x /home/gregs/p50596_Untitled2.ped mismatched parameters. (2570)
?

In both of the last 2 examples, the body of function x() does NOT execute. But an ERROR condition is not raised. Instead, the function call is short circuited to unknown value and program execution continues.

I also note that you can eliminate the INT64 function and just return the oversized constant:

FUNCTION f RETURNS INTEGER: RETURN 9223372036854775807. END.
MESSAGE f().

This works fine too.

does this proves somehow that there is no real integer type in Progress arithmetic ?

Possibly.

Should we override the integer return type of user-defined functions to int64 ?

No, I want to avoid that.

Perhaps we can:

1. Detect the case where this needs to happen.
2. Allow an integer to be constructed with the oversized data and return it.

The INT64 is only needed when running Progress in -noint64 mode to make the literal integer-type. In absence of the function, the literal is indeed of decimal type. You cannot use a decimal here or you get the compile 2782 error:

RETURN statement in user defined function or method 'f' is not type compatible with the function or method definition. (2782)

From your test and my observation in the last days looks like Progress only checks the bounds only at assignment. This includes passing an argument to a function in INPUT mode which I believe assigns the value to an scoped variable (the same way we do). This is why x does not execute - because the intermediary step of initialization (copy the parameter value to local int variable) fails so there is no data on stack available.

A solution can be to disable the bound checking for integers that are used for returning values from functions. At this moment this is the only place where this happens.

I remember now some other interesting sample:

DEF VAR d1 AS DATETIME. 
DEF VAR d2 AS DATE. 
d1 = TODAY. 
d2 = TODAY.
FUNCTION f1 RETURNS DATETIME. RETURN d1. END. 
FUNCTION f2 RETURNS DATETIME. RETURN d2. END.

MESSAGE f1() f2().
MESSAGE DYNAMIC-FUNCTION("f1") DYNAMIC-FUNCTION("f2").

LE: In fact the date/datetime gives a hint about returning values in Progress.
The argument of RETURNS statement is just a HINT to compiler for what kind of values are allowed to be returned from within the function body. In fact the returning value can be of any kind (if you can pass the 4GL compiler). The same are the POLY functions implemented.

When do you expect to have the next update ready for review? I see the same om_upd20130416b.zip posted 3 times, but I wonder if you have something newer ready.

In a few minutes. I'm working on finalizing the update.
I don't understand the 2 extra updates, probably the browser re-uploaded them when I refreshed the page ? I removed them anyway.

As part of adding support for new data types, please review com.goldencode.p2j.persist.HQLPreprocessor and the com.goldencode.p2j.persist.hql package to see if anything needs to be updated. I expect this area was passed over for M4, since it is purely about runtime functionality, but we will need to integrate the new data types here, too.

Code Review:

1. The change to RANDOM processing in ECW:

         case FUNC_INT:
            // possible override of the simple type
            oldtype = unpackOldType(source);
            if (oldtype == KW_RANDOM)
            {
               // if at least one of the operands are int64 the result is int64
               ftype = expressionType(source.getChildAt(0), infer, fuzzy);
               stype = expressionType(source.getChildAt(1), infer, fuzzy);

               if (jcls == null)
               {
                  // at least one operand is not numeric
                  errmsg = "Incompatible data types in expression or assignment. (223)";
               } 
               else 
               {
                  // same like MODULO operator, if at least one operand is decimal, rounding will 
                  // occur; RANDOM is always performed on integer-type operands and returns int64
                  jcls = "int64";
               }
            }
            break;

• In what case will jcls be null here?
• Why generate the ftype and stype if we aren't going to use them?

2. cleanup.rules needs a history entry.

Otherwise it looks ready to go. Really good work!

All the changes are OK except the PropertyMapper. PropertyMapper is used as a helper for the database import process.

Here are the issues:

1. LONGCHAR cannot ever be a database field, so we don't need a helper for that type.

2. The getPropertyClass() method should return the same type as we emit in the setter for a field of that type. That means:

• integer, int64 and decimal all use NumberType.class
• date, datetime and datetimetz all use date.class
• character uses Text.class (this is a bug already existing in the file)
• raw uses BinaryData.class
• leave rowid as NumberType.class for now, we aren't really sure if this mapper is even needed or the proper type

It looks good. Please do both conversion and runtime regression testing on this.

Code Feedback:

1. The TestCase support should be put back into date. At some point we will move that into a set of JUnit tests. But until then it is a useful check on the logic of dates.

2. It seems like ToClause would need signatures that take int64.

3. We have Evgeny Kiselev working on XML support right now. Please send him the minimum list of changes for XEntityImpl and he can merge it with his stuff. Don't send him anything cosmetic. As far as I can understand, you don't have a dependency on the changes in that class, right? If not, then we can let him deliver the change to that file.

4. On the Majic source changes, I have some concerns. Most changes look OK. But there are so many changes, I can't really be sure. Specific concerns:

• In some places there are format strings added that weren't there before (e.g. see ar/DunnT.java for "->,>>>,>>9").
• I don't understand why the frame definitions are using int64. That seems bad/wrong.

5. Does Majic compile with all these changes?

Greg Shah wrote:

Code Feedback:

1. The TestCase support should be put back into date. At some point we will move that into a set of JUnit tests. But until then it is a useful check on the logic of dates.

That code was a little annoying in IDE. My intention to remove it temporarily, I forgot to add it back.

2. It seems like ToClause would need signatures that take int64.

True, I'll add long/int64 constructors/methods.

3. We have Evgeny Kiselev working on XML support right now. Please send him the minimum list of changes for XEntityImpl and he can merge it with his stuff. Don't send him anything cosmetic. As far as I can understand, you don't have a dependency on the changes in that class, right? If not, then we can let him deliver the change to that file.

Already sent him an email.

4. On the Majic source changes, I have some concerns. Most changes look OK. But there are so many changes, I can't really be sure. Specific concerns:

• In some places there are format strings added that weren't there before (e.g. see ar/DunnT.java for "->,>>>,>>9").

I also observed that last evening after posting the diff file. I'm starting the investigation on this.

• I don't understand why the frame definitions are using int64. That seems bad/wrong.

I think this because some expressions are displayed. If a + b is to be printed, then the expression is evaluated to int64 even if both variables are integers. Normally, if the sum fits in default "->,>>>,>>9" format, there is difference. On the other hand, if the sum is larger than Integer.MAX_VALUE, 4GL will only show the error that the sum cannot be displayed using the current format. If the frame definition would have been generated as integer, P2J will attempt to assign the sum to respective integer and the "value too large to fit INTEGER (15747)" will prevent procedure to continue.

5. Does Majic compile with all these changes?

Yes, Majic converts and compile without any errors using the latest updates.

The changes look good. Go ahead with the runtime regression testing. Great work!

I encountered the following issue while implementing the SESSION:DISPLAY-TIMEZONE attribute:
The attribute can be inited on server-side form directory, and manually set/read as a SESSION attribute.
However, this attribute is used to display datetime-tz values which do not have time-zone specifiers in their format. The format is parsed and filled with values on the client side. The current value for the attribute, set on above line in Progress, is not available here. Should I 'stream' this attribute to client in order to be used in computing the right value to be displayed?

Please review NumberType.initContextSpecificSeps() and how it is called. Please note that using the DirectoryManager for such things allows this code to be called either from the client or the server. The problem with this is that it doesn't take dynamic changes into account (e.g. if this is set from converted code using a writable attribute).

Instead, we may want to define a remote interface that will push such configuration values down to the client dynamically (when the converted code makes a change). An example is PROPATH assignment (see EnvironmentOps.setSearchPath(String, boolean)).

Any such remote interface should be separated from the wrapper classes AND it should probably handle more than just one data type. So it could handle passing down values for all the wrapper types (date, numbertype...).

It may even be a good idea to pull these values out of the wrapper classes completely and put them into a Session singleton class (SessionUtils?). The advantage would be to centralize the directory init logic as well as the dynamic push down to the client. It may make the wrappers simpler, which is a very good thing. The storage can still be in the wrapper classes, but perhaps the session code for managing it should not be there.

Constantin: thoughts?

Constantin: thoughts?

My choice would be this:
- store these at the wrapper classes
- use SessionUtils to manage them; when an API is called to set i.e. DISPLAY-TIMEZONE, it will save it at the wrapper class on server side and will push it to the client too, to save it at the wrapper class on that end

Idea is, I don't see a benefit of storing these in a central class, but centralizing both their initialization and their manangement looks good.

Let's do it that way.

Runtime regression testing produces these failures:

Caused by: java.lang.IllegalArgumentException: Unknown DMO implementation class:  class com.goldencode.p2j.util.int64
        at com.goldencode.p2j.persist.DatabaseManager.normalizeDMOClass(DatabaseManager.java:801)
        at com.goldencode.p2j.persist.DBUtils.makeTypeArray(DBUtils.java:191)
        at com.goldencode.p2j.persist.DBUtils.makeTypeArray(DBUtils.java:139)
        at com.goldencode.p2j.persist.HQLHelper.obtain(HQLHelper.java:349)
        at com.goldencode.p2j.persist.RandomAccessQuery.getHelper(RandomAccessQuery.java:2408)
        at com.goldencode.p2j.persist.RandomAccessQuery.activateUnique(RandomAccessQuery.java:2345)
        at com.goldencode.p2j.persist.RandomAccessQuery.unique(RandomAccessQuery.java:1516)
        at com.goldencode.p2j.persist.RandomAccessQuery.unique(RandomAccessQuery.java:1433)
        at aero.timco.majic.xt.Menulaun$1$3.body(Menulaun.java:1124)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6939)
        at com.goldencode.p2j.util.BlockManager.coreLoop(BlockManager.java:8115)
        at com.goldencode.p2j.util.BlockManager.repeatWorker(BlockManager.java:8009)
        at com.goldencode.p2j.util.BlockManager.repeat(BlockManager.java:1468)
        at aero.timco.majic.xt.Menulaun$1.body(Menulaun.java:196)
        at com.goldencode.p2j.util.BlockManager.processBody(BlockManager.java:6939)
        at com.goldencode.p2j.util.BlockManager.topLevelBlock(BlockManager.java:6846)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:213)
        at com.goldencode.p2j.util.BlockManager.externalProcedure(BlockManager.java:195)
        at aero.timco.majic.xt.Menulaun.execute(Menulaun.java:63)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:601)
        at com.goldencode.p2j.util.ControlFlowOps$InternalEntryCaller.invoke(ControlFlowOps.java:3776)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:3099)
        at com.goldencode.p2j.util.ControlFlowOps.invokeImpl(ControlFlowOps.java:2878)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:301)
        at com.goldencode.p2j.util.ControlFlowOps.invoke(ControlFlowOps.java:287)
        at aero.timco.majic.menu.Call$1$2.body(Call.java:172)

You have to update the TYPES array in com.goldencode.p2j.persist.DBUtils to reflect the new data types.

Does this need to also add a line for Int64Field into the TYPES map?

One more change is needed:
persist.hql.DataTypeHelper.initJavaClasses() needs the int64 mapping added too.

Eric,
While looking over the DataTypeHelper for any lines that would need int64 support I reached the initBasicTokens().
My question is whether NUM_LITERAL should be mapped to with HQLTypes.LONG (in the case of large literals)?

I'm wondering if a separate token type for int64 literals (which would map to HQLTypes.LONG) is more appropriate. Greg, thoughts?

Note that the introduction of int64 breaks DataTypeHelper in other places as well. For example, the expressionType method currently does not properly account for integers larger than 32 bits in its analysis of mathematical operations in where clauses. Every use of HQLTypes.INTEGER in the project should be examined.

I'm wondering if a separate token type for int64 literals (which would map to HQLTypes.LONG) is more appropriate. Greg, thoughts?

There is no such thing as an int64 literal. There is only 1 literal type for integral data (NUM_LITERAL) and both integer and int64 literals are handled by the same token.

This is the current NUM_LITERAL rule from hql.g:

NUM_LITERAL
   :
      {
         int ntype = NUM_LITERAL;
      }
      (  
           '.' (DIGIT)+              { ntype = DEC_LITERAL; }
         | '.'                       { ntype = DOT;         }
         | (DIGIT)+ ( '.' (DIGIT)*   { ntype = DEC_LITERAL; } )?

      )
      {
         $setType( ntype );
      }
   ;

I am suggesting something like:

NUM_LITERAL
   :
      {
         int ntype = NUM_LITERAL;
      }
      (  
           '.' (DIGIT)+              { ntype = DEC_LITERAL; }
         | '.'                       { ntype = DOT;         }
         | (DIGIT)+ ( '.' (DIGIT)*   { ntype = DEC_LITERAL; } )?

      )
      {
         if (ntype == NUM_LITERAL)
         {
            try
            {
               long l = Long.parseLong(getText());
               if (l < Integer.MIN_VALUE || l > Integer.MAX_VALUE)
               {
                  ntype = LONG_LITERAL;
               }
            }
            catch (NumberFormatException exc)
            {
               // what to do here?
            }
         }
         $setType( ntype );
      }
   ;

Does that make sense?

Currently, the only place where we distinguish between HQLTypes.LONG and HQLTypes.INTEGER is when deciding how to cast a query substitution parameter in a CASE construct like case ... then ? else ?, so that we end up with something like case ... then cast(? as long) else cast(? as long) (this syntax is needed by H2). As we add user defined functions with int64 parameters or return types (i.e., representing builtin functions), we will need to make the distinction for those as well.

OK.

The changes are good. Go ahead with runtime regression testing.

Please be aware that some overlapping changes have been made in #1580 and they will likely be checked into bzr within the next 24 hours. Be prepared to merge with that code. It probably will require another round of regression testing. But I think it probably makes sense to still go forward with testing now to identify if there are any remaining issues as the update stands right now.

Constantin Asofiei wrote:

Runtime showed more persistence related errors:

Regarding the ClassCastException: there is an assumption in the RecordBuffer invocation handler that an index value passed to an indexed getter/setter in a DMO is either an instance of integer or Integer. Based on the changes to the data wrappers hierarchy to add int64, we should be using Class.isAssignableFrom(Class) instead of instanceof in the code which extracts the index (line 7711).

Regarding the "PLUS" not found error: IIRC, the references to "plus", "minus", "divide", etc. should be overloaded to deal with different combinations of parameter types. So, "plus" would be something like "plus_1" or "plus_4" (like the "toInt_5" we see in the same query). I suspect the problem is with DataTypeHelper.expressionType, specifically here (note, I am looking at the latest code in bzr, so your version may differ):

case PLUS:
case MINUS:
case MULTIPLY:
case DIVIDE:
   c0 = expressionType(node.getChildAt(0), dmoClass);  // lvalue
   c1 = expressionType(node.getChildAt(1), dmoClass);  // rvalue
   return (c0 == HQLTypes.DECIMAL || c1 == HQLTypes.DECIMAL
           ? HQLTypes.DECIMAL
           : HQLTypes.INTEGER);

I think this code needs to be int64 aware. I suspect we are getting into trouble with the FunctionKey class based on the HQLTypes[] signature parameter passed to that class' constructor, because of the code above. Changes in related areas may be necessary, too.

BTW, has something changed with the logging configuration? I'm seeing lots of errors in stdout.log that I would normally expect to see in server.log.

Eric Faulhaber wrote:

BTW, has something changed with the logging configuration? I'm seeing lots of errors in stdout.log that I would normally expect to see in server.log.

TIMCO's changes to one of their customer_libs/ jars loads a log4j configuration which writes the hibernate logs to stdout.log instead of server.log. I think this can be solved if we remove/change the etc/log4j.properties file.

I have investigated the new found issues. As expected Progress is always processing integer data on 64 bits. However, out mapping of operators and functions are still on 32 bits so, even if the entry data is on 32 bit, the result can be different:
Here is a simplified test:

define temp-table tt field i as INT FORMAT ">>>>>>>>>>>".

create tt.
ASSIGN tt.i = 2000000000.

FIND FIRST tt WHERE i + i > 2000000000.
IF AVAILABLE tt THEN
    DISPLAY tt.i tt.i + tt.i.
ELSE 
    DISPLAY "not found".

Greg,

I am investigating adding ISO date-time literals to progress.g.
If d is a kind of progress date variable, then the following are valid:

d = 2013-05-31T18:30:59.999+01:00.
d = 2013-05-31T18:30:59.999.
d = 2013-05-31T18:30:59.
d = 2013-05-31T18:30. 

I tried to alter the last branch of NUM_LITERAL (progress.g:29792) but my problem is that the lexer cannot guarantee that the first group of digits has 4 figures. So I attempted to create a separate branch that looks like this:

  // datetime-tz literal
| DIGIT DIGIT DIGIT DIGIT '-' DIGIT DIGIT '-' DIGIT DIGIT ('t' | 'T') DIGIT DIGIT ':' DIGIT DIGIT
  (':' DIGIT DIGIT ( '.' DIGIT DIGIT DIGIT (('+'|'-') DIGIT DIGIT ':' DIGIT DIGIT )? )? )?
  { ntype = DATE_LITERAL; }

Could you give me a hint here?

Ovidiu Maxiniuc wrote:

My proposal is to switch all calculations from sql server (pljava) to 64-bit, assigning all integer-types to Long and back to int64 (here I am not sure if this is possible). This will match the 4GL engine and we get rid of overloading the operators with both Integer and Long operands.

I agree with this proposal. This will require changes to the p2jpl.ddr file, in addition to the Functions and Operators classes. Probably also several classes in the persist.hql package. Stanislav will be checking in his #1580 update shortly, which will likely have changes in the same areas.

In regard to note 90:

Please see testcases/uast/datetime_literals.p and testcases/uast/datetimetz_literals.p for some good examples. I think you will find that there are some additional optional cases that need to be considered. For example, the minutes can be specified as 1 or 2 digits.

Here is my summary from the testcase comments:

/* the date can only use - as a separator (no / or .); zero padding is required */
/* so that the yyyy-mm-dd is as fixed 10 character date portion; no spaces */
/* before or after the T (or t); the following time can be specified as HH:M:S */
/* or HH:MM:S or HH:M:SS or HH:M:S.s (and so on up to SS and sss); note that the */
/* hours must be 2 digits and there must be minutes and seconds fields (even if */
/* only 1 digit each; the millis field is optional and can be specified in 1, 2 */
/* or 3 digits; the timezone offset is optional; when present there must be the */
/* seconds portion and no intervening whitespace; the hours and/or minutes can be */
/* specified as a single digit and the sign is not optional (if there is an offset */
/* at all); the ":M" or ":MM" portion of the offset is optional */

Your overall approach is the correct approach. We must make the first portion of this fixed and it will allow unique matching with the datetime-tz case. The key thing is to place this just after the standalone DOT alternative. AND to fix the greedy nature of the matcihng, we must make the silly looking rule even more silly:

           // datetime-tz literal
         | {
             (LA(1) >= '0' && LA(1) <= '9')   &&
             (LA(2) >= '0' && LA(2) <= '9')   &&
             (LA(3) >= '0' && LA(3) <= '9')   &&
             (LA(4) >= '0' && LA(4) <= '9')   &&
             LA(5) == '-'                     &&
             (LA(6) >= '0' && LA(6) <= '9')   &&
             (LA(7) >= '0' && LA(7) <= '9')   &&
             LA(8) == '-'                     &&
             (LA(9) >= '0' && LA(9) <= '9')   &&
             (LA(10) >= '0' && LA(10) <= '9') &&
             (LA(11) == 't' || LA(11) == 'T')
           }?
           DIGIT DIGIT DIGIT DIGIT '-' DIGIT DIGIT '-' DIGIT DIGIT
           ('t' | 'T') DIGIT DIGIT ':' DIGIT (DIGIT)?
           (
              ':' DIGIT (DIGIT)? ('.' DIGIT (DIGIT (DIGIT)? )? 
              (('+'|'-') DIGIT (DIGIT)? (':' DIGIT (DIGIT)? )? )? )?
           )?
           { ntype = DATETIME_TZ_LITERAL; }

I have added a semantic predicate to lookahead and only match if we are in a datetime-tz literal. I also make a quick attempt to fix the optional nature of many of the fields. I have not tested this at all, so it needs careful review and testing. Also note that the token type is set to DATETIME_TZ_LITERAL, not DATE_LITERAL.

I am fine with the changes, but most of them are in code of which Eric is the "owner".

Eric: please take a look at the code changes and confirm if you are OK.

Ovidiu: does this resolve all known issues found in regression testing? In other words: is this ready to go back into testing or is there more work to do?

Yes, this update should fix the Function "PLUS" not found error from stdout.log.
The update also contain the fix for int64 / Integer ClassCastException from server.log.

Go ahead with runtime testing. If Eric finds anything of concern we can deal with it at that time.

It looks fine.

Regarding DATETIME_TZ_LITERAL note 93.

I have tested and adjusted a little the rule for the literal. The seconds and theirs fractions (which can be any digits long but only first 3 are taken in consideration) are optional "in parallel" with timezone, but keeping the current order.
At first sight, everything seemed OK, in my testcases it fails only once: when the DOT is assimilated to the dot that separates the seconds from theirs fractions.
I tried to get some inspiration from the decimal '.' case, but things are a little tricky. As the rules are more complex I try to count the exact look-ahead step for investigating if after the '.' is a digit or not.

Here is how it turned out:

    (LA(1) >= '0' && LA(1) <= '9')   &&
    (LA(2) >= '0' && LA(2) <= '9')   &&
    (LA(3) >= '0' && LA(3) <= '9')   &&
    (LA(4) >= '0' && LA(4) <= '9')   &&
    LA(5) == '-'                     &&
    (LA(6) >= '0' && LA(6) <= '9')   &&
    (LA(7) >= '0' && LA(7) <= '9')   &&
    LA(8) == '-'                     &&
    (LA(9) >= '0' && LA(9) <= '9')   &&
    (LA(10) >= '0' && LA(10) <= '9') &&
    (LA(11) == 't' || LA(11) == 'T') &&
    (LA(12) >= '0' && LA(12) <= '9') &&
    (LA(13) >= '0' && LA(13) <= '9') &&
    LA(14) == ':' &&
    (LA(15) >= '0' && LA(15) <= '9')
  }?
  DIGIT DIGIT DIGIT DIGIT '-' DIGIT DIGIT '-' DIGIT DIGIT
  't' // implies 'T' also, as caseSensitive = false
  DIGIT DIGIT ':' DIGIT (DIGIT)?
  (':' DIGIT (DIGIT)? 
         (
            // avoid matching a following "lone" dot (no digits after it)
            '.' { (LA(1) >= '0' && LA(1) <= '9') }? (DIGIT)+
            | // empty alternative
         )
  )? // seconds and millis are optional
  ( ('+'|'-') DIGIT (DIGIT)? (':' DIGIT (DIGIT)? )? )?   // tz offset is also optional
  { ntype = DATETIME_TZ_LITERAL; }

The progress.g update looks good. Well done.

Why are you deleting files? We generally want to leave old files around to show the progression of the work and allow all the comments in the history to make sense.

Thanks.
I am moving majic-related archives to new task #2150.

I'm reviewing the database portions of 20130507b.

Some of the latest updates committed to bzr last week are not present (e.g., import.xml, PropertyMapper, DBUtils, RecordBuffer, DataTypeHelper). Please merge your changes with the latest revision of code in bzr. Note that some changes conflict, primarily where they are about the new data types. In most cases, your data type-related changes are more complete (except DataTypeHelper; we need to reflect datetime and datetimetz).

For some unknown reason, if at least one operand was a bigint the other was automatically converted to bigint, too. The case when both operands are integer had to be declared as a overloaded operator even if calling the same java function.

I believe when doing comparison and mathematical operations, the database will perform a widening conversion on one operand when the other is already a bigint, and will produce a widened result.

I think the overloading approach you have taken is OK for PostgreSQL. I'm concerned about H2 getting the type conversions right, though, since it creates its UDFs directly from the Functions and Operators classes' public, static methods (there is nothing like the p2jpl.ddr file for H2). I expect we will see failures in cases where we try to invoke UDFs (like plus) with two 32-bit integer operands. I'm not sure H2 will know to widen them to invoke Operators.plus(Long, Long). Have you tried such a test using H2 only?

I was fine-tuning my implementation of datetime/datetime-tz when I have found some /interesting features/ of Progress, but at this moment it's a little difficult to emulate them in P2J. Here is a bit of code:

SESSION:DATE-FORMAT = "ymd".

DEFINE VARIABLE ref AS CHARACTER.
FORM ">" ref "<" WITH NO-LABELS.

DEFINE VARIABLE d AS DATETIME.
d = NOW.

DISPLAY d @ ref.

This will be converted by P2J when printing to:

new Element(d, "99/99/9999 HH:MM:SS.sss", frame0.widgetRef())

as the types of ref and d do not match.
This seems to be fine, but Progress takes the date-format into consideration and prints:

> 2013/05/10 15:32:26.897 <

So the default format seems to be somewhat dynamic, depending on current date-format. We are currently resolving this format statically, at conversion time (in common-progress.rules, <function name="format_for_type">) if types do not match when using DISPLAY @.

The things are even more complicated. I added some formatting to d variable:

DEFINE VARIABLE d AS DATETIME FORMAT "99-9999-99".

P2J converted correctly. 4GL continued to ignore the given format and the separators, printing:

> 2013/05/10 <

However, my luck improved when I altered the format to "99-99999-99" or "99-999-99". These times it started producing "expected" outputs like

> 13/00005/10 <
> 13/005/10 <

The climax was when using "999-999-999" format which caused Progress to print > ??????????? < and complain that

** Value 13/05/10 cannot be displayed using 999-999-999. (74)

I found another issue. However, I think this can be solved at conversion time by adding some parameter or overloading a method or creating a static function for parsing literals.

Here it is:

SESSION:DATE-FORMAT = "dmy".
MESSAGE DATE("04/05/-4716") EQ 04/05/-4716.

This will be converted by p2j to

message(isEqual(new date("04/05/-4716"), new date("04/05/-4716")));

which will lead to yes as the same string will be parsed the same way to same date.
On 4GL it will print NO, as the DATE function will use the SESSION:DATE-FORMAT to choose the order of components, while the literal is always in mdy format.

At this moment you cannot tell at runtime whether the parsing string was the literal or passed as argument to DATE function. This must be fixed by one of the solutions I wrote above.

In regard to my last note.
I tried today to implement parsing of date literal in TRPL but I found it to be rather complicated.
Instead I created a static method in date:

public static date fromLiteral(String str)

      Pattern p = Pattern.compile("^(\\d+)/(\\d+)/((\\-)?\\d+)$");
      Matcher matcher = p.matcher(str);
      if (!matcher.matches())
      {
         return date.instantiateUnknownDate(); // format not recognized
      }

      try 
      {
         int month = Integer.parseInt(matcher.group(1)); 
         int day = Integer.parseInt(matcher.group(2));
         String strYear = matcher.group(3);
         int year = Integer.parseInt(strYear);
         return new date(month, day, year, strYear.length() < 3); 
      }
      catch (NumberFormatException e)
      {
         return date.instantiateUnknownDate(); // invalid number format
      }

I struggled today with some particular cases of date/datetime arithmetics, namely the "Gregorian gap".
It looks like Progress has some small bugs here, which will probably need to be duplicated.
Here are some examples:

add-interval(10/04/1582, +1, "month") EQ 11/14/1582 /* I was expecting 11/04/1582 here */

add-interval(11/14/1582, -1, "month") EQ 10/24/1582 /* I was expecting to get the initial 10/04/1582 */

add-interval(10/15/1582, -1, "month") EQ 09/15/1582 /* this looks normally */

add-interval(09/15/1582, +1, "month") EQ 10/25/1582 /* not the initial 10/15/1582 */

At this moment I cannot figure exactly how Progress handles this arithmetic as the size of months differs:

MESSAGE INTERVAL(ADD-INTERVAL(10/15/1582, -1, "month"), 10/15/1582, "days"). /* = -20 */
MESSAGE INTERVAL(ADD-INTERVAL(09/15/1582, +1, "month"), 09/15/1582, "days"). /* = 30 */

For the moment, I suspended the work on this, as probably this year will not be very used in today's applications.

• There ARE datetime literals! There are a few places in the project where they are denied and, of course, the documentation affirms this.
  However, this is not true. My mistake for taking this as granted. Here are two samples that proves it:
  

  FUNCTION getDateTime RETURN DATETIME:
      RETURN 2050-12-31T10:30:21.555+10:00.
  END.
  

  
  This will not compile, you will get an error about wrong return type. However, if the time-zone part is remove, the compiler happely accepts the literal.
  Second, you cannot compare one literal that has time-zone with another that hasn't. Again, the compiler will print error 223: "Incompatible data types in expression or assignment".
  

     MESSAGE 2050-12-31T10:30:21.555 GT 2050-12-31T10:30:21.555+10:00.
  

  
  For the moment, P2J is more permisible form this point of view, ie it will accept datetime and datetime-tz literals interchangely and will not print errors and warnings as Progress does.

• As seen in previous example, datetime and datetime-tz are not compatible for comparing. In fact they cannot be compared normally with date values too, so the following lines are not valid in 4GL:
  

     /* MESSAGE 2050-12-31T10:30:21.555 GT 12/31/2050.                    -- not compilable */
     /* MESSAGE 2050-12-31T10:30:21.555 GT 2050-12-31T10:30:21.555+10:00. -- not compilable */
     /* MESSAGE 2050-12-31T10:30:21.555+10:00 GT 12/31/2050.              -- not compilable */
  

  
  On the other hand, if the values to be compared are obtained dynamically, things change. I found that, in this case, a date value can be compared with both datetime and datetime-tz values, BUT datetime and datetime-tz are yet not compatible (at least not without adding aome extra conversion function).
  Examples:
  

     DISPLAY DYNAMIC-FUNCTION("getDateTime") EQ 12/31/2050.
     DISPLAY DYNAMIC-FUNCTION("getDateTimeTz") EQ 12/31/2050.
     DISPLAY DYNAMIC-FUNCTION("getDateTimeTz") EQ DYNAMIC-FUNCTION("getDateTime") NO-ERROR. /* otherwise this will fail at runtime */
  

  
  You'll get yes yes ? as output because the operators will convert the datetime and datetime-tz to date before doing the comparaison. The 3rd result is unknown because of NO-ERROR clause, otherwise error 223 will break the program.

Eric Faulhaber wrote:

I think the overloading approach you have taken is OK for PostgreSQL. I'm concerned about H2 getting the type conversions right, though, since it creates its UDFs directly from the Functions and Operators classes' public, static methods (there is nothing like the p2jpl.ddr file for H2). I expect we will see failures in cases where we try to invoke UDFs (like plus) with two 32-bit integer operands. I'm not sure H2 will know to widen them to invoke Operators.plus(Long, Long). Have you tried such a test using H2 only?

Indeed, H2 need additional overloads of the operators and functions. I am in progress of adding the needed method overloads, at this time I have about 100 of each one because of multiple possible signatures that involves 3 data types: integer/Integer, int64/Long and decimal/BigDecimal.
On the other hand, I think the relational operators do not need to appear in the Operators.java as I see in converted code, the native relational operators are used directly (namely <, >, <=, >=, =, !=). I don't know if this is a good thing or not (I mean if this operators give the same results as the ones from P2J).

I'm running conversion/runtime testing on devsrv01 with this update.

Only one issue found, related to formatting. For a test like:

def var i as int init 1000.
form i format "zzzzz9" with frame f1.
def stream rpt.
output stream rpt to a.txt.
display stream rpt 1000 + i @ i with frame f1.
down stream rpt with frame f1.
display stream rpt i with frame f1.
output stream rpt close.

Please double-check how the int64 vars are output too.

om_upd20130517a.zip has passed runtime regression testing.

Go ahead and check this (0517a) in and distribute it.

One question: in FillIn.java, don't we need to deal with the int64 and decimal "sameness"? It seems like we handle the other possible combinations. Even if this needs refining, it can wait for the next update. I still want you to check in 0517a since it is a major improvement.

From Ovidiu:

-------- Original Message --------
Subject:     2 more issues about datetime implementation
Date:     Tue, 21 May 2013 20:38:45 +0300
From:     Ovidiu Maxiniuc <om@goldencode.com>
To:     Greg Shah <ges@goldencode.com>, Eric Faulhaber <ecf@goldencode.com>

Greg,

I have two more issues about datetime today.

I spent a lot of time fixing datetime and datetime-tz conversion and 
arithmetic operators.
Most of my test involved lots of test-data, which was chosen relatively 
at random, but targeted at important points that was of interest for me: 
year 0, day '0' (12/31/-4714), days round today and 2 digits, windowing 
year, even dates that pass the 32 bit representation. The randomness 
implied various days / time / timezones scattered around those key dates.
I initialized and apply operators on variable and display the result on 
Progress 4GL and using converted code, followed by digit-by-digit 
comparison between the two output, adjusting with 2 hours timezone 
difference (I'm on EEST and the customer's server is on UTC).

There was a little strange thing about conversion from date/datetime to 
datetime-tz as sometimes OE displayed a 60 min offset, sometimes none. 
At first it looked like assigning was keeping the current timezone, and 
using the conversion constructor was not.  More strange, when converting 
backwards, form datetime-tz to datetime, sometimes the timezone offset 
was substracted from actual time (and date) and sometimes not.
This was very annoying since fixing a method in a class looked like 
adding a new difference in the output in a place that was already fixed.

However, I eventually got the cause of this strange issue. Progress is 
counting the *DST as part of the timezone* and computes the timezone 
dynamically. This means that an offset of -05:00 does not identifies an 
exact geographical localization, it can be, for example, Houston with 1 
hour DST either Atlanta in standard time.

The problem in current implementation is that the timezones of any 
date/datetime/datetime-tz used in P2J is computed based on 
TimeZone.getDefault() which is the timezone for current host and DST is 
not taken into consideration at all. The truth is, I could not find any 
word in documentation about how Progress handles DST and there is not a 
word about the fact that in the winter, the "timezone" changes:

DEFINE VARIABLE dtz0 AS DATETIME-TZ.
DEFINE VARIABLE dtz1 AS DATETIME-TZ.

dtz0 = 11/21/2013.
dtz1 = 05/21/2013.

display dtz0 skip dtz1 skip(1).
display add-interval(dtz1, 0, "months") skip add-interval(dtz1, 6, 
"months").

The output is a little strange:
dtz0: 21/11/2013 00:00:00.000+00:00
dtz1: 21/05/2013 00:00:00.000+01:00

21/05/2013 00:00:00.000+01:00
21/11/2013 00:00:00.000+01:00

The second issue is that it seems to me that the default constructor of 
date / datetime and datetime-tz are too heavy used.
They are used for generating the current day and time as implementation 
of NOW and TODAY functions of Progress. This is a little different than 
the way all other data types are initialized. I see that if they remain 
uninitialized, Progress automatically treat them as unknown values. I 
believe we should do the same. I know that there is a lot of TRPL code 
that tests for these kind of data in order to wire the 
instantiateUnknownXXX() function. Which, again, will create new objects 
initialized with now/today values just to set them to unknown before 
returning.

Wouldn't it be easiest and cleaner to add some static methods 
date.today() and datetimetz.now() for implementing respective functions? 
I suppose that there is some historical reason (in older Progress 
versions maybe default values for uninitialized date were "today", or 
something like this ?) why the current implementation uses this approach.

However, the problem that I encountered today is also generated by the 
extra work in the default constructor. In some conditions, the client 
hungs when datetime-tz is sent to be displayed. This is because when the 
object is deserialized, the default constructor is used, and then 
readExternal() method is used for restoring its internal value. Here is 
also a reason to keep the default constructor light and the object to 
unknown because anything processed here is lost when reading the object 
from stream. And this is exactly my problem, since the default 
constructor is now constructing a current timestamp it tries to access 
date.getDefaultTimeZone() which will access the context-local workarea 
and attempt to query values back from server before the request is fully 
read from stream, which will eventually lead to client deadlock.

So my question is: is there any reason why the date is implemented this 
way and I cannot visualize?

Regards,
    Ovidiu

Progress is counting the DST as part of the timezone and computes the timezone dynamically. This means that an offset of -05:00 does not identifies an exact geographical localization, it can be, for example, Houston with 1 hour DST either Atlanta in standard time.

Yes, I think they really mean it when they call it a timezone OFFSET. It is truly GMT +/- OFFSET.

The problem in current implementation is that the timezones of any date/datetime/datetime-tz used in P2J is computed based on

TimeZone.getDefault() which is the timezone for current host and DST is not taken into consideration at all.

   public static int getDefaultOffset()
   {
      integer tzAttr = work.obtain().timezoneOffset;
      int     offset = 0;

      if (tzAttr.isUnknown())
      {
         // get the raw offset in millis and convert it to minutes
         offset = TimeZone.getDefault().getRawOffset() / (1000 * 60);
      }
      else
      {
         offset = tzAttr.intValue();
      }

      return offset;
   }

The truth is, I could not find any word in documentation about how Progress handles DST and there is not a word about the fact that in the winter, the "timezone" changes:

Does the offset actually change here (as part of the add-interval) or is this a display artifact based on the same offset interpreted across a DST boundary?

If the offset actually changes, you will have to determine:

1. How do they determine the base timezone? I assume it is based on the current locale or timezone reported by the operating system (in other words, the same as the JVM default). If the offset is changing, one MUST know the timezone in order to know the DST rules (which dates start and end DST and what the DST shift should be).

2. What operations cause the DST shift to be applied? ADD-INTERVAL is one, but there must be other cases too. On the other hand, it seems like there may be cases when it should be ignored (for example, there should be ways of storing historical data such that the offset won't change even if the DST rules are different now than when the data was created.

Wouldn't it be easiest and cleaner to add some static methods date.today() and datetimetz.now() for implementing respective functions?

Yes, it might be better to do it this way. I am open to the possibility.

I suppose that there is some historical reason (in older Progress versions maybe default values for uninitialized date were "today", or something like this ?) why the current implementation uses this approach.

The original reason for the design choice was to be consistent with how the Java Date classes work. These all cause the current date/time to be used if the default constructor is used.

Please note that we have other wrapper classes (e.g. memptr or raw) that do not generate an unknown instance when using the default constructor.

But I can accept the change for the date classes. Go ahead with it.

About note 112:

So the default format seems to be somewhat dynamic, depending on current date-format. We are currently resolving this format statically, at conversion time (in common-progress.rules, <function name="format_for_type">) if types do not match when using DISPLAY @.

If the code does NOT have an explicit format string, then we can provide the default at runtime via a method call that can take the current SESSION:DATE-FORMAT into account. Instead of this:

new Element(d, "99/99/9999 HH:MM:SS.sss", frame0.widgetRef()) 

We could do this:

new Element(d, datetime.defaultFormatString(), frame0.widgetRef()) 

That assumes that the defaultFormatString() actually operates the way we need it to. If that is not appropriate, then we can add a method.

The things are even more complicated. I added some formatting to d variable:

DEFINE VARIABLE d AS DATETIME FORMAT "99-9999-99".

P2J converted correctly. 4GL continued to ignore the given format and the separators, printing

These problems are runtime problems with proper behavior and error handling. In other words, if the code has an EXPLICIT format string, we will try to use it. But the runtime should work/fail the same way it does in the 4GL.

In regard to note 113, I had previously sent this response via email. I am including it here to memorialize it.

In regard to this one, the simplest solution seems to be: parse the literal case at conversion time and emit the constructor for date(double month, double day, double year).  The values would be emitted as int literals and everything should work cleanly and consistently.

In regard to note 115, I think you are right that the gregorian gap is not likely to affect code in apps we are working on right now. Please open a new Bug in Redmine (under the Base Language project), put the full details on note 115 in there (along with any other notes that are not yet in Redmine).

Please set that task as "related to" this one.

In regard to note 116, the "There ARE datetime literals" part:

We can accept P2J being more permissive so long as:

1. The errors/warnings in the 4GL are all at COMPILE time. In other words, if such conditions cannot be encountered at runtime in valid 4GL code, then we don't have to implement the more strict behavior.

2. Our javadoc suitably describes the "feature" and how we deviate, so that future readers will know the exact situation.

We will defer the work related to #2050 and the gregorian gap work on note 115.

What is left to do on this task before it is otherwise 100% complete?

My fault, I did not reconvert before testing om_upd20130517a.zip. There is a bug in case_statements.rules: fChildType should be String, not Integer. Also, please test the following:

def var i as int64.
case i + 100:
when 1 then message "bla".
when 2 then i = 1.
when 9000000000 then message "bla".
end.

If a branch is outside of int, it needs to convert using IFs, and not Java's switch statement.

Please first fix the fChildType bug in case_statements.rules and release it.

Actually, after discussing this, we need to rollback the case_statements.rules change, as we lose info when .intValue is called for a CASE int64Expr test.

• completely rewrote the DatetimeTzUserType as implements CompositeUserType. It declares two properties { "timestamp", "offset" }, having { TimestampType.INSTANCE, IntegerType.INSTANCE } types respectively.
• mapped the new user type in ImportWorker and DBUtils:
  

  TYPES.put(datetimetz.class, new CompositeCustomType(new DatetimeTzUserType()));
  TYPES.put(DatetimeTzField.class, new CompositeCustomType(new DatetimeTzUserType()));
  

• created a new dedicated template two_column_property_def in hibernate_templates.tpl that is grafted from createProperty function of hibernate.xml if the annotated datatype is "datetimetz".

Now, suppose that we have a datetime-tz field named dtz, when converting

FIND FIRST tt1 WHERE dtz <= NOW.

            new FindQuery(tt1, "lte(tt1.dtz, ?)", null, "tt1.id asc", new Object[]
            {
               datetimetz.now()
            }).first();

Caused by: org.hibernate.QueryException: Expected positional parameter count: 2, actual parameters: [1, 05/24/2013 21:00:27.312+03:00] [select tt1.id from TempRecord1Impl as tt1 where (tt1._multiplex = ?) and (lte(tt1.dtz, ?)) order by tt1._multiplex asc, tt1.id asc]
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:372)
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:328)
        at org.hibernate.internal.QueryImpl.list(QueryImpl.java:97)
        at org.hibernate.internal.AbstractQueryImpl.uniqueResult(AbstractQueryImpl.java:905)
        at com.goldencode.p2j.persist.Persistence.load(Persistence.java:1680)
        at com.goldencode.p2j.persist.RandomAccessQuery.executeImpl(RandomAccessQuery.java:2863)
        at com.goldencode.p2j.persist.RandomAccessQuery.execute(RandomAccessQuery.java:2165)
        at com.goldencode.p2j.persist.RandomAccessQuery.first(RandomAccessQuery.java:849)
        at com.goldencode.p2j.persist.RandomAccessQuery.first(RandomAccessQuery.java:755)
        at com.goldencode.testcases.datetime.DatetimeTestset$1.body(DatetimeTestset.java:167)

new FindQuery(tt1, "tt1.dtz > ?", null, "tt1.id asc", new Object[] { new integer(10), new integer(10) }).first();

Parameter "#4" is not set; SQL statement:
select temprecord0_.id as col_0_0_ from tt1 temprecord0_ where temprecord0_._multiplex=? and temprecord0_.dtz_timestamp>? and temprecord0_.dtz_offset>? order by temprecord0_._multiplex asc, temprecord0_.id asc limit ? [90012-169]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
        at org.h2.message.DbException.get(DbException.java:169)
        at org.h2.message.DbException.get(DbException.java:146)
        at org.h2.expression.Parameter.checkSet(Parameter.java:73)
        at org.h2.command.Prepared.checkParameters(Prepared.java:163)
        at org.h2.command.CommandContainer.query(CommandContainer.java:85)

A second change in generated code:

new FindQuery(tt1, "gte(tt1.dtz, ?)", null, "tt1.id asc", new Object[] { new integer(10), new integer(10) }).first();

Function "GTE" not found; SQL statement:
select temprecord0_.id as col_0_0_ from tt1 temprecord0_ where temprecord0_._multiplex=? and gte((temprecord0_.dtz_timestamp, temprecord0_.dtz_offset), ?) order by temprecord0_._multiplex asc, temprecord0_.id asc limit ? [90022-169]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:329)
        at org.h2.message.DbException.get(DbException.java:169)
        at org.h2.message.DbException.get(DbException.java:146)
        at org.h2.command.Parser.readJavaFunction(Parser.java:2122)

We tried to find some solutions to persist this datatype in database using other workarounds, but because we must go through Hibernate and practically unaware of the backing SQL server it looks like we don't have other choice but to keep the actual 2-column implementation.

Some preprocessing should be done in an early phase of the execution as we cannot always rely on Hibernate's disassemble and assemble mechanisms for even more complex where expressions (like adding to an integer or substracting one dtz from another) which must be implemented as p2j functions in pl/java.

Greg Shah wrote:

What is left to do on this task before it is otherwise 100% complete?

With _POLY issues deferred to #2050, the persistence of datetime-tz is the biggest issue in this task. And looks like HQLPreprocessor is the place where the current issues will be solved.

Any other small bugs will arise, they will be fix as they will be found.

The server project failed compilation:

    [javac] ...: error: no suitable constructor found for SpaceField(int64)
    [javac] ...: error: no suitable constructor found for SkipField(int64)
    [javac] ...: error: no suitable method found for apply(int64)

More, there are compile problems when passing an expression which has an in64 result to an integer param, at a simple function call.

Ovidiu Maxiniuc wrote:

• completely rewrote the DatetimeTzUserType as implements CompositeUserType. It declares two properties { "timestamp", "offset" }, having { TimestampType.INSTANCE, IntegerType.INSTANCE } types respectively.
• mapped the new user type in ImportWorker and DBUtils:

Please provide or point me to your new DatetimeTzUserType implementation (and supporting code). I didn't see it in your latest 2 updates.

Code Review (for 0527b)

I am fine with the update except for one question: why not use writeShort() and readShort() in the writeExternal() and readExternal() of NullField? It seems that is more efficient and reads better too.

After making that change, you can put this into runtime testing OR you can just include it with the other pending changes you are working on.

It looks good.

With this table:

DEFINE TEMP-TABLE tt1
   FIELD dtz  AS DATETIME-TZ
   FIELD ddtz AS DATETIME-TZ EXTENT 2.
CREATE tt1.
ASSIGN
   tt1.dtz = NOW
   /* tt1.ddtz[1] is unknown */
   tt1.ddtz[2] = 2013-05-21t12:34:56.789+10:00.

FIND FIRST tt1 WHERE dtz NE ?.
is converted to
new FindQuery(tt1, "tt1.dtz is not null", null, "tt1.id asc").first();
which is send to SQL:

select temprecord0_.id as col_0_0_ from tt1 temprecord0_ where temprecord0_._multiplex=? and (temprecord0_.dtz_timestamp is not null or temprecord0_.dtz_offset is not null) order by temprecord0_._multiplex asc, temprecord0_.id asc limit ?

Things work fine, too, with
FIND FIRST tt1 WHERE ddtz[2] NE dtz.
which gets converted to:
new FindQuery(tt1, "tt1.ddtz[1] != tt1.dtz", null, "tt1.id asc").first();
which is send to SQL:

select temprecord0_.id as col_0_0_ from tt1 temprecord0_ inner join tt1__2 composite2x1_ on temprecord0_.id=composite2x1_.parent__id where temprecord0_._multiplex=? and composite2x1_.ddtz_timestamp<>temprecord0_.dtz_timestamp and composite2x1_.ddtz_offset<>temprecord0_.dtz_offset and composite2x1_.list__index=1 order by temprecord0_._multiplex asc, temprecord0_.id asc limit ?

However, I cannot see why I get the error message when running
new FindQuery(tt1, "tt1.ddtz[1] != ?", null, "tt1.id asc", new Object[] { datetimetz.now() }).first();
obtained from conversion of
FIND FIRST tt1 WHERE ddtz[2] NE NOW.
The error message (including SQL) is:

Caused by: org.hibernate.QueryException: Expected positional parameter count: 2, actual parameters: [1, 05/29/2013 20:40:36.577+03:00] [select tt1.id from TempRecord1Impl as tt1 join tt1.composite2 as tt1_composite2_1 where (tt1._multiplex = ?) and ((tt1_composite2_1.ddtz != ?) and index(tt1_composite2_1) = 1) order by tt1._multiplex asc, tt1.id asc]
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:372)
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:328)
        at org.hibernate.internal.QueryImpl.list(QueryImpl.java:97)
        ...

new FindQuery(tt1, "tt1.ddtz[1] != (?, ?)", null, "tt1.id asc", new Object[]{
       new java.sql.Timestamp(datetimetz.now().dateValue().getTime()), 
       new java.lang.Integer(datetimetz.now().getOffset())
}).first();

Caused by: java.lang.IllegalArgumentException: Invalid where expression substitution argument:  2013-05-29 21:00:38.079 [0]
        at com.goldencode.p2j.persist.AbstractQuery.validateSubstitutionArguments(AbstractQuery.java:258)
        at com.goldencode.p2j.persist.RandomAccessQuery.<init>(RandomAccessQuery.java:662)
        at com.goldencode.p2j.persist.FindQuery.<init>(FindQuery.java:353)
        at com.goldencode.p2j.persist.FindQuery.<init>(FindQuery.java:180)
        ...

Ovidiu Maxiniuc wrote:

However, I cannot see why I get the error message when running
new FindQuery(tt1, "tt1.ddtz[1] != ?", null, "tt1.id asc", new Object[] { datetimetz.now() }).first();
obtained from conversion of
FIND FIRST tt1 WHERE ddtz[2] NE NOW.
The error message (including SQL) is:

Caused by: org.hibernate.QueryException: Expected positional parameter count: 2, actual parameters: [1, 05/29/2013 20:40:36.577+03:00] [select tt1.id from TempRecord1Impl as tt1 join tt1.composite2 as tt1_composite2_1 where (tt1._multiplex = ?) and ((tt1_composite2_1.ddtz != ?) and index(tt1_composite2_1) = 1) order by tt1._multiplex asc, tt1.id asc]
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:372)
        at org.hibernate.internal.AbstractQueryImpl.verifyParameters(AbstractQueryImpl.java:328)
        at org.hibernate.internal.QueryImpl.list(QueryImpl.java:97)
        ...

As you can see, none of datetime-tz values are expanded.

I wouldn't expect them to be expanded at this stage. The query is not yet expressed as SQL; this error is happening before Hibernate generates SQL. The query in the message above is still HQL; it is what we submitted to Hibernate after augmenting the HQL to deal with our "multiplexed" temp-table implementation (see HQLHelper.preprocessWhere), and rewriting the HQL to deal with the tt1.ddtz[1] extent field reference (see HQLPreprocessor.restructure).

That being said, the message seems misleading to me: it says it's expecting 2 parameters, and then it lists the actual parameters received (2 of them, as expected). Reading the HQL statement, the expectation of 2 parameters looks correct: the first one should be the multiplex ID for the temp table, and the second one should be the datetime-tz value to be compared with tt1_composite2_1.ddtz.

You would need to debug into org.hibernate.internal.AbstractQueryImpl.verifyParameters to find out where things are going wrong (maybe you already have). It looks to me like it is counting the substitution parameter placeholders (?) in the HQL, but is then comparing that number (2, in this case) to the number of backing columns (local variable positionalValueSpan in AbstractQueryImpl.verifyParameters(boolean)) (3, in this case). This doesn't make sense to me at the HQL analysis stage, which I would expect would be about comparing the number of placeholders with the number of actual parameters passed. I would expect the break-out into number of backing columns to happen later, when the SQL is generated.

This looks like a similar issue to [[https://forum.hibernate.org/viewtopic.php?f=1&t=945611]], though unfortunately that was never resolved. It may simply be that passing in an instance of a custom, composite type as a substitution parameter is an unsupported feature in Hibernate, though I don't really understand why -- clearly they have all the necessary information, since they're doing it for the cases of HQL without substitution parameters. If so, this would explain why there was no follow up to the 8-year-old forum entry above.

I think you are on the right track by "manually" changing the HQL, but I think the place to do it is in the HQLPreprocessor (i.e., expanding "?" to "(?, ?)" in the HQL for datetime-tz parameters, and expanding the substitution parameters array to pass in the datetime-tz components separately). Before you go through the effort, though, temporarily remove the type check restriction in com.goldencode.p2j.persist.AbstractQuery.validateSubstitutionArguments and make sure the rest of it works with Hibernate. If so, put back the type check and make the necessary HQLPreprocessor changes.

After a full day of debugging and small changes:
Converted code:

new FindQuery(tt1, "tt1.i != ? and tt1.dtz != ?", null, "tt1.id asc", new Object[] { new integer(1), datetimetz.now() }).first();

select tt1.id from com.goldencode.testcases.dmo._temp.impl.TempRecord1Impl as tt1 where (tt1._multiplex = ?) and (tt1.i != ? and tt1.dtz != (?, ?)) order by tt1._multiplex asc, tt1.id asc

[05/30/2013 21:28:37 EEST] (org.hibernate.hql.internal.ast.HqlSqlWalker:WARN) [DEPRECATION] Encountered positional parameter near line 1, column 107.  Positional parameter are considered deprecated; use named parameters or JPA-style positional parameters instead.

select temprecord0_.id as col_0_0_ from tt1 temprecord0_ where temprecord0_._multiplex=? and temprecord0_.i<>? and temprecord0_.dtz_timestamp<>? and temprecord0_.dtz_offset<>? order by temprecord0_._multiplex asc, temprecord0_.id asc

Later on, when binding parameter values, collectedParameterSpecifications is only composed of the fist two integer typed specfications, so the values for our datetimetz are not binded in Loader.prepareQueryStatement() , as consequence, the Parameter "#4" is not set error is printed.

Unfortunatelly, I was not able to step through Hibernates' hql parser, seems like the sources were not in sync, as the debugger was going jumpy and not making any sense.

No more validation fails at this moment. I will try tomorrow to clear the last part.

When processing multicolumn values there are a few keypoints of interest:

BinaryLogicOperatorNode.mutateRowValueConstructorSyntax(int) line: 130    
BinaryLogicOperatorNode.mutateRowValueConstructorSyntaxesIfNecessary(Type, Type) line: 96    
BinaryLogicOperatorNode.initialize() line: 76    
HqlSqlWalker.prepareLogicOperator(AST) line: 1224    
HqlSqlWalker(HqlSqlBaseWalker).comparisonExpr(AST) line: 4242    
HqlSqlWalker(HqlSqlBaseWalker).logicalExpr(AST) line: 1947    
HqlSqlWalker(HqlSqlBaseWalker).logicalExpr(AST) line: 1900    
HqlSqlWalker(HqlSqlBaseWalker).logicalExpr(AST) line: 1875    
HqlSqlWalker(HqlSqlBaseWalker).whereClause(AST) line: 794    
HqlSqlWalker(HqlSqlBaseWalker).query(AST) line: 595    
HqlSqlWalker(HqlSqlBaseWalker).selectStatement(AST) line: 299    
HqlSqlWalker(HqlSqlBaseWalker).statement(AST) line: 247    
QueryTranslatorImpl.analyze(HqlParser, String) line: 248    
QueryTranslatorImpl.doCompile(Map, boolean, String) line: 183    
QueryTranslatorImpl.compile(Map, boolean) line: 136    
HQLQueryPlan.<init>(String, String, boolean, Map, SessionFactoryImplementor) line: 105    
HQLQueryPlan.<init>(String, boolean, Map, SessionFactoryImplementor) line: 80    
QueryPlanCache.getHQLQueryPlan(String, boolean, Map) line: 168    
SessionImpl(AbstractSessionImpl).getHQLQueryPlan(String, boolean) line: 221    
SessionImpl(AbstractSessionImpl).createQuery(String) line: 199    
SessionImpl.createQuery(String) line: 1735    
Persistence$Context.getQuery(String, int, int, boolean) line: 3997    
Persistence.load(RecordBuffer, String, Object[], Type[], LockType, boolean, boolean) line: 1684    
...

When mutating the vector (temprecord0_.dtz_timestamp, temprecord0_.dtz_offset) != (?, ?) to scalar temprecord0_.dtz_timestamp<>? and temprecord0_.dtz_offset<>? the types should also be expanded. However, at this moment the types (ParameterSpecification) are checked against the children of the current node (!=) and since none of them are ParameterNode-s (1st is a DotNode having our CompositeCustomType, 2nd is a vector with two ParameterNode), both rhs and lhs composite parameter specification are computed as null. When translating the vectors, the current children are lost
along with any chance of expanding the CompositeCustomType type of the 1st child and individually assign to the newly created children.

As consequence, later when generating the sql:

SqlGenerator.out(AST) line: 130    
SqlGenerator(SqlGeneratorBase).sqlToken(AST) line: 1559    
SqlGenerator(SqlGeneratorBase).simpleExpr(AST) line: 2509    
SqlGenerator(SqlGeneratorBase).expr(AST) line: 1383    
SqlGenerator(SqlGeneratorBase).binaryComparisonExpression(AST) line: 2769    
SqlGenerator(SqlGeneratorBase).comparisonExpr(AST, boolean) line: 1211    
SqlGenerator(SqlGeneratorBase).booleanExpr(AST, boolean) line: 860    
SqlGenerator(SqlGeneratorBase).booleanOp(AST, boolean) line: 2665    
SqlGenerator(SqlGeneratorBase).booleanExpr(AST, boolean) line: 840    
SqlGenerator(SqlGeneratorBase).booleanOp(AST, boolean) line: 2690    
SqlGenerator(SqlGeneratorBase).booleanExpr(AST, boolean) line: 840    
SqlGenerator(SqlGeneratorBase).booleanOp(AST, boolean) line: 2670    
SqlGenerator(SqlGeneratorBase).booleanExpr(AST, boolean) line: 840    
SqlGenerator(SqlGeneratorBase).whereExpr(AST) line: 725    
SqlGenerator(SqlGeneratorBase).selectStatement(AST) line: 178    
SqlGenerator(SqlGeneratorBase).statement(AST) line: 111    
QueryTranslatorImpl.generate(AST) line: 232    
QueryTranslatorImpl.doCompile(Map, boolean, String) line: 203    
QueryTranslatorImpl.compile(Map, boolean) line: 136    
HQLQueryPlan.<init>(String, String, boolean, Map, SessionFactoryImplementor) line: 105    
HQLQueryPlan.<init>(String, boolean, Map, SessionFactoryImplementor) line: 80    
QueryPlanCache.getHQLQueryPlan(String, boolean, Map) line: 168    
SessionImpl(AbstractSessionImpl).getHQLQueryPlan(String, boolean) line: 221    
SessionImpl(AbstractSessionImpl).createQuery(String) line: 199    
SessionImpl.createQuery(String) line: 1735    
Persistence$Context.getQuery(String, int, int, boolean) line: 3997    
Persistence.getQuery(Persistence$Context, String, int, int, Object[], Type[], boolean) line: 3873    
Persistence.load(RecordBuffer, String, Object[], Type[], LockType, boolean, boolean) line: 1684    

As consequence, in

QueryLoader.bindParameterValues(PreparedStatement, QueryParameters, int, SessionImplementor) line: 582    
QueryLoader(Loader).prepareQueryStatement(String, QueryParameters, LimitHandler, boolean, SessionImplementor) line: 1757    
QueryLoader(Loader).executeQueryStatement(QueryParameters, boolean, SessionImplementor) line: 1726    
QueryLoader(Loader).doQuery(SessionImplementor, QueryParameters, boolean, ResultTransformer) line: 852    
QueryLoader(Loader).doQueryAndInitializeNonLazyCollections(SessionImplementor, QueryParameters, boolean, ResultTransformer) line: 293    
QueryLoader(Loader).doList(SessionImplementor, QueryParameters, ResultTransformer) line: 2411    
QueryLoader(Loader).doList(SessionImplementor, QueryParameters) line: 2397    
QueryLoader(Loader).listIgnoreQueryCache(SessionImplementor, QueryParameters) line: 2227    
QueryLoader(Loader).list(SessionImplementor, QueryParameters, Set, Type[]) line: 2222    
QueryLoader.list(SessionImplementor, QueryParameters) line: 470    
QueryTranslatorImpl.list(SessionImplementor, QueryParameters) line: 355    
HQLQueryPlan.performList(QueryParameters, SessionImplementor) line: 195    
SessionImpl.list(String, QueryParameters) line: 1247    
QueryImpl.list() line: 101    
QueryImpl(AbstractQueryImpl).uniqueResult() line: 905    
Persistence.load(RecordBuffer, String, Object[], Type[], LockType, boolean, boolean) line: 1703    

The strange thing is that, when prepareQueryStatement(), some information about the parameters is available through queryTranslator.parameterTranslations.ordinalParameters[], even though the information is incomplete, only the the index of the parameter is available and the type for other but the composite types.

How can we fix this?
Apparently we can't. The top of the stack accessible to P2J is Persistence.load() where the accessible object is the query object. We should somehow access the query internal data.
And since this is only a restricted interface this looks like a dead-end.

On the other side, searching the net I found some new information I was not fully aware of:
"Currently, positional parameters cannot be used for multi-column values. You must use named parameters."
I knew that positional parameters are deprecated (and hibernate keeps reminding it in the err console). I am investigating switching to named parameters, but this seems to involve some radical changes in P2J regarding the hql parsing.

I added some small patched to have my CompositeCustomType written as named parameters. Simple equality queries execute fine.
However, not equals (!=) are badly expanded (looks like a bug in exapanding vectors in hibernate):
4GL: tt1.dtz != now
hql: t1.dtz != ?
preprocessed hql: t1.dtz != :px
sql: temprecord0_.dtz_timestamp<>? and temprecord0_.dtz_offset<>?
The sql form is bad as the correct test should have tested temprecord0_.dtz_timestamp<>? *or* temprecord0_.dtz_offset<>? instead.

Another issue is using built-in functions via pl/java.
I have used the simplest available function for datetime-tz: timezone(dtz). As a timezone-tz field is composed by to fields, I created a getTimezone(Timestamp, Integer) corresponding to compounded columns. When running
the sql server will complain about not able to run the function: getTimezone((temprecord0_.dtz_timestamp, temprecord0_.dtz_offset)). A new idea was to create a function with one parameter as they are grouped above in a Object[]. This was also unsuccessful:
he public static Java method was not found: "getTimezone([Ljava.lang.Object;) (com.goldencode.p2j.persist.pl.Functions)"; SQL statement: create alias if not exists getTimezone_2 for "com.goldencode.p2j.persist.pl.Functions.getTimezone([Ljava.lang.Object;)"

Since this is becoming quite complicated, let's take a step back to see if we really need this right now.

The datetime-tz data type is not used in any permanent or temp table field in the current project. It is the type of several metadata field types, including some that are in tables that are in use in the current project (e.g., _File, _Field, _User, _Index, _Index-Field), BUT those fields are in use neither directly in the code, nor referenced by indexes in those metadata tables. So, I think we can work around this type and omit those troublesome fields.

Unless you believe the solution to the above problems is just a few hours of work (from reading your descriptions above, I'm guessing it is not), then let's defer this effort. Please open a separate (but related) issue for the remaining Hibernate problems with datetime-tz. Also relate the task to #1580, since the named parameters work is discussed in that issue's history.

Then wrap up the work you've done on the current issue and let's get it regression tested so you can move on.

Please post your current code, even if not yet complete. I want to start the code review process, since it may take some iterations.

I would also like to see the list of all open/remaining todos/issues.

While testing queries that use pl/java functions I observed that in 4GL, eventual errors are silently discarded, for example

   IF CAN-FIND(dtt WHERE d NE DATE('99-99-99')) THEN ...
   IF CAN-FIND(dtt WHERE INTERVAL(dt, 2013-10-10T10:20:30.400, 'boogey') EQ 10) THEN ..

In P2J, the converted code will look like:

if ((new FindQuery(dtt, "tt1.d != ?", null, "tt1.id asc", new Object[] {
         new date("99-99-99")
     }, LockType.NONE).hasOne()).booleanValue()) { ...
if ((new FindQuery(tt1, "getInterval(tt1.dt, sqlToDatetime('2013-10-10 10:20:30.400'), 'HOURS') = 10", null, "tt1.id asc", LockType.NONE).hasOne()).booleanValue()) ...

** The month of a date must be from 1 to 12. (80)
** Third parameter must be a unit of measure. (11391)

• all methods in Functions / Operators to do the bracketing (save headless status, set to true, execute body, restore headless)
• identify all places where hql using pl/java functions are executed (I think there are only 4 places in Persistence, the very same where getQuery() is called: deleteOrUpdate(), list(), load() and scroll()) and activate headless during the sql execution.
  The second solution looks better.

Beside the pl/java functions some other functions are executed on 'client-side': the evaluation of Object[] args of the FindQuery and the WhereExpression. I don't have a solution for these at this moment.

Ovidiu Maxiniuc wrote:

While testing queries that use pl/java functions I observed that in 4GL, eventual errors are silently discarded, for example

IF CAN-FIND(dtt WHERE d NE DATE('99-99-99')) THEN ...
IF CAN-FIND(dtt WHERE INTERVAL(dt, 2013-10-10T10:20:30.400, 'boogey') EQ 10) THEN ..

will not print any warnings...

In the 4GL, is this behavior only triggered for CAN-FIND, or do you see the same for a FIND statement? If the former, couldn't we better target the solution within FindQuery.has{Any|One}()?

Code Review (0606a and 0607a)

Eric will have to look carefully at the database related parts. Overall, it is very good work. And it is a huge amount of work. Well done!

My feedback:

1. The PSC footer processing should not be in Steam and FileStream classes. These are supposed to be generic classes for processing streams. We rely upon these for 4GL I/O processing. Only in a very rare use case are these classes also used for database import. That processing all needs to be in the ImportWorker (and/or related classes that are specific to the database import). For sure, we want to remove that from FileStream.

2. Even the case where the windowing year and date format are used in Stream is rare. We know it should be used in the database import case, but you also have put it into the regular IMPORT statement use-case. Is that the way the 4GL works (I want to ensure other uses of ReadField are broken)? If so, then it may be OK to put the 2 new data members into the Stream class, but generally we don't want to have to carry them around for all other cases. Do we really need to use a special date constructor? Why not use a special factory method which can just access the windowing year and date format directly?

3. What is the design idea to remove the zone member of date and all related processing? The original idea was that the date processing in Java is inherently timezone-aware. We tried to "neuter" this as much as possible, but when we added datetime and datetimetz support, there is naturally a way that there can be instances that now must be timezone aware instead of neutered. The idea was to ensure that the base class processing that is timezne dependent (from a Java perspective) would also always be consistent with any timezone set in the datetimetz class. By removing the zone member, this seems to no longer be possible. Perhaps it is all still safe, but I am worried that one can get improper results with the new approach.

4. Logic error in date.setTimeSource():

      if (timeSource != null && timeSource.isUnknown())
      {
         setTimeSource("LOCAL"); 
      }
      else 
      {
         setTimeSource(timeSource.toStringMessage());
      }

If timeSource null, this will generate an NPE. I think the code should be: if (timeSource null || timeSource.isUnknown())

There is a similar problem in date.setDisplayTimeZone().

5. For performance reasons, please place the test for !force first in the following (in date.java, datetime.java and datetimetz.java):

   public void assign(date value, boolean force)
   {
      if (ErrorManager.isPendingError() && !force)

The previous idea was that if force == true, then we could avoid ever calling ErrorManager.isPendingError() which is costly because it needs to access context AND (worse) it can even call over to the server side when used from the client. The new approach always calls this. Is there a reason that is needed?

6. In SessionUtils, the exports member needs Javadoc and should be placed at the beginning of the class (according to coding stds, all members are to be placed at the beginning of the class). In addition, the contained initialValue() method also needs Javadoc. Finally, I think the entire body of that method unnecessarily duplicates the code in RemoteObject.obtainInstance(). Just switch to using that instead.

7. What was broken in frame_generator.xml that required the legacyType to be set without the hyphen? I had deliberately set that up so that it would report the compatible result downstream.

8. DateTypeUserType is missing a history entry.

9. DateOps.interval() is very complicated. Wasn't it possible to do more of the math using a Calendar instance?

10. The SessionExports instance should not be needed in LogicalTerminal. I don't want to tie the LT to the SessionExports. The LT is UI code and the SESSION stuff is no specific to the UI. The SessionUtils seems to already handle the obtaining of the instance as needed and that is where it is called anyway. The LT doesn't need or call the SessionExports instance anyway.

11. What about all the instantiateUnknownDate*() calls emitted in convert/variable_definitions.rules that are no longer needed? You made the change for TODAY, NOW and literals, but is seems that the normal initialization is not yet cleaned up.

Again, this is very good work.

3. All date and datetime in 4GL are not localized. Or perhaps, as I found somewhere written, they are localtime. The only place where timezone offset is used is when querying the current date/time (today and now functions). The only interaction with java Date is when interacting with Hibernate in order to persist or fill in parameters of a query. I believe that in these 2 points the TimeZone can be of importance, but using the java default timezone should be sufficient when converting to/from java Date.

4. Right. Really stupid for me. My original implementation was to check only the unknown case. To be in "safe" side, I tried to quickly patch the condition for null values. But nulls are really unlikely to came as actual arguments.

5. I was not aware of the computation effort of that getter. I changed it back to original.

6. Working on this.

7. You get java.lang.RuntimeException: Attempt to set invalid data type datetime-tz when running the converted

      DISPLAY 2013-10-10T10:20:40.500+10:00.

8. added

9. No. All Calendar put at our disposal are additive operations. I heard about Joda Time a library that supposedly would be useful for date-time operations, but I preferred our own implementations as some 4GL "features" might not be exactly emulated by this new library.

10. Working on this, too.

1, 2, 11. Changes are on the way.

I am reviewing the database changes in 20130606a. Although I'm not finished, I see one change so far that is likely going to be a very big problem, in where_clause_pre_prep.rules.

I have tried in the past using ne/neq/etc. PL/Java operator UDFs instead of the native HQL/SQL operators, but it destroyed database performance and I had to back it out. The problem is, these UDFs are not transparent to the query optimizer (at least not in PostgreSQL), and so the correct indexes cannot be selected for most queries, making them take orders of magnitude longer, because everything becomes a full table scan. The queries technically still work, but a real application becomes unusable. You won't see this effect with test cases that use small data sets, because for small tables, a table scan generally will be used anyway. The problem occurs when you have tables with lots of rows, where indexes become critical for performance.

What is the reason for this change? If it is specific to the new data types, it needs to be much more limited in scope, but better to remove it completely.

It was an attempt to implement the datetime-tz operators. The initial idea was to handle (only) the two column datetime-tz relational ops with UDFs. The default hibernate implementation (a.timestamp = b.timestamp and a.offset = b.offset) is wrong; the correct compare operations need both column to be used to bring a and b values to same offset before comparing the timestamps.

However, the 20130606a update contains the "disabled" version, as you can see the server_op annotation is set to false:

<action>putNote("server_op", false)</action>

Ah, good point, I didn't read carefully enough, but the comments are misleading. Since this is the only change (and it's not really a change from the previous, default behavior), this rule set probably should not be part of the update.

• There is an exc.printStackTrace() debug call left behind at line 1800 in Persistence. Was this triggered by the flush mode change earlier in the method?
• Also in the Persistence class...is your change to getQuery finished? It looks a bit like test code that you haven't finalized yet.
• DatetimeTzUserType: please complete javadoc for replace method.

The rest of the update looks good to me, from a database perspective.

• There is an exc.printStackTrace() debug call left behind at line 1800 in Persistence. Was this triggered by the flush mode change earlier in the method?

Don't think so. Most probably this is a left-over from the Hibernate debugging session.

• Also in the Persistence class...is your change to getQuery finished? It looks a bit like test code that you haven't finalized yet.

This is a temporary implementation until the named parameters will be implemented in the entire project.

• DatetimeTzUserType: please complete javadoc for replace method.

Ok.

Code Review

I am good with all the non-database stuff. Eric will have to review the DB parts.

My only remaining concerns:

1.

7. You get java.lang.RuntimeException: Attempt to set invalid data type datetime-tz when running the converted

DISPLAY 2013-10-10T10:20:40.500+10:00.

because the type of widget created has the type set to "datetime-tz".
It looked a little strange to me, that's why I kept the original code in comment.

I can see where this is better for our internal runtime use. BUT, I am pretty sure that the following code will now fail:

if my-widget:DATA-TYPE eq "datetime-tz" then message "GOOD!".

create fill-in my-handle assign DATA-TYPE = "datetime-tz".

Perhaps we can do a fix-up in GenericWidget.getDataType() and the GenericWidget.setDataType()?

Once that code is fixed, the comment can be removed from frame_generator.xml since it is now misleading.

2. Please check the hand coded Majic Java source for instances where they use the default date() constructor (for TODAY) and also for instantiateUnknownDate*() calls. I don't know if they are used for sure, but it is possible.

There are no differences in the files zipped into 0619a.

I am fine with the 0619a changes. Eric still needs to finish his review of the DB parts.

How is the conversion testing going?

Code review 20130618a (no DB content in 20130619a):

• where_clause.rules: lines 340 and 353 are commented out. If this is a final change, please remove them.
• ImportWorker: I didn't think of this in my previous review, but it seems like we will have a problem if the export files do not have the PSC footer, since I did not see where we fall back to the original approach of using the Configuration class (more on this below).
• This is a relatively minor issue, but I noticed it in several places, so it's worth mentioning: why are you adding blank lines in the middle of a block of import statements in some files? Is your IDE doing this automatically? I have noticed that sometimes eclipse inserts import statements that I do not want and generally messes with the import statement format when I cut and paste code.
• Functions.java: please remove the commented-out methods at lines 3005 and 3071.
• Persistence.java: minor point, but I'm looking at diffs since last time, so it jumped out at me: why move the comment at line 1686?

Regarding the PSC footer in the export files, I went back and saw that you previously wrote:

Some of these settings are incorrectly read from come Configuration file.

This approach is not incorrect, it was intentional. Did you find a problem with the implementation? It seemed to work well for M3. Forgetting to update the configuration file may certainly cause a problem, but that is up to the person running the import. You also wrote:

If the implementation fails to parse the PSC block it will use the defaults for date-format (date-format and year-offset).

I think there should be two fallback levels: the first should be to the original approach, the second to hard-coded defaults. Otherwise, we may have to make a code change to deal with missing footers, rather than just a configuration change.

BTW, our first production environment was missing the footers, so this is a very real scenario. The customer had written a custom data export program and ran it simultaneously on various subsets of tables on a multi-core system, in order to export their data much more quickly than the Progress Data Dictionary could. However, their program did not write out the footers.

One minor issue exists in the generated code. Here is an example:

diff -r generated/20130625a/src/aero/timco/majic/ap/ApinvR.java generated/20130625b/src/aero/timco/majic/ap/ApinvR.java
75c75
<    date sDate = new date("1/1/00");
---
>    date sDate = new date(date.fromLiteral("1/1/00"));
77c77
<    date eDate = new date();
---
>    date eDate = new date(date.today());

This is not a real problem, it just is an extra constructor than can be removed, I believe.

I am fine with the 0702a changes. How is testing going?

Conversion was fine, there are a lot of changes but they were expected.
The runtime test has hung-up for unknown reasons. I restarted it, and Constantin is keeping an eye on it.

Ovidiu seems to have some LOCALE-related problems too, but they look different than Vadim's. All failed tests log an error which looks like is related to the fact that the baseline screens were not read properly by the harness engine:

Expected '�' (0xFFFD at relative Y 1, relative X 0) and found '┌' (0x250C at relative Y 1, relative X 0).

     [exec] perl: warning: Setting locale failed.
     [exec] perl: warning: Please check that your locale settings:
     [exec]     LANGUAGE = (unset),
     [exec]     LC_ALL = (unset),
     [exec]     LC_PAPER = "en_custom.UTF-8",
     [exec]     LC_MEASUREMENT = "en_custom.UTF-8",
     [exec]     LC_TIME = "en_custom.UTF-8",
     [exec]     LANG = "en_US.UTF-8" 
     [exec]     are supported and installed on your system.

To move on, I've ran the runtime testing with my user and there are errors like this:

ERROR:  java.lang.SecurityException: read on /usr/share/javazi/GMT-04:00

The problem is isolated to a sql like select toInt(current_date);. The root cause is related to the changes in date.dateToLocalMillis, which ends up calling TimeZone.getTimeZone. We've isolated the problem in PL/Java, which fails to execute calls like TimeZone.getTimeZone("GMT-04:00"); strangely, calls like TimeZone.getTimeZone("GMT") do work.

1. changes to date.dateToLocalMillis don't seem to be OK. In previous code, if the zone is null, the passed date instance was converted using the default calendar returned by nullGC.get(). In current code, if null, the zone is initialized with date.getDefaultTimeZone();, and the calendar for that specific zone is used. Problem is: why this Date instance gets associated with the timezone set at the P2J server, considering that the instance originates from the PL/Java, and this I guess uses system timezone?
2. even if we change date.dateToLocalMillis to avoid the TimeZone.getTimeZone("GMT-04:00") problem, we should determine why this fails, as this might bite us in the future, if there are other cases when this construct is needed.

Code Review 0716a

I am OK with this code being checked in.

tc_job_002: failure in step 40: 'Unexpected EOF in actual at page # 1.'

This is known to always fail. Everything should process OK except the report will not match.

tc_job_clock_002: failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

I often see this fail, BUT it should not always fail. If more than 1 regression run has occurred and this has passed one of the runs, then we are OK.

I suspect that there is a problem with the latest bzr revision (10364).

10364 should be fine. It was reviewed carefully and multiple runs determined that only the expected tc_job_02 failure existed in common.

Code Review 0716b

I am fine with the changes, but Eric needs to review them since many affect the persistence layer.

gso_290: failure in step 19: 'timeout before the specific screen buffer became available (Mismatched data at line 10, column 2. Expected 'N' (0x004E at relative Y 10, relative X 2) and found '' (0x0000 at relative Y 10, relative X 2).)'

Please check with Eugenie, who is our expert in these matters. You may have to run another time to confirm. When you run again, please do run on top of revision 10364.

Code review 20130716a/b:

The database content of this update generally looks good, with a few issues:

• Use of ErrorManager from ImportWorker$DataFileReader is problematic, I think. ErrorManager relies on the P2J server, which is not running during database import. Use ImportWorker.log for reporting errors instead.
• The no-argument variant of the mtime() function in a where clause should not convert to a server-side function, but rather to a query substitution parameter. This will simplify the HQLPreprocessor (with respect to the workaround for no-arg PL/Java functions), and getMTime() can be dropped from Functions and p2jpl.ddr.
• In FindQuery, you made the ErrorManager effectively headless for all CAN-FINDs for PL/Java support, but converted CAN-FINDs can also run on the client (i.e., inside the P2J application server) when they're not embedded in a where clause. Have you confirmed the changed behavior is correct in this case?
• There is an undocumented change to Int64UserType (just an @Override annotation).

The no-argument variant of the mtime() function in a where clause should not convert to a server-side function, but rather to a query substitution parameter. This will simplify the HQLPreprocessor (with respect to the workaround for no-arg PL/Java functions), and getMTime() can be dropped from Functions and p2jpl.ddr.

One question here: isn't it possible that in a table with many rows, that the 4GL implementation may return different values from mtime()? The no arg version reports the number of milliseconds since midnight and on a large table (millions of rows), it seems like this will potentially be different over the course of the query. In this case, it seems like this must be server side instead of resolving to what is a constant if we make it a substitution parm.

I know what you're thinking: "who could have possibly encoded a dependency like that?". And you know what we always find out (in the end) as the answer to that question.

Eric, thoughts?

My recommendation was spurred by Ovidiu's deprecation of Functions.getMTime() because the database server could produce a different time than the P2J application server. You may be right, but haven't we already defeated this with something like AdaptiveQuery, where we execute the query fewer times than the 4GL does?

I just heard from Eric. He is getting on a plane right now and doesn't have the full answer on my question. But we have decided to leave the mtime() implementation as you have coded it for now. Please address the other concerns.

• tc_job_002: failure in step 40: 'Unexpected EOF in actual at page # 1.' is still there
• gso_290: failure in step 19 has disapeared
• tc_job_clock_002: failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
• tc_job_clock_005: failure in step 23: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

The last two are pretty much the same. Also I cannot cannot reproduce either of them, I always get the following error:

+------------------------- Error -------------------------+
| ERROR: Cannot post labor during employee's lunch break. |
| ------------------------------------------------------- |
|                          <OK>                           |
+---------------------------------------------------------+

tc_job_clock_002: failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
tc_job_clock_005: failure in step 23: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'

These two errors are frequently happening when starting tests in a "too early" time. As I can see the time was ~ 07:03. This is too early for these tests to be passed. I don't remember if I saw them passed at that time. I guess this is wrong failure tests. The only way to check - run once again in better time(~ after 14:00 at devsrv01 local time).

And it is really very difficult to retest them manually. You will need a chronograph and execute some preliminary steps.

gso_290: failure in step 19 has disapeared

This is another case of wrong failure. It happens very rare. But this test can be easily reproduced. On my memory this was always confirmed as passed.

tc_job_002: failure in step 40: 'Unexpected EOF in actual at page # 1.' is still there

I need more info. What is the file tc_job_002.txt looks like in your test?
However I guess according the Constantin's testing doc:

...
- "unix2dos" is used throughout MAJIC, but its usage is broken. At least one test is known to fail,
because of this command: the report for test tc_job_002.
...

My preliminary conclusion - we can consider there are no regressions in these cases.

The tc_job_002 failure is at the expected step (report comparison). I think the unix2dos is only part of the problem. If I recall properly, Constantin found that this test also only properly runs on a certain weekday (I don't know which one). This test is OK.

Ovidiu: was this testing based on a version with Eric's requested changes? If so, please upload it and you can distribute it/check it in.

If not, then I think we still want you to check in your changes and distribute them. But we will need you to make Eric's changes and get them tested. But you would do that in a separate update.

What do you think?

Strange enough, my tc_job_002.txt has 58K and looks like a weekly table starting with current DoW (each of my previous text shifts the columns in this file). Instead I see that the reference majic_baseline/reports/tc_job_002.txt is empty. Is it possible that this file got trimmed at a moment ?

Back to the update.
I did not finished integrating Eric's last review because of the 3rd point.
I believe that it is best to check in now the changes as this is another big update and delaying this will need additional merging work if other updates will overlap it.
A future update will address the unfinished issues.

I believe that it is best to check in now the changes as this is another big update and delaying this will need additional merging work if other updates will overlap it.
A future update will address the unfinished issues.

Agreed. Please check in and distribute both updates.

The om_upd20130716a.zip has been committed as bzr revision 10365.

The om_upd20130716b.zip has been committed as bzr revision 10366.

Both updates have been distributed to the team.

Am I correct in my understanding that there is no remaining work in this main task (all remaining work is in separate subtasks)?

Greg Shah wrote:

Am I correct in my understanding that there is no remaining work in this main task (all remaining work is in separate subtasks)?

That is correct.
The main issue IMHO is Feature #2156: "datetime-tz and hibernate".

Greg/Ovidiu,

With revision 10385, there is a case which doesn't convert properly. It looks something like this:

function f1 returns int (input i as int, input j as int).
end.

def temp-table tt1 field f1 as int.
find first tt1.

f1(tt1.f1 + 10, tt1.f1 + 20).

f1(new integer(plus(tt1.getF1(), 10)), new integer(plus(tt1.getF1(), 20)));

f1(plus(tt1.getF1(), 10), plus(tt1.getF1(), 20));

Any idea where I should look?

Constantin,
please remove the xml that caches the functions signature.
You probably had an f1 function with int64 as 1st param and now P2J uses the old signature.

Ovidiu, name_map.xml was removed before the build. Anyway, I've isolate the problem to calls like:

def var i as int.
def var j as int.
f1((tt1.f1 + 10), tt1.f1 + 20).
f1((i + 10), j + 20).

    <ast col="0" id="3582002724959" line="0" text="assignment" type="ASSIGNMENT">
      <ast col="0" id="3582002724960" line="0" text="expression" type="EXPRESSION">
        <annotation datatype="java.lang.Long" key="peerid" value="3594887626842"/>
        <ast col="1" id="3582002724961" line="17" text="f1" type="FUNC_INT">
          <annotation datatype="java.lang.Long" key="oldtype" value="2343"/>
          <annotation datatype="java.lang.Long" key="refid" value="3582002724867"/>
          <annotation datatype="java.lang.Boolean" key="ignorecast" value="true"/>
          <annotation datatype="java.lang.String" key="casttype" value="integer"/>
          <annotation datatype="java.lang.Long" key="peerid" value="3594887626843"/>
          <annotation datatype="java.lang.Boolean" key="wrap" value="true"/>
          <ast col="4" id="3582002724963" line="17" text="(" type="LPARENS">
            <annotation datatype="java.lang.String" key="chp_wrapper" value="integer"/>
            <ast col="7" id="3582002724964" line="17" text="+" type="PLUS">
              <annotation datatype="java.lang.Long" key="peerid" value="3594887626844"/>
              <ast col="5" id="3582002724967" line="17" text="i" type="VAR_INT">
                <annotation datatype="java.lang.Long" key="oldtype" value="2343"/>
                <annotation datatype="java.lang.Long" key="refid" value="3582002724911"/>
                <annotation datatype="java.lang.Long" key="peerid" value="3594887626845"/>
              </ast>
              <ast col="9" id="3582002724969" line="17" text="10" type="NUM_LITERAL">
                <annotation datatype="java.lang.Boolean" key="use64bit" value="false"/>
                <annotation datatype="java.lang.Long" key="peerid" value="3594887626846"/>
              </ast>
            </ast>
          </ast>
          <ast col="16" id="3582002724970" line="17" text="+" type="PLUS">
            <annotation datatype="java.lang.String" key="chp_wrapper" value="integer"/>
            <annotation datatype="java.lang.Long" key="peerid" value="3594887626848"/>
            <ast col="14" id="3582002724972" line="17" text="j" type="VAR_INT">
              <annotation datatype="java.lang.Long" key="oldtype" value="2343"/>
              <annotation datatype="java.lang.Long" key="refid" value="3582002724922"/>
              <annotation datatype="java.lang.Long" key="peerid" value="3594887626849"/>
            </ast>
            <ast col="18" id="3582002724975" line="17" text="20" type="NUM_LITERAL">
              <annotation datatype="java.lang.Boolean" key="use64bit" value="false"/>
              <annotation datatype="java.lang.Long" key="peerid" value="3594887626850"/>
            </ast>
          </ast>
        </ast>
      </ast>
    </ast>

Seems like the wrapping annotation is put on the wrong node that is later removed.
I will dig into this tomorrow in the morning.

I'm fine with the change. My only question: what is the purpose of the nodeRef.numImmediateChildren == 1 test as part of the while expression?

My intention was to identify nodes of this type: (E), ie. an expression formed by another one (E) between unneeded parenthesis so the LPARENS has only one child, E, and to reccursively annotate the inner node (usually up to an operator).
But now, that you mentioned it, I realized that LPARENS cannot have multiple immediate children. If it would had (something like (E1, E2) ?), it would be some function call or some incorrect syntax that should have already been detected.
I will remove the extra test as it will always be valid.

Good. You can check it in and distribute it.

Just noticed that Constantin has committed some changes in the same file, in a very close area (consecutive line numbers).
I would like to be on the safe-side. I will re-run the test tomorrow and if no changes I will check it in and distribute it.

Conversion test ended with success.
Committed revision 10389 and distributed by mail.