Project

General

Profile

Bug #9228

FQL to SQL conversion for CONTAINS when the output is expected to be always false is not working corectly for all cases.

Added by Stefanel Pezamosca almost 2 years ago. Updated almost 2 years ago.

Status:
Test
Priority:
Normal
Assignee:
Stefanel Pezamosca
Target version:
-
Start date:
Due date:
% Done:

100%

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

History

#1 Updated by Stefanel Pezamosca almost 2 years ago

  • Status changed from New to WIP
  • Assignee set to Stefanel Pezamosca
The issue is that for some cases CONTAINS parameter can be an empty string.
  • 1. This should be picked up by FQLPreprocessor.fixEmptyContains, There is an issue here, because it is checking only for '', but it should also check for ' ', ' ' etc.
  • 2. For the cases when this is not picked by FQLPreprocessor, it reaches FqlToSqlConverter, and sets the containsRewriteData.alwaysFalse flag to true. There is an issue here for some cases when the converted SQL is wrong.

e.g. The final SQL could look like this:

 mcte2 as (
    select recid from table1
    where (1 = 0)
)
select mcte2.recid from mcte2
   left join wcte1 on (mcte2.recid = wcte1.recid)
Notice the missing WITH statement and wcte1.

#2 Updated by Stefanel Pezamosca almost 2 years ago

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

I have created task branch 9228a from trunk revision 15476.

I committed the fix in 9228a revision 15477. Please review.

#3 Updated by Constantin Asofiei almost 2 years ago

What's the FQL being converted here?

#4 Updated by Stefanel Pezamosca almost 2 years ago

Constantin Asofiei wrote:

What's the FQL being converted here?

The one you discovered while testing with 6667i.

#5 Updated by Constantin Asofiei almost 2 years ago

Was your test with 6667i? Because this code does not check correctly for 'order by' ASTs, in the case when the sort is via upper(rtrim(tt1.f1)):

      List<Aast> orderAsts = orderNode == null ? Collections.emptyList() : 
                                                 orderNode.getImmediateChildren(ALIAS);
      boolean onlySpecial = true;
      for (Aast ast: orderAsts)
      {
         FieldInfo data = new FieldInfo();
         onlySpecial &= generateProperty((FQLAst)ast, false, true, data);
         orderFields.add(data);
      }
      if (onlySpecial && orderFields.size() > 0 && relation == null)

#6 Updated by Stefanel Pezamosca almost 2 years ago

Constantin Asofiei wrote:

Was your test with 6667i? Because this code does not check correctly for 'order by' ASTs, in the case when the sort is via upper(rtrim(tt1.f1)):
[...]

Yes, it was with 6667i.
The original purpuse of this task was to fix the syntax error in the converted sql.

I don't know if I should also fix the orderFields here, or in another task.

#7 Updated by Stefanel Pezamosca almost 2 years ago

I committed to 9228a revision 15478 a fix for the wrong orderFields generation.

Please review.

#8 Updated by Constantin Asofiei almost 2 years ago

Ovidiu, please review.

#9 Updated by Ovidiu Maxiniuc almost 2 years ago

Revision of 9228a, r15478.

I think the code will work fine. I have just a few notes to add:

  • FqlToSqlConverter:
    • line 1340: please insert a single space in front so that dialect starts at the same column with crwd. They both start left and right subexpression operands of || operator, wrapped into ( starting on line 1339;
    • line 1990: the @param tags are incorrect. They seem to be a copies of those from generateOrderByField() method;
    • line 2021: the recursive call actually unwraps the column (from trim/upper functions) which will be used in order by clauses. Although, in the result is the one expected in this case, I think this should be noted in the javadoc: the resulting list of nodes are the 4GL fields used in original query, not the expressions to be used in the SQL to be constructed. These two lists are different.

#10 Updated by Stefanel Pezamosca almost 2 years ago

  • Status changed from Review to Internal Test

Ovidiu Maxiniuc wrote:

Revision of 9228a, r15478.

I think the code will work fine. I have just a few notes to add:

  • FqlToSqlConverter:
    • line 1340: please insert a single space in front so that dialect starts at the same column with crwd. They both start left and right subexpression operands of || operator, wrapped into ( starting on line 1339;
    • line 1990: the @param tags are incorrect. They seem to be a copies of those from generateOrderByField() method;
    • line 2021: the recursive call actually unwraps the column (from trim/upper functions) which will be used in order by clauses. Although, in the result is the one expected in this case, I think this should be noted in the javadoc: the resulting list of nodes are the 4GL fields used in original query, not the expressions to be used in the SQL to be constructed. These two lists are different.

Fixed in revision 15479.

#11 Updated by Constantin Asofiei almost 2 years ago

I've tested ETF and is OK. I think we need to wait for 6667i to be merged, and after that we'll test some other apps, early next week. The ETA to merge this is by Wednesday.

#12 Updated by Constantin Asofiei almost 2 years ago

Stefanel, please go ahead and test other projects. If they are OK, we can queue this for merge.

#13 Updated by Stefanel Pezamosca almost 2 years ago

I had the changes patched to my local trunk for a while now. I didn't encounter any obvious issues while testing.

#14 Updated by Constantin Asofiei almost 2 years ago

  • Status changed from Internal Test to Merge Pending

Stefanel Pezamosca wrote:

I had the changes patched to my local trunk for a while now. I didn't encounter any obvious issues while testing.

Go ahead and merge 9228a now.

#15 Updated by Stefanel Pezamosca almost 2 years ago

  • Status changed from Merge Pending to Test

Branch 9228a was merged to trunk rev 15483 and archived.

#16 Updated by Greg Shah almost 2 years ago

Please update Open Regressions in FWD v4 with the status.

Also available in: Atom PDF