Project

General

Profile

Bug #7999

FWD does not honor FIELDS/EXCEPT at dynamic queries

Added by Constantin Asofiei over 2 years ago. Updated 4 months ago.

Status:
Review
Priority:
Urgent
Assignee:
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
Alexandru Lungu, Lorian Sandu
production:
No
env_name:
topics:

7999.p Magnifier (16.8 KB) Eduard Soltan, 12/18/2025 08:39 AM


Related issues

Related to Database - Feature #2137: runtime support for FIELDS/EXCEPT record phrase options New
Related to Database - Bug #11347: IGNORE_FIELDS_ERROR is thrown in RAQ, extend rereadfields support Review

History

#1 Updated by Constantin Asofiei over 2 years ago

Consider this test:

def var hb as handle.
def var hq as handle.

create buffer hb for table "book".
create query hq.
hq:add-buffer(hb).
hq:query-prepare("for each book fields (isbn) no-lock").
hq:query-open().
hq:get-first().
message hb::book-title. // error

For static queries, FIELDS and EXCEPT can appear only at DEFINE QUERY. In dynamic query case, FWD appends them to the OPEN QUERY statement, from where they get ignored.

More, FWD doesn't convert properly the DEFINE QUERY ... FIELDS case, either:

define query q for book fields (isbn).
open query q for each book.
get first q.
message book.book-title.

This was found in standalone tests.

#2 Updated by Constantin Asofiei over 2 years ago

  • Related to Feature #2137: runtime support for FIELDS/EXCEPT record phrase options added

#3 Updated by Alexandru Lungu over 2 years ago

  • Status changed from New to WIP
  • Assignee set to Eduard Soltan

#4 Updated by Eduard Soltan over 2 years ago

Committed on 7999a, revision 14833.
- DynamicQueryHelper, added methods to traverse fields and except phrases of record phrase of the query string, and set include and exclude variables of AbstractQuery.
- in CompundQuery, added methods to set include and exclude of child components.

#5 Updated by Alexandru Lungu over 2 years ago

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

#6 Updated by Greg Shah over 2 years ago

Ovidiu: Please review.

#7 Updated by Ovidiu Maxiniuc over 2 years ago

Review of 7999a, r14833.

Good job. The goal of the task was reached. But I have some things to note, mainly because of performance. They may not seem as much, but they add to global application speed:

  • CompoundQuery.java:
    • history header: try keeping the information compact. The two entries are highly redundant;
    • there is a common typo in the new method names setIncludedCompoents() / setExcludedCompoents;
    • use an local variable to cache included.get(buffers[j].getDMOProxy()) value used in 2 places;
    • the code in these methods are pretty much identical. I think we can create a parametrised method to merge both of them;
  • DynamicQueryHelper.java:
    • history header: see above. The closing tag */ is broken. Luckily it is closed by the copyright comment;
    • the import list contains new unneeded and unwanted entries;
    • line 650 and 663: the cast to AbstractQuery is not needed, the include() and exclude() methods are declared in P2JQuery interface;
    • the for loops at lines 644 and 657 can be replaces with a bit faster iterations over the Map's entry set. For example:
                  Set<Map.Entry<Buffer, String[]>> kvPairs = include.entrySet();
                  for (Map.Entry<Buffer, String[]> pair : kvPairs)
                  {
                     String[] fields = pair.getValue();
      
                     if (fields != null)
                     {
                        BufferImpl buffer = (BufferImpl) pair.getKey();
                        query0.include(buffer.buffer().getDMOProxy(), fields);
                     }
                  }
      
      The creation and iteration of KeySet and EntrySet are very similar but in the latter case the Map.get() call is replaced by the simple getValue() getter. Also, note the include.get(buffer) was invoked twice in initial code;
    • in methods collectFieldsList() and collectExcludeList():
      • as above they share pretty much of their code so they are candidate for merging into a single one, to avoid code duplication.
      • line 1452: I think the correct code is propList[j] = field.name; (the property name), not fieldAast.getText() (the legacy field name);
      • does collectFieldsList() throw any Exception?
      • the errMsg. Please be sure the message is the same as in OE, including inner spaces and final dot. Please split it on left aligned substrings which do not pass the standard line limit (110);
      • the int nrChildren = chAst.getNumImmediateChildren(); / for (int i = 1; i < nrChildren; i++) / Aast bufParamAast = chAst.getChildAt(i); is not recommended from PoV of performance. getNumImmediateChildren() iterate children to count them, the getChildAt(i) iterates them again. Use instead a iteration starting with getFirstChild() and advance with getNextSibling. This is probably the fastest way. Alternatively, there is getImmediateChild(int type, Aast start) method in AnnotatedAst but it's slower. The same stands for the inner loop;
      • AFAIK, there can be only one KW_FIELD / KW_EXCEPT sub-node of RECORD_PHRASE. In this case it's normal to leave the outer loop as soon as the (first) node was encountered and processed;
      • when computing the bufname. Instead of bufname.contains("."), please compute bufname.lastIndexOf(".") first (returns -1 if the needle is not present in the stack), cache it and reuse in substring.

