Project

General

Profile

Bug #10391

Comparisons methods not returning the correct result in poly cases

Added by Paul Bodale 12 months ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Paul Bodale
Target version:
-
Start date:
Due date:
% Done:

100%

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

reg.patch Magnifier (4.54 KB) Șerban Bursuc, 10/30/2025 09:34 AM

History

#1 Updated by Paul Bodale 12 months ago

This is the case I found:

DEFINE TEMP-TABLE tt NO-UNDO
   FIELD field1 AS CHARACTER
   FIELD field2 AS INTEGER.

DEFINE VARIABLE h AS HANDLE.
h = TEMP-TABLE tt:HANDLE.

h:DEFAULT-BUFFER-HANDLE:BUFFER-CREATE().
h:DEFAULT-BUFFER-HANDLE:BUFFER-FIELD("field1"):BUFFER-VALUE() = "12345".

IF h:DEFAULT-BUFFER-HANDLE::field1 > 0 THEN
   MESSAGE "TRUE".
ELSE
   MESSAGE "FALSE".

IF TEMP-TABLE tt::field1 > 0 THEN
   MESSAGE "TRUE".
ELSE
   MESSAGE "FALSE".

Both of them should print "TRUE" but FWD prints "FALSE".

The problem comes from the com.goldencode.p2j.util.Text#compareTo metod which returns -1 if the right operand is not a String or a Text instance.

#3 Updated by Paul Bodale 12 months ago

  • Subject changed from _isGreaterThan method not returning the correct result in poly case to Comparisons methods not returning the correct result in poly cases

Greg/Ovidiu I would like your input on this.
I think that what is happening in background is that the character operand is converted to integer. I tried both ways and as long as the character variable represents a valid number, the result is based on the number represented. If the character variable doesn't hold a valid number then, progress always returns FALSE for the < and <= operators and TRUE for the > and >= operators.

This problem was initially discovered on the > operator but I also tested with the others and they have the same problem.

#4 Updated by Ovidiu Maxiniuc 12 months ago

That is an interesting observation. A true detective eye! On this project we are bound to back box principle. If we find some testcases which differs from what we have, we need to adjust the adjust the implementation, taking care not to break the existing tests.

Therefore, please prepare the testcase for the tests you mentioned. Those from 1st note are just one of the side of the problem. If you write them using the recommended code style, we can add them to testcase project. Then, adjust the implementation of Text.compareTo() method and we will regressing test it.

#5 Updated by Paul Bodale 11 months ago

This thing goes a little bit deeper than expected. I made the necessary changes in > implementation for NumberType instances other than decimal and it worked. I assume that a similar thing would need to be done for the other ones but the real problem is with the decimals.

Found some weird cases which I'm still working on but the behavior is different depending on the separator used. Both . and , are allowed but they produce different results. I'll post more details here as I move forward.

#6 Updated by Paul Bodale 11 months ago

  • % Done changed from 0 to 20
  • Status changed from New to WIP

Greg/Ovidiu, please advise.

This case:

IF -1.0 > -1 THEN
   MESSAGE "TRUE".
ELSE
   MESSAGE "FALSE".

Prints FALSE.

And this case:

FUNCTION f RETURNS CHARACTER ():
   RETURN "-1.0".
END FUNCTION.
IF DYNAMIC-FUNCTION("f") > -1 THEN
   MESSAGE "TRUE".
ELSE
   MESSAGE "FALSE".

Prints TRUE.

If I instead reference a temp-table record buffer with TEMP-TABLE tt::f the result is the same.

Another thing I've noticed is that both . and some cases of , decimal separator formats are supported no matter the system setting.

I think we need to have a different method that is generated strictly for poly cases which would provide the behavior needed for this.

#7 Updated by Paul Bodale 11 months ago

I committed the tests I wrote for this issue on the testcases project. They are located in the tests/base_language/expressions/operators/ folder. You can take a look for yourself to see what I mean by weird cases.

#8 Updated by Ovidiu Maxiniuc 11 months ago

Paul,
The first "if" execute a the compare operation after applying a default widening cast from int / int64 to decimal. The values are equals, therefore, gt operator returns false. This is clear.

What I think happens in the second testcase is, due to dynamic component, the left operand is converted to a string. In this condition, we compare "-1.0" returned by dynamic function against "-1". Lexicographically, "-1" <"-1.0", or "-1.0" > "-1", leading TRUE to be printed.

I do not know how to interpret the sentence about tt::f. What is the type of f? Which side of the > operator is replaced? If it replaced the left side and its type is int64, int or decimal and value -1, I expect the result to be FALSE because the 'common denominator' type would lead to same logic as in your 1st sentence. If it is a character field, the logic from dynamic-function case will govern the compare operation.

You need to do more tests and confirm if I am right, or infirm my conjecture and find a suitable explanation.

#9 Updated by Paul Bodale 11 months ago

  • % Done changed from 20 to 80

Committed rev. 16109 on branch 10391a that introduces a fix for the behavior of the comparison operators in poly cases. This fix uses the existing implementation of the operands (from the CompareOps class).

I did some more investigation on this and it turns out that it was the complete opposite of what I initially thought. If one of the operands is text, then the comparison is done in lexicographic order. I ran this fix on the existing tests and all of them passed.

I will now focus on the other poly cases in which the dynamic operand gets wrapped in the non-dynamic operands type and also the cases where both operands are poly.

#10 Updated by Constantin Asofiei 11 months ago

Please check also when right-side operand is POLY, like if -1 < DYNAMIC-FUNCTION("f") - does this use integer as comparison or character type?

#11 Updated by Paul Bodale 11 months ago

Constantin Asofiei wrote:

