FWD » Core Development » Database

Regression from trunk/r16278 (10729a merge), see #10729-149.

When FQLPreprocessor rewrites param < field / param <= field, it swaps the operands but keeps the operator. The query then runs field < param.

Example: indexed tt.f1 with values 1, 2, ? and v = 1:

Proposed fix: keep the original operand order when rebuilding the comparison.

=== modified file 'src/com/goldencode/p2j/persist/FQLPreprocessor.java'
@@ -6138,10 +6139,8 @@
-               HQLAst oppositeOperand = (HQLAst) ((next.getIndexPos() == 0)
-                                                 ? next.getNextSibling()
-                                                 : next.getPrevSibling());
-
+               boolean substFirst = (next.getIndexPos() == 0);
+               HQLAst oppositeOperand = (HQLAst) (substFirst ? next.getNextSibling() : next.getPrevSibling());

                oppositeOperand.remove();
@@ -6153,8 +6152,9 @@
                next.remove();

-               compareOperator.addChild(oppositeOperand);
-               compareOperator.addChild(next);
+               // keep the operand order, the operator is not mirrored
+               compareOperator.addChild(substFirst ? next : oppositeOperand);
+               compareOperator.addChild(substFirst ? oppositeOperand : next);
                oppositeOperand.setParent(compareOperator);
                next.setParent(compareOperator);

This fixes tests/query/TestJoinQueries: testOuterLessThanInner, testOuterLessterLessThanInner.

Comparisons against the unknown value itself are wrong both with and without the patch, so they need a separate fix: ? > f1, ? >= f1, f1 < ?, f1 <= ?, and v < f1 / v > f1 when v = ?.

I am ok with the changes, but I think there are still some cases that are left unfixed. These come from two places in FQLPreprocessor.simplifyUnknownsOrInjectNullCheck:

1. f > ? and ? < f: for an indexed field the code picks the IS_NULL*1000+GT / +LT rewrite without checking whether the parameter is unknown. OE returns nothing above unknown, so with an unknown parameter the result should be false.
2. f < ? and ? > f: the code first picks field is not null, which matches OE, then replaces it with true when the field is indexed. That contradicts the method's own javadoc, which says t.f < ? is equivalent to t.f <> ?.

Also, please test tables without indexes (e.g. queries that end up with tt.recid asc in the order by).

Stefanel Pezamosca wrote:

Not covered:
Comparisons against the unknown value itself (? > f1, f1 < ?, or v < f1 when v = ?) behave the same as on trunk. They seem related to what #11108 is working on.

Stefanel, I rebased 11108c. Can check these cases with the 11108c?

PS:

Comparisons against the unknown value itself (? > f1, f1 < ?, or v < f1 when v = ?) behave the same as on trunk. They seem related to what #11108 is working on.

My bad, I did not spot these. I am ok with the changes considering the limitations.

PS: we will need to find out the customer branches that are susceptible to this bug and suggest porting this fix.

I tested 11992a rev. 16805 against OpenEdge and against trunk r16804 (11992a with the fix reverse-applied), using the same probes on all three. I think 11992a is safe to merge.

Coverage:

• Field types: integer, character, date and decimal.
• Indexes: ascending, descending, two-field (leading equality, non-leading component), unique, and a table with no index.
• Comparisons: all 8 v op f / f op v forms with <, <=, > and >=, plus = and <>, with variables and literals, combined with AND/OR.
• Statements: FOR EACH with and without BY, FIND FIRST/LAST/NEXT/PREV, CAN-FIND, and joins (EACH, FIRST, LAST).
• Dynamic queries: GET-NEXT/GET-PREV and buffer FIND-FIRST/FIND-LAST.

These branches have the bug: 10614_main, 8755d, 9866e (d?), 11748a.

The 11992a commit needs createSubtree() (trunk r16787+), so we need to use this minimal patch instead. It applies cleanly to all four, and on trunk r16804 it gives the same results as 11992a.

PatchPatch

--- a/src/com/goldencode/p2j/persist/FQLPreprocessor.java
+++ b/src/com/goldencode/p2j/persist/FQLPreprocessor.java
@@ -6731,10 +6731,8 @@
                      break;
                }

-               HQLAst oppositeOperand = (HQLAst) ((next.getIndexPos() == 0)
-                                                 ? next.getNextSibling()
-                                                 : next.getPrevSibling());
-               
+               boolean substFirst = (next.getIndexPos() == 0);
+               HQLAst oppositeOperand = (HQLAst) (substFirst ? next.getNextSibling() : next.getPrevSibling());

                oppositeOperand.remove();
                HQLAst copyOppositeOperator = (HQLAst) oppositeOperand.duplicate();
@@ -6746,8 +6744,9 @@

                next.remove();

-               compareOperator.addChild(oppositeOperand);
-               compareOperator.addChild(next);
+               // keep the operand order, the operator is not mirrored
+               compareOperator.addChild(substFirst ? next : oppositeOperand);
+               compareOperator.addChild(substFirst ? oppositeOperand : next);
                oppositeOperand.setParent(compareOperator);
                next.setParent(compareOperator);

• 10614_main: Please open a task in customer project and describe the bug. I send you by email the process for the customer.
• 8755d and 11748a: Please port to these task branches now.
• 9866e: Radu, please assess.

Alexandru Lungu wrote:

• 9866e: Radu, please assess.

I agree that this is an important fix, so I added it to the #11981 list.