#8 Updated by Alexandru Lungu over 2 years ago

Eduard, please address the review to get this done.

#9 Updated by Eduard Soltan over 2 years ago

Committed on 7999a, revision 14837.

Addressed review in #7999-7. Also for second case in #7999-1, with DEFINE QUERY ... FIELDS, added support at conversion phase to add include/exclude method call for query variable.

#10 Updated by Alexandru Lungu over 2 years ago

Eduard, I rebased 7999a to latest trunk. It is now at rev. 14958. Please do the testing of it with POC and a large customer application regression tests.
I see changes in database_access.rules. Are these mandatory and require reconversion?

Ovidiu, please review latest 7999a.

#11 Updated by Eduard Soltan over 2 years ago

Alexandru Lungu wrote:

I see changes in database_access.rules. Are these mandatory and require reconversion?

It is required for support of DEFINE QUERY ... FIELDS, because for now a fields/except clause inside a DEFINE QUERY is not taken into consideration at conversion phase. And it requires reconversion.
I tested conversion of Hotel_Gui with this change.

#12 Updated by Ovidiu Maxiniuc over 2 years ago

Review of 7999a/14954-14958.
The code looks promising, most of the issues below are low priority. but there are a couple of things in DynamicQueryHelper which seem unfinished. Has the code been incorrectly merged when rebased?

  • please update the copyright year in file header to all affected files, regardless of the date in H section;
  • database_access.rules:
    • line 420: routines exposed in CommonAstSupport are automatically available to TRPL code, no need to create a special worker. Actually, I have not seen it in use.
    • line 1739: variable name, typo bufnameJavanmae instead of bufnameJavaname ?
  • CompoundQuery.java:
    • typo: setIncludedCompoents, setFieldsCompoents, nrCompoents
    • line 3487-3492: I guess a more expressive (and compact) initialization for fieldsCompMap would be:
      Map<DataModelObject, Property[]> fieldsCompMap = isInclude ? included : excluded;
    • setFieldsCompoents() can be private I think;
  • DynamicQueryHelper.java:
    • line 92: invalid multi-line comment terminator. Does this compile?
    • lines 153, 520, 521, 1309, 1316: rogue empty lines
    • line 1324: @param for token parameter is missing from method javadoc
    • line 1351: we have getImmediateChild(tokenType) but your code is locally optimized to seek a specific token type and I agree with it;
    • line 1368: byLegacyName() requires that the argument to be normalized (lowercase). You may test with an exclude/field name with not matching casing;
    • line 1377: in case of an invalid field name does OE attempt to recover by looking for another list of fields? is it possible to have multiple fields/exclude options?
    • in parse() method, I think something is missing. The include and exclude maps are defined (line 347) and at lines 637/653 they are checked for null. Where is the private method exclude() called?
  • PreselectQuery.java: missing H entry