Please check also when right-side operand is POLY, like if -1 < DYNAMIC-FUNCTION("f") - does this use integer as comparison or character type?

This case is included in the tests and it uses character comparison.

#12 Updated by Paul Bodale 11 months ago

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

Committed rev. 16109 on branch 10391a that introduces a fix for the behavior of the when one of the comparison operator when one of the operands is representing a decimal value. Also, cases when one or both of the operands are a DYNAMIC-FUNCTION call are correctly not wrapped anymore.

I'm not sure if this was also happening for DYNAMIC-INVOKE or other poly cases but with the current changes I was not able to find a case where the poly argument would get wrapped in a BDT constructor in the converted code.

I've noticed that the result of the comparison operators depends upon the numeric format of the session. After I adjusted the program that I used to test the behavior all the comparison pass with the correct result.

#13 Updated by Paul Bodale 11 months ago

  • Assignee set to Paul Bodale

Sorry, I forgot about this.

#14 Updated by Constantin Asofiei 11 months ago

  • Status changed from Review to Internal Test

Paul, the changes look good. Please add history entries.

#15 Updated by Paul Bodale 11 months ago

Committed rev. 16111 on branch 10391a that addresses this feedback.

I also committed the procedures I used to test the behavior of comparison operators in the testcases project. They are located in tests/base_language/expressions/operators/procedures

#16 Updated by Constantin Asofiei 9 months ago

  • Status changed from Internal Test to Merge Pending

Paul, please merge 10391a now.

#17 Updated by Paul Bodale 9 months ago

  • Status changed from Merge Pending to Test

Branch 10391a was merged into trunk as rev. 16229 and archived.

#18 Updated by Constantin Asofiei 9 months ago

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

Paul - please work on a standalone testcase for #7143-1723 and lets discuss it.

#19 Updated by Paul Bodale 9 months ago

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

Created task branch 10391b and committed rev. 16251 that fixes the regression found in #7143-1723

#20 Updated by Constantin Asofiei 9 months ago

Please add history entries.

#21 Updated by Greg Shah 9 months ago

+Eugenie

Paul: Please update Open Regressions in FWD v4 to add a entry for this issue.

#22 Updated by Paul Bodale 9 months ago

Constantin Asofiei wrote:

Please add history entries.

Sorry for that. Committed rev 16252 on branch 10391b.

Greg Shah wrote:

+Eugenie

Paul: Please update Open Regressions in FWD v4 to add a entry for this issue.

Done.

#23 Updated by Constantin Asofiei 9 months ago

Alexandru: we need this in trunk if we are making a release today.

#24 Updated by Alexandru Lungu 9 months ago

Alexandru: we need this in trunk if we are making a release today.

I will be doing a delivery today. Please focus on Review and Test. I am freezing trunk now for delivery.

#25 Updated by Alexandru Lungu 9 months ago

Serban, can you confirm that 10391b is fixing the OG issue in the customer application?

#26 Updated by Șerban Bursuc 9 months ago

Alexandru Lungu wrote:

Serban, can you confirm that 10391b is fixing the OG issue in the customer application?

If the branch contains changes from this patch then yes. I tested with this patch and the issue was solved.

#27 Updated by Paul Bodale 9 months ago

Șerban Bursuc wrote:

Alexandru Lungu wrote:

Serban, can you confirm that 10391b is fixing the OG issue in the customer application?

If the branch contains changes from this patch then yes. I tested with this patch and the issue was solved.

The changes on the 10391b branch are exactly the ones from the patch I sent + history entries. The testing was done before committing the solution.

#28 Updated by Alexandru Lungu 9 months ago

  • Status changed from Review to Merge Pending

Please merge 10391b to trunk now.

#29 Updated by Paul Bodale 9 months ago

  • Status changed from Merge Pending to Test

Branch 10391b was merged into trunk as rev. 16254 and archived.

#30 Updated by Eugenie Lyzenko 8 months ago

Paul,

I have a question for branch repos on devsrv01.

Did you create 10800a branch? What was the purpose?

I tried to make new branch to fix #10800 issue but I see the 10800a branch already exists. But to fix another issue. What is the current status of the 10800a branch? Are you going to commit something to this?

Or it was a mistake to create 10800a and we need to remove it? And then create 10800b to commit #10800 related fixes?

Please clarify what is going on with 10800a branch.

#31 Updated by Paul Bodale 8 months ago

I don't think I created a 10800a branch. Didn't know about it until now

#32 Updated by Eugenie Lyzenko 8 months ago

Paul Bodale wrote:

I don't think I created a 10800a branch. Didn't know about it until now

../10800a$ bzr log -c16254
------------------------------------------------------------
revno: 16254 [merge]
committer: Paul-Beniamin Bodale <pbb@goldencode.com>
branch nick: trunk
timestamp: Thu 2025-10-30 13:46:19 +0000
message:
  Make sure that comparison is done with the actual value and not with a proxy. (Ref. #10391)
------------------------------------------------------------

#33 Updated by Eugenie Lyzenko 8 months ago

Paul Bodale wrote:

I don't think I created a 10800a branch. Didn't know about it until now

OK, never mind. I have updated 10800a to sync with latest trunk.

#34 Updated by Paul Bodale 8 months ago

I checked out the branch locally just to be sure. The only thing I understand from #10391-32 is that the branch was made right after I committed the changes from this task.

#35 Updated by Eugenie Lyzenko 8 months ago

Paul Bodale wrote:

I checked out the branch locally just to be sure. The only thing I understand from #10391-32 is that the branch was made right after I committed the changes from this task.

OK. Seems like the assistance from little helpers of Santa <g>. But not from you and me.

#36 Updated by Constantin Asofiei 6 months ago

  • Status changed from Test to Closed

Also available in: Atom PDF