#13 Updated by Eduard Soltan over 2 years ago

Ovidiu Maxiniuc wrote:

  • line 1368: byLegacyName() requires that the argument to be normalized (lowercase). You may test with an exclude/field name with not matching casing;

I tested with not matching cases, and it works fine with byLegacyName.

  • line 1377: in case of an invalid field name does OE attempt to recover by looking for another list of fields? is it possible to have multiple fields/exclude options?

I checked in OE, it is not possible to have multiple fields/except options.

  • in parse() method, I think something is missing. The include and exclude maps are defined (line 347) and at lines 637/653 they are checked for null. Where is the private method exclude() called?

It is missing, I do not why it was deleted at some point.

 include = collectFields(pAst, buffers, ProgressParserTokenTypes.KW_FIELD);
 exclude = collectFields(pAst, buffers, ProgressParserTokenTypes.KW_EXCEPT);

I tested this changed on POC, and it was causing some regressions:
1) in query 2 consecutive buffers, where referenced to the same persistence table. for each Customer1 fields (name), each Customer2 fields (name), like in this case Customer1 and Customer2 reference the same table Customer.

This issue is solved by changes in FqlToSqlConverter.

2) a issue related to #8259.

Commited on 7999a, revision 14959.

#14 Updated by Lorian Sandu 9 months ago

When the changes from this task are regression tested, please also include the following scenario mentioned in #10763:

def query q1 for a fields (a1).
open query q1 for each a no-lock.
get first q1 exclusive-lock.
message a.a1 a.a2.

We need to verify whether the get first q1 exclusive-lock operation reloads the full record.
This is important because an exclusive-lock on a query defined with a fields clause should trigger a full record fetch, even though the query was originally opened with NO-LOCK.

#16 Updated by Alexandru Lungu 7 months ago

  • Priority changed from Normal to High

Eduard, this task will be required by #10934. I am raising priority. Please rebase 7999a and recap what is left to be done. I think it shall get a second review iteration, right?

#17 Updated by Eduard Soltan 7 months ago

I have rebased the 7999a, to latest trunk. rev. 16321.

Added changes to handle the case when the query AST is retrieved from cache. Plus the #7999-14 is also handled.

#18 Updated by Eduard Soltan 7 months ago

I think the way by which we handle the LOCK type change and all fields retrieval is not right.

We retrieve all fields regardless of the FIELDS/EXCEPT clause, if the LOCK type set by the query is different then LockType.NONE. Which is not correct, because LockType.SHARE lock could also retrieve partial records. Only LockType.EXCLUSIVE lock retrieve the al the fields.

An update of lock does not necessary means that all fields should be retrieved.

NO-LOCK -> SHARE-LOCK - partial fields are retrieved.
NO-LOCK -> EXCLUSIVE-LOCK - all fields are retrieved.
SHARE-LOCK -> EXCLUSIVE-LOCK - all fields are retrieved.
SHARE-LOCK -> NO-LOCK -  partial fields are retrieved.
EXCLUSIVE-LOCK -> NO-LOCK - partial fields are retrieved.
EXCLUSIVE-LOCK -> SHARE-LOCK - partial fields are retrieved.

Things get very fuzzy when 2 buffers to the same table are used:

define buffer book1 for book.
define buffer book2 for book.

def query q1 for book1 fields (isbn), book2 fields (isbn).
open query q1 for each book1 NO-LOCK, each book2.
get first q1.
message book1.book-title book2.book-title.

NO-LOCK on the first buffer will cause retrieval of all fields in both buffers.

open query q1 for each book1, each book2 NO-LOCK.

However NO-LOCK on second buffer does not trigger the same behaviour.

#19 Updated by Alexandru Lungu 7 months ago

Lorian/Artur: please advice on this topic.

#20 Updated by Eduard Soltan 7 months ago

+ added Artur and Lorian as watchers

I guess my question is why we check if the lock is different then LockType.NONE

=== modified file 'src/com/goldencode/p2j/persist/Persistence.java'
--- old/src/com/goldencode/p2j/persist/Persistence.java    2025-08-18 13:28:13 +0000
+++ new/src/com/goldencode/p2j/persist/Persistence.java    2025-11-06 15:39:50 +0000
@@ -685,6 +685,7 @@
 ** 218 LS  20250728          Modified 'tenantChanged' to skip buffer release if the tenant name has not changed.
 ** 219 OM  20250731          Dropped dead code related to old technology no longer in use.
 ** 220 LS  20250722          Changed 'load' to allow skipping full hydration.
+** 221 LS  20251106          Set 'partialFields' to null when the lock is not NO-LOCK.
 */

 /*
@@ -2638,7 +2639,11 @@
       RecordLockContext lockContext = local.getRecordLockContext();
       boolean resetLock = false;
       boolean vstTable = false;
-      
+
+      if (partialFields != null && lockType != LockType.NONE)
+      {
+         partialFields = null;
+      }
       if (temporary)
       {
          updateLock = false;

#21 Updated by Lorian Sandu 7 months ago

I think that can be changed to lockType.isExclusive()

#22 Updated by Eduard Soltan 7 months ago

The changes are in 7999a, rev. 16331.

I have made a set of tests for the case when 2 buffers to the same table are used.

And actually the single case when the records are selected partially is this one:

def query q1 for book1 fields (isbn), book2 fields (isbn).
open query q1 for each book1, each book2.
get first q1.

In rest the records are always fetched fully.

#23 Updated by Alexandru Lungu 7 months ago

  • reviewer Alexandru Lungu, Lorian Sandu added

#24 Updated by Eduard Soltan 7 months ago

Eduard Soltan wrote:

I have made a set of tests for the case when 2 buffers to the same table are used.

And actually the single case when the records are selected partially is this one:

[...]

In rest the records are always fetched fully.

I think this could be handled separately.

For now this branch is ready for review.

#25 Updated by Eduard Soltan 6 months ago

Could you review the branch?

#27 Updated by Teodor Gorghe 5 months ago

  • Priority changed from High to Urgent

#28 Updated by Eduard Soltan 5 months ago

Rebased 7999a to trunk rev. 16447.

This branch will work for the large majority of case.

There is only a small case that is not handled by this branch, having 2 buffers to the same table (BOTH have FIELDS or EXCEPT clause, first one and the second one have NO-LOCK). To solve this issue, it would require to add additional overhead of getting previous queries information.

I think this case should be handled in a separate branch, this branch could be reviewed.

#29 Updated by Teodor Gorghe 4 months ago

  • Related to Bug #11347: IGNORE_FIELDS_ERROR is thrown in RAQ, extend rereadfields support added

#31 Updated by Teodor Gorghe 4 months ago

While working on #11298, I have discovered that optimized CQ components does not have the included and excluded maps populated.
This is what I have changed to fix this (CQ.serverJoinPreselect and CQ.serverJoinAdaptive):

=== modified file 'src/com/goldencode/p2j/persist/CompoundQuery.java'
--- old/src/com/goldencode/p2j/persist/CompoundQuery.java    2026-04-06 12:34:41 +0000
+++ new/src/com/goldencode/p2j/persist/CompoundQuery.java    2026-04-06 13:49:32 +0000
@@ -5437,6 +5437,8 @@
          {
             return null;
          }
+         optimizedQuery.included = included;
+         optimizedQuery.excluded = excluded;

          return optimizedQuery;
       }
@@ -5494,6 +5496,8 @@
             optimizedQuery.addComponent(serverCopy);
             qcList.add(serverCopy);
          }
+         optimizedQuery.included = included;
+         optimizedQuery.excluded = excluded;

          return optimizedQuery;
       }

#32 Updated by Constantin Asofiei 4 months ago

Teodor, keep in mind that excluded/included fields belong to a single component. So, if you join multiple components, how do you qualify the fields so you know which field belongs to which table?

Also available in: Atom PDF