FWD » Core Development » Database

From Eric:

While you should read the whole history there, I can summarize as follows:

• We think there will be a performance benefit from denormalizing temp-table extent fields.
• When we shifted <customer_name> to use all denormalized extent fields, the change initially included temp-tables.
  • The differences in the temp-table P2O files broke the proxy Open Client generation rules, so I partially rolled back my update (see 3821c revisions 14041 and 14054).
  • I seem to recall there was another problem applying denormalization to temp-table extents, but perhaps it was only the Open Client proxies.

The problem is that denormalization changes the conversion output; specifically, the DMO interface API. This was intentional in the beginning, because we wanted to support custom naming hints, whereby each expanded column could be assigned a custom name (e.g., weekday[1] -> Monday; weekday[2] -> Tuesday, etc.).

However, in practice, this feature was never used and we are willing to remove it in order to get a consistent conversion, regardless of whether the backing implementation is denormalized extent fields, normalized (to be deprecated), or native array support at the database (e.g., for PostgreSQL). In other words, conversion always should produce DMO interfaces that look like they do when converted with the current, normalized approach. The backing implementation (denormalized vs. database-native array) should vary only in the persistence runtime, and should be decided by dialect or possibly configuration.

In scope:

• re-enabling denormalization for temp-table extent fields;
• converting DMO interfaces as if we were going to use the normalized approach;
• mapping the persistence runtime to use denormalized columns by default.

Out of scope (deferred beyond Jan 15):

• implementing database-native array types.

An alternate approach for the short term could be to consider what changes would be needed to make the proxy Open Client rules work with the denormalized temp-table P2O files generated for the modified, denormalized API, and thus avoid the larger conversion and runtime changes for now. Constantin, are you able to assess how much work this would be? However, since we do ultimately want to rationalize the conversion, this would be throwaway code, which makes this approach less appealing.

Greg, I know we had talked about the conversion rationalization being out of scope, but I hadn't remembered that this was in fact the cause of the regression with <customer_name>.

From Ovidiu:

I read this email several times in order to get the things right. I understand that the denormalization is going to be the standard from this point forward and the DMOs output for denormalized and optionally normalized conversion will be identical. The only thing that will change with the change will be the DDLs. It seems to be that this becomes mainly a runtime issue. As the properties will always be generated as now in normalized mode (both DMO interfaces and code occurrences using []), the backing runtime code will convert these to individual columns at some level - probably as late as Fql2SqlConverter. I guess some additional property in annotation will help.

If we get rid of the custom name (I am not sure but it seems to me that I saw it used in some places, though) we can greatly simplify the code. What I am trying to say is that now we have a denormalization at both property (Java) and column (SQL) level, but this task will eliminate the former. Since we will have a bijective relation between the extent fields and their SQL columns (field[x] <--> column_x) the Persister and Loader classes will be easily modified. Do I oversimplify things?

My second problem is that I am not familiar with Open Client proxy and its generation rules. Is this <customer_name> specific or something more general?

From Eric:

I think you have understood it exactly right.

If we get rid of the custom name (I am not sure but it seems to me that I saw it used in some places, though)

If you saw the custom name feature used in some places, it may have been in test cases (or maybe Hotel?). The only customers which leverage denormalized extent fields at all are customer_x and customer_y. Neither uses hints to customize the conversion of extent field names. <customer_chui_regression_app> extensively customizes table/field names via datanames.xml and other schema hints, but not the hints specific to expanded extent fields.

My second problem is that I am not familiar with Open Client proxy and its generation rules. Is this <customer> specific or something more general?

AFAIK, the Open Client proxy generation rules are general purpose, not specific to <customer>. Constantin, can you confirm? The rules currently are based on the assumption that the statically converted temp-table DMOs follow the "normalized" convention. The goal is to maintain this form of the DMO interface API, regardless of how extent fields are handled internally by the runtime.

One other consideration that I should have made explicit in my first note: business logic and where clause conversion also is different when the denormalized feature is enabled, in that both use references to those DMO interface expanded properties that we are proposing to eliminate. These differences will need to be eliminated as well, so that all converted business logic and FQL where clauses appear to be based on the legacy schema structure (as we do today for the normalized approach), but the underlying runtime code will need to map this to the denormalized mechanism. IIRC, some of the logic to "hide" the use of the secondary table for the normalized approach is in the FQL preprocessing logic, so there will likely be changes there, too.

Do I oversimplify things?

I don't know yet, probably ;) Your deeper analysis will tell.

AFAIK, the Open Client proxy generation rules are general purpose, not specific to <customer>. Constantin, can you confirm?

Yes, correct.

The Open Client proxy generation is the replacement of the OpenEdge "proxygen" tool which creates Java stub classes that can be used to make appserver calls from the OpenEdge "Java Open Client". Any table parameter usage and probably the dataset parameters will be dependent upon DMO generation.

Greg Shah wrote:

The Open Client proxy generation is the replacement of the OpenEdge "proxygen" tool which creates Java stub classes that can be used to make appserver calls from the OpenEdge "Java Open Client". Any table parameter usage and probably the dataset parameters will be dependent upon DMO generation.

Where do I find the manual for this FWD utility? I would like the test the compatibility after my work with temp-tables is done.
Thank you!

• added a new denormalized attribute in @Property annotation

Please name this expanded because the "denormalized" terminology is not accurate and will be eliminated.

Where do I find the manual for this FWD utility?

ProxyGen and Open Client Usage

I committed r14362.

I have a problem which I did not solved: the dynamic extent indices.
For example, for the above table and the query:

define variable v1 as integer init 2.
find first den-tt where e1[v1] eq 7.

Evidently, without the dynamic index, the SQL should be something like:

select * from denTt where e1_2=7;

I do not know how to parametrize the query to make it work as expected.

Don't we already deal with this case in the current implementation of expanded extent fields? I may be misremembering, but I am pretty sure this issue came up before and we implemented a solution.

Code review 7020a/14361-14362:

Nice work! Thank you for turning it around in such a short time.

TBH, it is hard to know whether the generated SQL in the FQL-SQL generator, loader, persister is correct from just a review. I didn't see anything wrong there, but I will need to rely on testing to confirm the rework is without regression. How much testing have you done so far?

Please rename TempTableHelper method addcolumn to addColumn (camelCase).

The new method getIndexComponents in RecordMeta is not used, AFAICT.

On the dynamic index issue, we will have the variable's value at runtime. Isn't this enough to resolve the correct extent field "element" (i.e. discrete column) at SQL generation, or during FQL preprocessing, if needed? Unless the dynamic index is a field reference in the current buffer, the where clause conversion should have refactored v1 (in the example above) to a query substitution parameter, for which we will have the substitution value with which to execute the query. In fact, I thought we already do this, as we have to deal with the same dynamic index syntax already today, even though we refactor the extent field into separate DMO properties currently.

Generally: in comments, please don't use the terms "normalized" when referring to the extent field notation of the legacy code. This is confusing, as "normalized" already was a misleading term when I started using it, and it was meant to refer to the structure of the tables in SQL. I know I probably encouraged this confusion in my description of this issue's purpose (sorry about that), but let's correct it now, so the code isn't forever confusing to someone new coming along:

• I think "[legacy] array notation" is probably the right term to use when referring to the 4GL syntax for referencing a specific extent field element (e.g., contact-info.address[1]). I refer to "legacy" as optional, because we may want to differentiate this from any code which we may soon add to support native database arrays.
• We've already agreed on "expanded" as the term to replace "denormalized" (the latter also was a poor choice on my part, way back when), and it looks like you've already represented it this way in the code/comments. There shouldn't really be a notion of "expanded" in terms of the converted code, so this term pretty much applies only to the generation of and runtime use of SQL DML and DDL/schema.
• Let's not refer to approaches as old/new, unless using this information as a temporary guide to identify code that will be removed/modified shortly, as we complete the transition to use expanded columns or native arrays only. My point is that I don't want the temporal notions of older/newer to linger in the code forever, since at some point soon, we will be finished with this transition and the only approaches that matter will be the ones encoded at the end.

I understand the need to continue the support for the "normalized" schema for now, until we can transition all projects to the expanded or native array approaches. Expanded should be the default for BOTH temp-tables and persistent tables, and we should have a temporary configuration option to continue to use the "normalized" schema. Going forward, we probably will handle the native array support through dialects, and perhaps a conversion configuration override.

These defaults should be reflected in the Property.expanded annotation, and whatever downstream code that change affects. The new AUTO/DEFAULT setting assumes expanded for temporary tables, but normalized for permanent tables. We do not want to default permanent tables to a normalized schema, as this will result in new projects coming online with the wrong schema. The defaults need to encourage the extinction of the normalized approach.

Eric Faulhaber wrote:

Code review 7020a/14361-14362:
Nice work! Thank you for turning it around in such a short time.

Thank you!

TBH, it is hard to know whether the generated SQL in the FQL-SQL generator, loader, persister is correct from just a review. I didn't see anything wrong there, but I will need to rely on testing to confirm the rework is without regression. How much testing have you done so far?

Not much, Several of my targeted testcases. I usually do a smoke-test using the hotel project, but it does not use _temp tables with extent fields to be expanded.

Please rename TempTableHelper method addcolumn to addColumn (camelCase).

Done. This happens accidentally. It's strange that I did not notice in my own clean-up/review.

The new method getIndexComponents in RecordMeta is not used, AFAICT.

It was not related to this task. I will remove it from the branch.

On the dynamic index issue, we will have the variable's value at runtime. Isn't this enough to resolve the correct extent field "element" (i.e. discrete column) at SQL generation, or during FQL preprocessing, if needed? Unless the dynamic index is a field reference in the current buffer, the where clause conversion should have refactored v1 (in the example above) to a query substitution parameter, for which we will have the substitution value with which to execute the query. In fact, I thought we already do this, as we have to deal with the same dynamic index syntax already today, even though we refactor the extent field into separate DMO properties currently.

I will create a testcase to test the solution in denormalized case.

Generally: in comments, please don't use the terms "normalized" when referring to the extent field notation of the legacy code. This is confusing, as "normalized" already was a misleading term when I started using it, and it was meant to refer to the structure of the tables in SQL. I know I probably encouraged this confusion in my description of this issue's purpose (sorry about that), but let's correct it now, so the code isn't forever confusing to someone new coming along:

• I think "[legacy] array notation" is probably the right term to use when referring to the 4GL syntax for referencing a specific extent field element (e.g., contact-info.address[1]). I refer to "legacy" as optional, because we may want to differentiate this from any code which we may soon add to support native database arrays.
• We've already agreed on "expanded" as the term to replace "denormalized" (the latter also was a poor choice on my part, way back when), and it looks like you've already represented it this way in the code/comments. There shouldn't really be a notion of "expanded" in terms of the converted code, so this term pretty much applies only to the generation of and runtime use of SQL DML and DDL/schema.
• Let's not refer to approaches as old/new, unless using this information as a temporary guide to identify code that will be removed/modified shortly, as we complete the transition to use expanded columns or native arrays only. My point is that I don't want the temporal notions of older/newer to linger in the code forever, since at some point soon, we will be finished with this transition and the only approaches that matter will be the ones encoded at the end.

Thank you for clarification. I will update the comments and javadocs.

I understand the need to continue the support for the "normalized" schema for now, until we can transition all projects to the expanded or native array approaches. Expanded should be the default for BOTH temp-tables and persistent tables, and we should have a temporary configuration option to continue to use the "normalized" schema. Going forward, we probably will handle the native array support through dialects, and perhaps a conversion configuration override.

The solution with AUTO was so that I could differentiate at runtime, between the _temp and permanent tables so that the update would work for existing code without the need to reconvert the whole application. The DDLs are already generated and database already imported with possible denormalized structure. This solution avoid immediate interferences here. OTOH, the DDLs and SQL for _temp tables are all dynamically generated at runtime so it works on exiting converted code, only if the extents were not denormalized. If they were, the getters and setters for the properties are changes and their columns are already stored in the primary table.

These defaults should be reflected in the Property.expanded annotation, and whatever downstream code that change affetc. The new AUTO/DEFAULT setting assumes expanded for temporary tables, but normalized for permanent tables. We do not want to default permanent tables to a normalized schema, as this will result in new projects coming online with the wrong schema. The defaults need to encourage the extinction of the normalized approach.

I would like to keep it this way for a little, until the conversion is "expanded"-aware. At that moment, the annotations will clarify the exact solution for the extent fields. As noted above, for _temp tables, this can be altered with each execution, but for permanent tables, the reconversion and re-import of the database is required.

Ovidiu Maxiniuc wrote:

I would like to keep it this way for a little, until the conversion is "expanded"-aware. At that moment, the annotations will clarify the exact solution for the extent fields. As noted above, for _temp tables, this can be altered with each execution, but for permanent tables, the reconversion and re-import of the database is required.

Fair enough, ok for now. But let's not forget to change this default behavior for FWDv4. Customers will need to re-convert their applications anyway, due to all the other conversion changes. At that time, for anyone currently using the deprecated schema, we will need to provide a means to refactor their database schema in place. But that is beyond the scope of this task, so we can discuss it at another time.

Eric Faulhaber wrote:

On the dynamic index issue, we will have the variable's value at runtime. Isn't this enough to resolve the correct extent field "element" (i.e. discrete column) at SQL generation, or during FQL preprocessing, if needed?

Theoretically, yes. The lookup is not trivial, the generated SQL is not reusable (except for similar queries which have the same index for the respective property). This should be done during FQL preprocessing. The converter is parameter-independent because its output is designed to be cached and reusable.

Unless the dynamic index is a field reference in the current buffer, the where clause conversion should have refactored v1 (in the example above) to a query substitution parameter, for which we will have the substitution value with which to execute the query. In fact, I thought we already do this, as we have to deal with the same dynamic index syntax already today, even though we refactor the extent field into separate DMO properties currently.

Unfortunately, as resulting from my simple test the answer is negative. Attempting to execute a query with dynamic index as described in #7020-9 will fail like this:

org.h2.jdbc.JdbcSQLSyntaxErrorException: Syntax error in SQL statement " 
SELECT [...] 
FROM     TT1 DENTT_1_1_0_ 
WHERE    DENTT_1_1_0_._MULTIPLEX = ? (DENTT_1_1_0_.E1?[*] != 1 OR DENTT_1_1_0_.E1? IS NULL) AND [...]
ORDER BY DENTT_1_1_0_._MULTIPLEX DESC, DENTT_1_1_0_.RECID DESC

The FqlToSqlConverter complains about the possible issue, twice (Failed to process extent ALIAS. and Property 'e1' not found to extract column name), but at this time is already too late.

I remember we encounter some time ago this issue. Apparently we have not implemented it yet. See #2134-7 and following. This is actually the only related task I could find.

I think we can fix this issue. Another problem is the self-referencing in constructs like this:

find last den-tt where e1[f1] ne k.

new FindQuery(denTt, (String) null, () -> isNotEqual(denTt.getE1(minus((denTt.getF1()), 1)), k), "denTt.recid asc").last();

I have committed my latest changes and rebase the branch to 6129b. The current revision of 7020a is 14377.

I've tested 7020a/14377 and there is an abend with a customer's application.

Thank you for doing the tests.
May I have some details on this abend? An error message or a stack trace / log file?

I do not have a recent conversion source/binaries of the customer's application and I am not aware of a repository for this. Where can I get the pre-built customer project so I can test myself?

Eric: Please review.

Ovidiu, can you rebase 7020b? I will like to pick it up and do some review / tests / profiling with it.

Constantin, besides me taking forever to review 7020b, we still have the issue with the Client Proxy generation blocking this task, don't we? What will it take to resolve that?

Code review 7020c/15089-15090 (long overdue, sorry):

This update seems to support both denormalized and a new expanded mode. At least, there is a check in dmo_common.rules which seems to treat them as mutually exclusive. In the TRPL code where the expanded annotation is applied, there is the comment: "if is extent and NOT denormalized and specified in [p2j.cfg.xml], mark this as EXPANDED". One of the conditions used to determine whether the field is annotated as expanded is !sourceAst.isAnnotation("denormalizedextent"). In what cases is the AST annotation denormalizedextent present in the AST and do we need it anymore, since we are willing to give up the custom extent field naming feature, as noted in #7020-1?

FastCopyHelper.appendColumnName: this is not new code, but there is a "TODO: add default for the extra DTZ__OFFSET field". Doesn't this need to be addressed?

Property (annotation package): We discussed AUTO/DEFAULT mode previously, in #7020-13 through #7020-15, but I think it is time to change it now. Permanent and temp-tables should be expanded by default. There is only one project which does not yet use expanded extent fields at this point, and we are trying to change this.

DDLGeneratorWorker$Helper.addField negates extent before storing it as a field, for expanded extent fields. When generating the DDL later, the negative extent value is pulled from the field and that copy of the extent int is negated again (back to positive) as it is used. However, the extent int itself is preserved in the P2JField as a negative value. This seems problematic. Any call to that P2JField instance's getExtent method will have the negative value returned. This may be ok for the limited use of that method now, but this seems like a future bug waiting to be exposed if anyone decides to invoke getExtent for some other purpose in the future, and gets the negative value.

FqlToSqlConverter.generateProperty: there is still a "TODO: how do we handle dynamic indices?" where the code just sets extIndex to 0 and continues processing. This will be a very subtle bug to find later on. We must do something different here.

There are several places in the code which have problems because WORD indices evidently were not supported at the time this code originally was written. The code needs to integrate with the WORD index support which has since been implemented.

Please double-check PropertyMeta.compareTo. Is it correct that if either instance being compared is expanded, we should only be comparing IDs?

SQLQuery: please ensure the interaction with RowStructure is still correct, since (I think) the latter class has changed significantly since this expanded code was written.

but I think it is time to change it now. Permanent and temp-tables should be expanded by default

For temp-tables, why would we ever need to support normalized mode? Can't we simply specify that they are ALWAYS expanded?

1. normalized. This is the old solution. In this case the properties are accessed either in bulk or indexed. The database support this mode by using secondary tables which are joined to primary table using the rowid of primary record;
2. denormalized / custom-extents. In this case, the hints dictates the names of the properties. If none or not indices in the extent are hinted, decorated names (with incremental number suffixes) are used. Each indexed item has its own access method in DMO. In database, there are no more secondary tables, the primary tables contains contiguous sets of N columns, one for each extent index;
3. expended. In this case, the DMOs have the structure from the normalized case, but the database layout from denormalized with no custom-extents.

• normalized and expanded share the same DMO classes;
• expanded and denormalized share the same database structure.

1. to switch between normalized and expanded, only the DDL must be regenerated.
   • in case of permanent tables, this requires a full f2 conversion, but we can add code to generate both in a single pass, if needed, and allow the customer the choose the database layout;
   • in case of temp-tables (_meta, too), the DDLs are generated at runtime, JIT. No need for a reconversion, just a flag to pick the desired layout;
2. switching between expanded and denormalized the DMOs must be regenerated.

Conclusion: in case of temp-tables, the reconversion is not necessary when switching to expanded mode. It has the same DMO set with the current solution normalized and the DDLs are generated dynamically.

Nice summary!

1. normalized. This is the old solution. In this case the properties are accessed either in bulk or indexed. The database support this mode by using secondary tables which are joined to primary table using the rowid of primary record;
2. denormalized / custom-extents. In this case, the hints dictates the names of the properties. If none or not indices in the extent are hinted, decorated names (with incremental number suffixes) are used. Each indexed item has its own access method in DMO. In database, there are no more secondary tables, the primary tables contains contiguous sets of N columns, one for each extent index;
3. expended. In this case, the DMOs have the structure from the normalized case, but the database layout from denormalized with no custom-extents.

Just to be clear: we want cases 1 and 2 to die a horrible death, as soon as possible. Only case 3 should survive. It should be the only approach used, for both permanent and temp-tables.

If I understand correctly, the only things holding back the removal of case 1 are this task and #7328 (the online migration of existing normalized databases to use expanded mode). Is that correct?

I don't think any customers use case 2 today. If ETF doesn't depend upon it, then it is unused and should be murdered immediately.

Ovidiu, the original concern with denormalized extents (or 'expanded extents') was because the .p2o (and the DMOs) were having a setter and getter for each element in that extent property, so if you had a field book as char extent 3 there would have been setBook1, setBook2, setBook3, and similar for getters.

If this does not happen, and the DMO for the temp-table is in 'normalized mode', then the proxies will be generated the same.

This is OK. As you can see from the #7020-30 table and the considerations below, the current normalized and the new expanded mode share the same DMO structure, only the database is changed.

• for temp-tables, switching to 7020c should be seamless because the DDL are generated at runtime from the DMO annotations;
• however, (although not expressly requested by this task) 7020c allows the permanent tables to be also switched to expanded database layout WHILE keeping the DMO normalized. This requires static regeneration of their DDLs and reimporting the database after that. Yet, I see that the main db is denormalized (both DDLs and DMOs). Isn't this a problem for the proxies? Or they only based on temp-tables?

I will put 7020c to the test with this second customer project after addressing the regressions from the unittests of the other project (about 2 dozens, scattered between different test sets).

While investigating a possible regression I identified the following problem I am trying to solve. Assuming we have the following temp-table definition:

define temp-table t1
   field f_1_1 as character
   field f_1 as integer extent 3.

   @Property(id = 1, name = "f11", column = "f_1_1", legacy = "f_1_1", format = "x(8)", order = 0)
   public character getF11();

   @Property(id = 2, name = "f1", column = "f_1", legacy = "f_1", format = "->,>>>,>>9", initial = "0", order = 10, extent = 3, expanded = true)
   public integer getF1(int index);

create local temporary table tt1 (
    recid bigint not null,
    _multiplex integer not null,
    _errorFlag integer,
    _originRowid bigint,
    _datasourceRowid bigint,
    _errorString varchar_ignorecase,
    _peerRowid bigint,
    _rowState integer,
    f_1_1 varchar_ignorecase,
    f_1_1 integer,
    f_1_2 integer,
    f_1_3 integer,
    primary key (recid)
) transactional MULTIPLEXED;

At runtime, the expanded algorithm assumes the suffixed names do not collide for any of the numbers in the interval.

To fix the problem, I am using an old trick we used in other places: using a double underscore instead of a single one. As result, the table definition will change as:

create local temporary table tt1 (
    recid bigint not null,
    _multiplex integer not null,
    _errorFlag integer,
    _originRowid bigint,
    _datasourceRowid bigint,
    _errorString varchar_ignorecase,
    _peerRowid bigint,
    _rowState integer,
    f_1_1 varchar_ignorecase,
    f_1__1 integer,
    f_1__2 integer,
    f_1__3 integer,
    primary key (recid)
) transactional MULTIPLEXED;

However, things are not that simple now, also. At the time of FQL preprocessing, some occurrences of properties must be 'expanded'. Therefore, tt1.f1[1] will be replaced with tt1.f1__1, as expected. The problem is now that, when the expanded property must be converted to SQL column name, it is not directly found. The metadata contains a single record of tt1.f1. And I do not like the idea of 'parsing' back the property name only to convert it then back to SQL column name. I am looking for best solution here.

I used a permanent table with same structure as the temp-table from #7020-36 and converted it with trunk (r15368), in both normalized and denormalized mode. The first results I got yesterday were really strange so I thought I have used a 'dirty' revision of the trunk. I used a clean trunk today and have redone the test. Because I got the same results, I altered the source table a bit by adding fields f_1_2 and f_1_3 but then removed f_1_2 since the result was more conclusive.

create table p_exp1 (
   recid int8 not null,
   f_1_1 int4,
   f_1_3 int4,
   primary key (recid)
);
create table p_exp1__3 (
   parent__id int8 not null,
   f_1 text,
   list__index int4 not null,
   primary key (parent__id, list__index)
);

create table p_exp1 (
   recid int8 not null,
   f_1_1 int4,
   f_1_3 int4,
   f_1_11 text,
   f_1_2 text,
   f_1_32 text,
   primary key (recid)
);

@Property(id = 1, name = "f11", column = "f_1_1", legacy = "f-1-1", format = "999999", initial = "0", order = 10)
public integer getF11();
@Property(id = 2, name = "f13", column = "f_1_3", legacy = "f-1-3", format = "999999", initial = "0", order = 11)
public integer getF13();
@Property(id = 3, name = "f1", column = "f_1", legacy = "f-1", format = "x(35)", order = 51, extent = 3)
public character getF1(int index);

@Property(id = 1, propertyId = 1, name = "f11", column = "f_1_1", legacy = "f-1-1", format = "999999", initial = "0", order = 10)
public integer getF11();
@Property(id = 2, propertyId = 2, name = "f13", column = "f_1_3", legacy = "f-1-3", format = "999999", initial = "0", order = 11)
public integer getF13();
@Property(id = 3, propertyId = 3, name = "f1_1", column = "f_1_11", legacy = "f-1", format = "x(35)", order = 51, index = 1, extent = 3, original = "f1")
public character getF1_1();
@Property(id = 3, propertyId = 4, name = "f1_2", column = "f_1_2", legacy = "f-1", format = "x(35)", order = 52, index = 2, extent = 3, original = "f1")
public character getF1_2();
@Property(id = 3, propertyId = 5, name = "f1_3", column = "f_1_32", legacy = "f-1", format = "x(35)", order = 53, index = 3, extent = 3, original = "f1")
public character getF1_3();

Eric, I think it may make sense (for expanded mode) to emit at the DMO property the list of SQL names associated with this extent property. This will make things consistent (i.e. we know exactly which SQL fields are mapped to this extent property and don't need to go through some complicated rules to figure out the names), and fixing expanded mode to emit the same DDL as for denormalized mode, I think we are OK with existing installations.

Constantin,
emitting a list of columns = "f_1_11, f_1_2, f_1_32" (as it was computed to avoid collisions in denormalized mode) is ugly, and difficult to read/ lookup.

The solution I am working on searches for a common base-name which will make sure adding the natural suffixes unique. In this case it is something like column = "f_11", which will result in f_11_1, f_11_2, f_11_3 to be derived and safely used as column names.

Constantin provided a simple example in #8805-64 where this is in a persistent DB:

ADD TABLE "ttw" 
  AREA "Schema Area" 
  DUMP-NAME "ttw" 

ADD FIELD "f1" OF "ttw" AS character 
  FORMAT "x(8)" 
  INITIAL "" 
  POSITION 2
  MAX-WIDTH 90
  EXTENT 5
  ORDER 10
  MANDATORY
  CASE-SENSITIVE

ADD INDEX "f1" ON "ttw" 
  AREA "Schema Area" 
  WORD
  INDEX-FIELD "f1" ASCENDING

The main procedure ext1.p contains:

create ttw.

def new shared frame f3.
form ttw.f1[1] with frame f3.
display ttw.f1[1] with frame f3.

run ext1b.p.

The external procedure ext1b.p contains:

create ttw.
def var i as int.
i = 1.

def shared frame f3.
form ttw.f1[1] with frame f3.
display ttw.f1[i] with frame f3.

When you convert without denorm-extents, the main procedure gets:

         FrameElement[] elementList0 = new FrameElement[]
         {
            new Element(new FieldReference(ttw, "f1", 0), f3Frame.widgetF1Array0())
         };

         FrameElement[] elementList0 = new FrameElement[]
         {
            new Element(new FieldReference(ttw, "f1_1"), f3Frame.widgetF11())
         };

The external procedure emits the same code, regardless of the denorm-extents:

         FrameElement[] elementList0 = new FrameElement[]
         {
            new Element(new FieldReference(ttw, "f1", minus((i), 1)), f3Frame.widgetF1Array(minus((i), 1)))
         };

But the frame definition is very different:

--- /tmp/x/src/com/goldencode/testcases/ui/fwd_tests/convert_/Ext1F3.java
+++ /home/rfb/testcases_v2/src/com/goldencode/testcases/ui/fwd_tests/convert_/Ext1F3.java
@@ -9,44 +9,35 @@
 {
    public static final Class configClass = Ext1F3Def.class;

-   public character getF1Array0();
+   public character getF11();

-   public character getF1Array(NumberType parm);
+   public void setF11(character parm);

-   public character getF1Array(double parm);
+   public void setF11(String parm);

-   public void setF1Array0(character parm);
+   public void setF11(BaseDataType parm);

-   public void setF1Array0(String parm);
-
-   public void setF1Array0(BaseDataType parm);
-
-   public FillInWidget widgetF1Array0();
-
-   public FillInWidget widgetF1Array(NumberType parm);
-
-   public FillInWidget widgetF1Array(double parm);
+   public FillInWidget widgetF11();

    public static class Ext1F3Def
    extends WidgetList
    {
-      FillInWidget f1Array0 = new FillInWidget();
+      FillInWidget f11 = new FillInWidget();

       public void setup(CommonFrame frame)
       {
          frame.setDown(1);
-         f1Array0.setIndex(1);
-         f1Array0.setDbname("tstcasesdb");
-         f1Array0.setTable("ttw");
-         f1Array0.setDataType("character");
-         f1Array0.setDbname("tstcasesdb");
-         f1Array0.setTable("ttw");
-         f1Array0.setFormat("x(8)");
-         f1Array0.setLabel("f1[1]");
+         f11.setDbname("tstcasesdb");
+         f11.setTable("ttw");
+         f11.setDataType("character");
+         f11.setDbname("tstcasesdb");
+         f11.setTable("ttw");
+         f11.setFormat("x(8)");
+         f11.setLabel("f1");
       }

       {
-         addWidget("f1Array0", "f1", f1Array0);
+         addWidget("f11", "f1", f11);
       }
    }
 }

Here is the frame definition without denorm-extents: ShowHide

package com.goldencode.testcases.ui.fwd_tests.convert_;

import com.goldencode.p2j.util.*;
import com.goldencode.p2j.ui.*;
import com.goldencode.testcases.ui.shared_frames.*;

public interface Ext1F3
extends FrameF3_1
{
   public static final Class configClass = Ext1F3Def.class;

   public character getF1Array0();

   public character getF1Array(NumberType parm);

   public character getF1Array(double parm);

   public void setF1Array0(character parm);

   public void setF1Array0(String parm);

   public void setF1Array0(BaseDataType parm);

   public FillInWidget widgetF1Array0();

   public FillInWidget widgetF1Array(NumberType parm);

   public FillInWidget widgetF1Array(double parm);

   public static class Ext1F3Def
   extends WidgetList
   {
      FillInWidget f1Array0 = new FillInWidget();

      public void setup(CommonFrame frame)
      {
         frame.setDown(1);
         f1Array0.setIndex(1);
         f1Array0.setDbname("tstcasesdb");
         f1Array0.setTable("ttw");
         f1Array0.setDataType("character");
         f1Array0.setDbname("tstcasesdb");
         f1Array0.setTable("ttw");
         f1Array0.setFormat("x(8)");
         f1Array0.setLabel("f1[1]");
      }

      {
         addWidget("f1Array0", "f1", f1Array0);
      }
   }
}

Here is the frame definition with denorm-extents: ShowHide

package com.goldencode.testcases.ui.fwd_tests.convert_;

import com.goldencode.p2j.util.*;
import com.goldencode.p2j.ui.*;
import com.goldencode.testcases.ui.shared_frames.*;

public interface Ext1F3
extends FrameF3_1
{
   public static final Class configClass = Ext1F3Def.class;

   public character getF11();

   public void setF11(character parm);

   public void setF11(String parm);

   public void setF11(BaseDataType parm);

   public FillInWidget widgetF11();

   public static class Ext1F3Def
   extends WidgetList
   {
      FillInWidget f11 = new FillInWidget();

      public void setup(CommonFrame frame)
      {
         frame.setDown(1);
         f11.setDbname("tstcasesdb");
         f11.setTable("ttw");
         f11.setDataType("character");
         f11.setDbname("tstcasesdb");
         f11.setTable("ttw");
         f11.setFormat("x(8)");
         f11.setLabel("f1");
      }

      {
         addWidget("f11", "f1", f11);
      }
   }
}

Roger,

I looked at the code. I do not see what the problem is. The only difference which is probably visible in application's GUI is the label (f1Array0.setLabel("f1[1]"); vs f11.setLabel("f1");). Then there are a couple of additional indexed getters and setters in the normalized case. I understand that there is something serious which prevents the project in #8805 to use the denorm-extents, but I cannot see it directly. Does the build fail because of the missing accessors?

Ovidiu Maxiniuc wrote:

Does the build fail because of the missing accessors?

This is the compilation error:

compile:
    [javac] Compiling 34 source files to /home/rfb/projects/testcases_v2_dev/build/classes
    [javac] Support for javac --release has been added in Java9 ignoring it
    [javac] Compiling 3 source files to /home/rfb/projects/testcases_v2_dev/build/classes
    [javac] Support for javac --release has been added in Java9 ignoring it
    [javac] /home/rfb/projects/testcases_v2_dev/src/com/goldencode/testcases/fwd_tests/convert_/Ext1b.java:49: error: cannot find symbol
    [javac]             new Element(new FieldReference(ttw, "f1", minus((i), 1)), f3Frame.widgetF1Array(minus((i), 1)))
    [javac]                                                                              ^
    [javac]   symbol:   method widgetF1Array(int64)
    [javac]   location: variable f3Frame of type FrameF3_1
    [javac] 1 error

BUILD FAILED

I understand now. The problem is display ttw.f1[i] with frame f3.. Note the variable i as index in ext1b.p as opposed to constant 1 in ext1.p.

As I wrote above, the frame definition for the expanded mode does not define a runtime indexed access to widgets. This is more like a UI issue rather than a persistence one. Nonetheless, it must be fixed. I do not have a clear idea of how this is correctly solved.

Hynek, would it be enough to emit the declaration of widgetF1Array(NumberType) in the expanded Ext1F3 interface?

Ovidiu Maxiniuc wrote:

I understand now. The problem is display ttw.f1[i] with frame f3.. Note the variable i as index in ext1b.p as opposed to constant 1 in ext1.p.

As I wrote above, the frame definition for the expanded mode does not define a runtime indexed access to widgets. This is more like a UI issue rather than a persistence one. Nonetheless, it must be fixed. I do not have a clear idea of how this is correctly solved.

Hynek, would it be enough to emit the declaration of widgetF1Array(NumberType) in the expanded Ext1F3 interface?

I didn't try but I don't think this would be enough. The GenericFrame proxy invocation logic for the call widgetF1Array(NumberType) would expect the widget name f1Array0 added to the widget list in @Ext1F3Def.

I have locally rebased 7020b branch to trunk revision 15561. ./gradlew all works, but functionality remains to be tested. Let me know if you want me to overwrite 7020b with my locally rebased version, or should I wait to test it properly.

Stefanel Pezamosca wrote:

I have locally rebased 7020b branch to trunk revision 15561. ./gradlew all works, but functionality remains to be tested. Let me know if you want me to overwrite 7020b with my locally rebased version, or should I wait to test it properly.

As far as I can remember, 7020c is the latest revision. I did not remove (archive) 7020b because I was afraid I might have broken it during rebase processes.

Ovidiu Maxiniuc wrote:

As far as I can remember, 7020c is the latest revision. I did not removed (archived) 7020b because I was afraid I might have broken it during rebase processes.

Oh, right ... Oops. I missed it, sorry.

Greg Shah wrote:

Ovidiu: What is the list of remaining work/testing to finish this task?

I have committed revision 15367 which contains some work I've done related to issue #7020-36 (collision of property names). Note that since I suspended abruptly the work on this task, this last revision might not be stable.
Of course 7020c must be rebased since it is a few months behind.

I remember I have tested the branch against at least one of the customer's unit-tests and the result were good, but not perfect.

• the where predicate contains buffer.ext-field[expression], when the expression uses local variables and other fields (not a constant) in case it can be converted to a SUBST node and evaluated on client side before the query is executed (it should be handled in FQL Preprocessor, the expression is evaluated and passed as value in args parameter);
• the where predicate contains buffer.ext-field[buffer.num-field], where num-field is another field of same table. Or an expression which can be fully encoded server-side. Due to the nature of this construct, the whole expression is SQL server-side expression. The resulting SQL query in this case were incorrect. I do not remember finding a solution for constructs like this.

Of course, both of above cases must be correctly handled for in both temp and permanent databases.

Firstly, I will try rebasing 7020c. If this fails, I will make a new 7020d branch and take the changes manually.

I have rebased 7020c to trunk revision 15658 and it's now at revision 15668. I need to make sure that I didn't break anything after doing this. Then, I will proceed with testing and making some testcases for the remaining issues.

Ovidiu, I have the following testcases:

def var id_ext as integer no-undo.
assign id_ext = 1.
for each tt1 where tt1.ext5[id_ext] = "2":
    message tt1.ext5[1].
end.

         AdaptiveQuery query0 = new AdaptiveQuery();
         forEach(query0, "loopLabel1", new Block((Init) () -> 
         {
            query0.initialize(tt1, "upper(tt1.ext5[?]) = '2'", null, "tt1.recid asc", new Object[]
            {
               minus((idExt), 1)
            });
         }, 

select 
    tt1_1_1__i0_.* 
from
    tt3 tt1_1_1__i0_ 
where
    tt1_1_1__i0_._multiplex = ? and tt1_1_1__i0_.ext5_1 = '2'
order by
    tt1_1_1__i0_._multiplex asc, tt1_1_1__i0_.recid asc
lazy

The second testcase:

for each tt1 where tt1.ext5[tt1.id_ext] = "2":
    message tt1.ext5[1].
end.

         AdaptiveQuery query1 = new AdaptiveQuery();
         forEach(query1, "loopLabel2", new Block((Init) () -> 
         {
            query1.initialize(tt1, ((String) null), () -> isEqual(tt1.getExt5(minus((tt1.getIdExt()), 1)), "2"), "tt1.recid asc");
         }, 

Does these two test cases present any issues in your opinion? Also, do you recall the specific problem you were aiming to address?
Also, I guess #7020-40 is still an issue, I will look into that.

Stefanel Pezamosca wrote:

Ovidiu, I have the following testcases:
[...] Converted to:
[...] And the sql: This seems correct, right ?
[...]

Yes, the SQL looks correct.

The second testcase:
[...] Converted to:
[...] All records from tt1 are being selected first, and the WHERE condition isEqual(tt1.getExt5(minus((tt1.getIdExt()), 1)), "2") is processed afterwards. While this approach works, it is negatively impacting performance.

select * from tt1 where
      case when id_ext = 1 then ext5_0
           when id_ext = 2 then ext5_1
           when id_ext = 3 then ext5_2
           when id_ext = 4 then ext5_3
           when id_ext = 5 then ext5_4
           -- else returns null
      end  = '2';

• this will require the processing to be moved more to Fql2SqlConverter rather than FqlPreprocessor;
• this will be probably much faster than whet you get above, since will be fully executed on SQL side;
• probably performance and readability will degrade with the size of extent, since multiple branches will be added to case;
• SQL planner may have opacity issue because of case, and fail to provide the optimum plan. To be tested for each dialect;
• this is a very simple expression (only the id_ext column), but the extent index can be a much more complex (ext5[2*id_ext1 + log(id_ext2)] + ext5[sin(id_ext1) + 4*id_ext2] = '24'). Will be FWD able to cope with these?

Does these two test cases present any issues in your opinion? Also, do you recall the specific problem you were aiming to address?

As noted above, these were the main concerns and caused for 7020c not yet being merged into trunk yet.
You can also try to see whet happens in a join query where the id_ext is a field/column of another table than ext5 / ext5_<n> extent field.

Also, I guess #7020-40 is still an issue, I will look into that.

That is a rather newly discovered issue, after my work on this task was suspended. It seems like the expanded version of a frame having a widget referring an element of an extent field (here named f1) is missing the methods for dynamic access:

   public character getF1Array(NumberType idx);
   public character getF1Array(double idx);

   public void setF1Array(NumberType idx, character val);
   public void setF1Array(NumberType idx, String val);
   public void setF1Array(double idx, character val);
   public void setF1Array(double idx, String val);

   public FillInWidget widgetF1Array(NumberType idx);
   public FillInWidget widgetF1Array(double idx);

I found a solution for the compilation issue:

=== modified file 'rules/annotations/frame_scoping.rules'
--- old/rules/annotations/frame_scoping.rules    2025-01-10 07:45:20 +0000
+++ new/rules/annotations/frame_scoping.rules    2025-01-27 14:12:53 +0000
@@ -1904,7 +1904,8 @@

                <!-- else: isDenorm, return the denormalized index as suffix -->
                <rule on="false">true
-                  <action>res = ref.getAnnotation("extent_elem_idx").toString())</action>
+                  <action>ext = #(long) ref.getAnnotation("extent_elem_idx")</action>
+                  <action>res = sprintf("Array%d", ext - 1)</action>
                </rule>
             </rule>
          </rule>

• Fixed inconsistency in frame widget handling when using denorm-extents.
• Fixed build_implicit_label for denorm-extent fields.

Who can review these ?

I managed to fix some of the errors mentioned in #7020-62. There are more places in code that uses PropertyMeta.getExtent() to check if the property is a scalar or denormalized extent field. If PropertyMeta.getExtent is > 0 it is assumed that the extent is normalized. In this case we need to also check for expanded property annotation.

I will try to come up with prettier solution for this.

Greg/Roger: I want to ask: Do you think we should bring the UI fixes for denorm-extents=true in another branch and merge to trunk earlier? So we could test conversion + runtime for #8805 / #9537 app.

Yes, it is a good idea.

Code review 7020c. The UI changes look OK to me.

There are some files that are missing file history entries.

Hynek Cihlar wrote:

Code review 7020c. The UI changes look OK to me.

There are some files that are missing file history entries.

9537a revision 15672 contains only the UI changes.
I will proceed with conversion testing of other apps.
Roger (or someone who can), could you help by testing 9537a with denorm_extents==true for #8805 / #9537.

Stefanel Pezamosca wrote:

Hynek Cihlar wrote:

Code review 7020c. The UI changes look OK to me.

There are some files that are missing file history entries.

9537a revision 15672 contains only the UI changes.

Yes, these are the same as in 7020c. And they look good.

Stefanel Pezamosca wrote:

Roger (or someone who can), could you help by testing 9537a with denorm_extents==true for #8805 / #9537.

I will try a conversion of the application with 9537a.

I have rebased 7020c to trunk revision 15674.
I have committed a few fixes and code cleanup after the rebase and 7020c is now at revision 15688.

Roger Borrello wrote:

Stefanel Pezamosca wrote:

Roger (or someone who can), could you help by testing 9537a with denorm_extents==true for #8805 / #9537.

I will try a conversion of the application with 9537a.

Unfortunately, I ran out of space on my conversion attempt (see #7183-106). I will retry. Did your attempts with Constantin's testcase in #7020-40 look correct?

Roger Borrello wrote:

Roger Borrello wrote:

Stefanel Pezamosca wrote:

Roger (or someone who can), could you help by testing 9537a with denorm_extents==true for #8805 / #9537.

I will try a conversion of the application with 9537a.

Unfortunately, I ran out of space on my conversion attempt (see #7183-106). I will retry. Did your attempts with Constantin's testcase in #7020-40 look correct?

I am running the conversion again.

Roger Borrello wrote:

Unfortunately, I ran out of space on my conversion attempt (see #7183-106). I will retry. Did your attempts with Constantin's testcase in #7020-40 look correct?
I am running the conversion again.

Ok. The conversion of the test case from #7020-40 with 9537a remains the same regardless of the denorm-extents setting (true/false). And it also seemed to work as expected.
I'm waiting for your conversion result.

Stefanel Pezamosca wrote:

So, an update on 7020c, I was testing a customer application and encountered a regression that seems to be related with dynamic queries. The stacktrace looks something like this:
[...] I'm looking into this at the moment and trying to replicate in a standalone test.

I managed to fix this. It was another case of unchecked expanded property in DmoMeta which resulted in fieldCount having a wrong value. Also, I don't leave the possibility of this being another case of missing code after some older rebase operations.

I committed the fix in revision 15689.

There are 2 builds of the customer application using 9537a-15672, one as we have always built, and one using denorm-extent set to true. I had not been able to complete a build with it set, due to the malformation of the frames.

What other testing or observations need to be performed? Should the database be usable as-is? Or is there some migration?

Roger Borrello wrote:

There are 2 builds of the customer application using 9537a-15672, one as we have always built, and one using denorm-extent set to true. I had not been able to complete a build with it set, due to the malformation of the frames.

Is it the same error as before or something else? Could you send me an email (sp) with the details ?
Edit: I found the builds, but I didn't see something wrong. Can you tell me exactly what issue did you actually had?

What other testing or observations need to be performed? Should the database be usable as-is? Or is there some migration?

Unfortunately, the database imported with denorm-extents=false (normalized extents), is not compatible with denorm-extents=true.

Could you send me an email (sp) with the details ?

Details can be discussed in #9537.

Stefanel Pezamosca wrote:

Roger Borrello wrote:

There are 2 builds of the customer application using 9537a-15672, one as we have always built, and one using denorm-extent set to true. I had not been able to complete a build with it set, due to the malformation of the frames.

Is it the same error as before or something else? Could you send me an email (sp) with the details ?
Edit: I found the builds, but I didn't see something wrong. Can you tell me exactly what issue did you actually had?

No errors in the build with 9537a-15672. I was indicating that the previous attempts (with trunk) and denorm-extent set to true had not ever completed.

Roger Borrello wrote:

No errors in the build with 9537a-15672. I was indicating that the previous attempts (with trunk) and denorm-extent set to true had not ever completed.

Ah, got it now. So, all that's left to do is test the application with a new database import. Are you able / available to reimport the database with 9537a-15672 and denorm-extent=true? Otherwise, we will need the tool to migrate the already normalized extents.

Stefanel Pezamosca wrote:

Roger Borrello wrote:

No errors in the build with 9537a-15672. I was indicating that the previous attempts (with trunk) and denorm-extent set to true had not ever completed.

Ah, got it now. So, all that's left to do is test the application with a new database import. Are you able / available to reimport the database with 9537a-15672 and denorm-extent=true? Otherwise, we will need the tool to migrate the already normalized extents.

I am. Is there already a tool for that? The customer would need to test in that manner.

No, that tool is not ready yet. Please test using a normal import.

The regression testing needs to do a diff between current/trunk DDLs and the new DDLs. We need to know how/if something has changed.

Roger Borrello wrote:

Constantin Asofiei wrote:

The regression testing needs to do a diff between current/trunk DDLs and the new DDLs. We need to know how/if something has changed.

There are a lot, but they follow the pattern of moving the additional table "into" the original table.

I've attached the 2 here. There weren't any changes in schema_index_menu_postgresql.sql.

I've meant for existing apps which use denormalized/expanded extents. If we switch an app from normalized to denormalized/expanded extents, then there will be differences.

Ovidiu noted this when I posted that my attempt to import into the database with normalized extents resulted in too many columns for PostGreSQL:

Ovidiu Maxiniuc wrote:

Oops, this is an issue I did not expect.

I do not see a problem with table strecord but

• stcopya has 1000+ columns and
• stcopyb has 2100+ columns when the fields are expanded. This is well beyond the 1.6K "hard limit" of columns per table in PSQL.

Are all these extents necessary?

PS: I did a bit of research for the other SQL dialects. In MariaDb the limit is 4Ki. In H2 apparently there is no limit. In MSSQL the limit is 1Ki.

Does this mean "back to the drawing board", or is there a way around this?

Stefanel Pezamosca wrote:

So, pretty much all the errors I encountered while testing 7020c with expand-extents=true have been fixed. I think 7020c is ready for another review. Eric/Ovidiu?

Please note that I did not do a full review of the latest changes (Ovidiu, please review). However, the mention of the configuration option expand-extents=true reminded me of the conversation below, from when Ovidiu began this implementation two years ago:

Ovidiu Maxiniuc wrote:

Eric Faulhaber wrote:

[...]
I understand the need to continue the support for the "normalized" schema for now, until we can transition all projects to the expanded or native array approaches. Expanded should be the default for BOTH temp-tables and persistent tables, and we should have a temporary configuration option to continue to use the "normalized" schema. Going forward, we probably will handle the native array support through dialects, and perhaps a conversion configuration override.

The solution with AUTO was so that I could differentiate at runtime, between the _temp and permanent tables so that the update would work for existing code without the need to reconvert the whole application. The DDLs are already generated and database already imported with possible denormalized structure. This solution avoid immediate interferences here. OTOH, the DDLs and SQL for _temp tables are all dynamically generated at runtime so it works on exiting converted code, only if the extents were not denormalized. If they were, the getters and setters for the properties are changes and their columns are already stored in the primary table.

These defaults should be reflected in the Property.expanded annotation, and whatever downstream code that change affetc. The new AUTO/DEFAULT setting assumes expanded for temporary tables, but normalized for permanent tables. We do not want to default permanent tables to a normalized schema, as this will result in new projects coming online with the wrong schema. The defaults need to encourage the extinction of the normalized approach.

I would like to keep it this way for a little, until the conversion is "expanded"-aware. At that moment, the annotations will clarify the exact solution for the extent fields. As noted above, for _temp tables, this can be altered with each execution, but for permanent tables, the reconversion and re-import of the database is required.

The goal here is to not require an explicit configuration option to enable expanded extents for both temp-tables and persistent tables. It should be the default. We temporarily need to support the normalized extents approach until we can get all projects shifted over to the default, expanded behavior. My question here is: will the DMOs generated by expanded mode still work with a normalized schema? In theory, based on the initial requirements in #7020-1:

In scope:

• [...]
• converting DMO interfaces as if we were going to use the normalized approach;
• [...]

...they should.

However, the runtime will need to know how to generate its SQL to match the target schema. So, we will need a runtime configuration to allow normalized mode to work with a persistent schema that hasn't been migrated yet. Note that ideally, the temp-tables in this scenario could/would still used expanded mode, since their schemas are created at runtime and thus aren't restricted to normalized mode, even if the application's persistent schemas still are.

Likewise, I think denorm-extents should be removed as a conversion configuration option in this branch. The conversion should warn that it is a discontinued (and ignored) option which should be removed. The DDL for the denormalized and expanded approach should be identical; the differences in conversion when moving from denormalized to expanded should only be in the DMOs. So, as long as the same post-7020c version of FWD is used for conversion and at runtime, using expanded by default with an expanded/denormalized schema should not be a problem.

Roger Borrello wrote:

Does this mean "back to the drawing board", or is there a way around this?

Ugh, this is the extreme use case I was trying to protect against when designing the normalized approach in the first place.

Well, it is a setback, but it should not take us all the way back to the drawing board. Since it is a hard limit of the database(s), this may be something we need to work through (i.e., refactor the schema) with customers.

Open to other ideas...

I'm considering a solution: We could remove all references to denorm-extent and replace them with expanded-extents. The DDLs and database would be compatible with expanded, requiring only a conversion.

Additionally, we will remove the expanded annotation from the property. The conversion will generate the same code/DMOs for both normalized and expanded modes, with the only difference being the database itself/DDLs.

I propose using the expand-extents configuration from p2j.cfg.xml solely to inform the database import process and runtime whether the extents in the database have been normalized or not.

Naturally, temporary tables will have the expanded property enabled by default. If the expanded annotation is not set, its default value should be true, or it should be taken from the configuration if we need to set it to false just for persistent tables.

So, should I remove all references to denormalized / custom-extents and keep just the normalized and expanded extents for now?

I did some other small changes and 7020c is now at revision 15707.

With denorm-extent the name of the word tables for extent fields is table__extname_1 vs table__extname for expand-extent. wordTableName is generated by [table name]__[field name] and in denorm_extent case, the field name used was extname_1 (extname[1]). Also, when using denorm_extents for contains, it worked only on extname[1] and not other subscripts.

So, We need to ensure proper handling of word tables and support for extent fields. The extent subscript is not relevant in this context.
When using for each tt where tt.words_ext[0] for example, the [0] subscript is ignored and the contains statement searches across all subscripts of words. (how contains work in 4GL)

We should remove the word table with list_index. Whether the field is an extent or not, the word table should only include parent__id and word.
But this change will require a word reindex to ensure proper functionality.

What do you think, should this be done in another task ?

The 7020c rebased with trunk revision 15703. New revision is 15723.

What do you think, should this be done in another task ?

No, let's solve it here, once and for all time.

First try to solve contains for expanded extent fields in a temp-table looks like this:

For example, for a field named "word" with an extent of 5 it will append to the final SQL string:
CONCAT_WS('|', word_1, word_2, word_3, word_4, word_5). I tested a bit and there is not any big performance hit by doing this. (Edit: Testing again showed a slight performance hit, but I made other optimizations that compensate for it.)

This change and other performance improvement for the Contains.words() function are committed in revisions 15730-15733.

7020c was rebased to trunk 15724 and is now at revision 15748.

7020c was rebased to trunk 15747 and is now at revision 15771.

7020c was rebased to trunk 15752 and is now at revision 15777. Last 5 revisions have some performance improvements for CONTAINS related methods.

7020c was rebased to trunk 15761 and is now at revision 15786.

Eric/Ovidiu: Please make it a priority to get this code review done. We have several bugs being held open due to this task.

Stefanel: Is there anything else that needs to get done on this task?

I have started reviewing the branch, but it will not be ready until tomorrow due to its size.

Review of 7020c / 15792.

I see that you did what I was afraid to do: completely drop the support for custom extents. This is a huge update, there are a lot of affected files. My plan was to work this task in 2 phases: first add the expanded mode and make it stable. This would allow a buffer in which possible issues would have been resolved. Then, in a subsequent branch to drop the deprecated mode, a job which I estimate would be the easier part. But sometimes one needs to really change tracks and never look the other way. These being said:

• TableMapper$LegacyFieldInfo
  • I am not sure the drop of original is the right choice. It is not used only for properties 'renamed' by hints but the common name for elements in the expanded field (see below).
• BufferImpl.java:
  • lines 2480-2500: I think the original should be check. That is the name of the property (converted from legacy name) before the expanded suffix is added. It groups all elements of the expanded field under a single property name.
    At any rate, the hibernateName (no hibernate, anyway), can be inlined.
• DmoProxyPlugin.java:
  • the definition of private static final String COMPOSITE can be safely dropped.
• frame_generator.xml
  • there are no actual changes except for H entry
• dmo_common.rules
  • since this will be a single commit to main trunk, the H entry of 20250206 should also fall under 069 (as in p2o.xml).
• P2JIndexComponent.java
  • the file does not contain actual logic changes. Beside, the H entry list is broken (009 -> 019).
• AdaptiveRowStructure.java and SQLQuery.java:
  • The copyright year should be updated.
• UnorderedRowStructure.java and TransactionManager.java lack the H entry.

• as noted above, is the drop of the 'original' property name. My guts say that it should have not been removed. Of course, there is a chance that they are wrong.
• in some cases, the expanded property/column name is computed by suffixing the property name with the index number, as opposed to use TempTableHelper.getExpandedName(). I think this is not always the case (see the example in #7020-37). OTOH, even if it works, I think we should have a single method which handles this in a unified fashion - that would be easier to maintain, also.

The above notes are based only on comparing the differences from the base 15770 revision. I did a double pass because the reference changes I encountered while reading the code. I think it would be best to use a testcase and convert/execute it using the available field mapping. That would early identify other possible issues I was not able to spot during my review.

We have to do extensive testing of this update. The conversion result will have to be manually inspected (as much as possible - hopefully, there will not be many affected projects, since they are already de-normalized). The DMO interfaces for temp-tables will be, for sure.

Eric, a large part of the changes are mine so even if a lot of time had passed and my mind is able to see that code form a different point of view, you have the final word. (I know you already did review the branch at a precedent revision, before Stefanel started working).

Stefanel Pezamosca wrote:

Now that I think about the potential issues, I believe this plan will be safer. So, I’m going to create a new branch (7020d) where I’ll add only the necessary changes to support expanded mode. For now, there will just be a warning that denorm-extents is deprecated and will no longer be supported in the future. Then after the merge we will work for removing the leftovers. What do you think about this?

Well, you did the big part. That was me praising you 👍. I am not sure it would be beneficial to step back now. Unless there are some regressions that cannot be easily fixed, in my opinion, you should defer work related to the new 7020d branch.

Is there any relationship between branches 9537a and 7020c? I am especially concerned with my big application, since we couldn't get a successful conversion of it due to the frame definitions.

Roger Borrello wrote:

Is there any relationship between branches 9537a and 7020c? I am especially concerned with my big application, since we couldn't get a successful conversion of it due to the frame definitions.

I think 9537a is standalone at the moment. This branch fixed the issue and was already tested.
Alexandru/Greg, can 9537a be merged?

For some reason, the work on 9537a is actually in #8982 and the last notes there suggest that we have a design limitation on 1600 columns in a table. I don't think we can move ahead with 9537a until that issue is resolved.

Greg Shah wrote:

For some reason, the work on 9537a is actually in #8982 and the last notes there suggest that we have a design limitation on 1600 columns in a table. I don't think we can move ahead with 9537a until that issue is resolved.

I don't see an 8982a branch. I want to be able to verify the customer's conversion, since the situation Constantin noted in #8805-64 (below, as well) is in their application:

• ext1.p
  

      /*
      ADD TABLE "ttw" 
        AREA "Schema Area" 
        DUMP-NAME "ttw" 
  
      ADD FIELD "f1" OF "ttw" AS character 
        FORMAT "x(8)" 
        INITIAL "" 
        POSITION 2
        MAX-WIDTH 90
        EXTENT 5
        ORDER 10
        MANDATORY
        CASE-SENSITIVE
  
      ADD INDEX "f1" ON "ttw" 
        AREA "Schema Area" 
        WORD
        INDEX-FIELD "f1" ASCENDING 
  
      */
      create ttw.
  
      def new shared frame f3.
      form ttw.f1[1] with frame f3.
      display ttw.f1[1] with frame f3.
  
      run ext1b.p.
  

• ext1b.p
  

      create ttw.
      def var i as int.
      i = 1.
  
      def shared frame f3.
      form ttw.f1[1] with frame f3.
      display ttw.f1[i] with frame f3.
  

The ttw.f1[i] is not emitting properly.

If the shared frame above is fixed, yet the customer application hit that other limit, we'd need both to be fixed before they can use denorm-extents=true in the conversion. But the original shared frame situation should be separate from the column limitation fix, IMHO.

Greg Shah wrote:

For some reason, the work on 9537a is actually in #8982 and the last notes there suggest that we have a design limitation on 1600 columns in a table. I don't think we can move ahead with 9537a until that issue is resolved.

Well, I think that this design limitation on 1600 columns in a table would be easier to fix and test with 9537a in place.
I can't think of a simple solution at the moment other than the customer rethinking the database schema (or keep normalized extent support). In any case, we'll need to discuss this further on #8982.

7020d was rebased to trunk revision 15813.
7020d contains now all the changes from 7020c + some improvements. There were some leftovers that I decided to fix directly in 7020d because 7020c had some bad commits.

I think that I will continue to use 7020d to retest conversion and runtime for most projects.

So, to summarize: 7020d is based on trunk 15813. 7020d has all the changes from 7020c but with some other improvements.

• Revision 15825 has some fixes for history-entries.
• Revision 15823 and 15824 contains the changes for removing custom denormalized extent support, extracted from 7020c and improved.
• Revisions 15815 - 15822 contains my changes to fix some issues and complete the implementation for expanded extent support.
• Revision 15814 is a flattened commit of 7020c with only all the change made by Ovidiu.

Ovidiu, Eric: Please review.
Ovidiu, I think that it is enough to re-verify the files from your previous review. There were more changes after revision 15822.

7020d was rebased to trunk revision 15830 and is now at revision 15843.

• Implemented inlineExpandedFieldSubscript that will inline a substitution parameter that serves as a subscript for an expanded extent field (this replaces inlineDenormalizedFields).

• FqlToSqlConverter.java
  • multiple usages of appendExpandedName(): the Static Member Access requires that classname.member syntax should be used. The section compares it against instance.member, but the reason is what I think is important: it makes it explicit that the method belongs to respective class instead of assuming it is a local method. This will allow removing the newly added static import.
  • 2926: an empty line sectioning the javadoc should be removed;
  • 3540: since the method now returns a value this should also be reflected in the javadoc. Please add the @return tag.
• UnorderedRowStructure.java: the update fixes a typo but add a new one, at line 216 (poperties).

• I compared it with 7020c (r15824). There are some H entries which in spite of being different, more localised to the file, they express the same thing and have the same numbers. A bit difficult because these two branches are based on different trunk revisions;
• block_properties.rules: copyright year is 2023;
• dmo_common.rules: small typo (and x 2) at line 1851;
• p2o.xml: there is an "s" at line 1677 (instead of Ctrl+S, maybe?)
• IndexSelectionWorker.java: "the" article doubled at line 389;
• CommonAstSupport.java:
  • missing H entry;
  • line 6124: getAnnotation() will return an Object (Long), not a scalar (long), the compare operation might fail (E.g. new Long(3) != new Long(3)). (Note: the code assumes that if node1 has extent annotation, node2 will also have and relies on extent_elem_idx for check. This probably works as expected).
• FieldReference.java: the H number skips 067.
• DmoProxyPlugin.java: the old COMPOSITE can be safely deleted;
• RecordMeta.java: are occurrences where the argument of toArray() was changed from an array of correct size to an empty array. Indeed, some optimisation hints recommend that this practice, but this is actually wrong. If you look at the java.util.AbstractCollection.toArray(T[] a) source and you will notice the following code:
  

        int size = size();
        T[] r = a.length >= size ? a :
              (T[])java.lang.reflect.Array
                   .newInstance(a.getClass().getComponentType(), size);

  In case the passed array is smaller than collection's size, this will dynamically (so slowly) create a new array object of the very same size as the original code. In case the array passed as parameter has enough space from the beginning, it will be used and simply returned. In conclusion, if the size is known, the best performance is to statically create and pass the array of the right size. If the size is not know, the empty array is probably the best choice due to minimal memory footprint. We are in 1st case;
• ExpandedExtentField.java: absence of a blank line between the field members and inside the constructor's javadoc. The file looks OK in 7020c, though;
• Dialect.java, P2JH2Dialect.java, P2JPostgreSQLDialect.java: when compared to 7020c, this files appear to lack some standard blank lines inside the javadocs and between class members.

Fixed. Please see 7020d revision 15846.

I also have rebased 7020c to trunk 15830. Same as 7020d.

Stefanel,
I think 7020d is broken at this moment, RecordMeta probably fails to compile. The reason is the usage of older name nonuniqueIndices instead of nonUniqueIndices in several places.

Also, I see in CommonAstSupport you completely dropped the check for extent. Is this correct?

Ovidiu Maxiniuc wrote:

Stefanel,
I think 7020d is broken at this moment, RecordMeta probably fails to compile. The reason is the usage of older name nonuniqueIndices instead of nonUniqueIndices in several places.

Oh, big oops. I did some changes using just meld and vs code, whitout checking the compilation. I will fix it now, my bad.

Also, I see in CommonAstSupport you completely dropped the check for extent. Is this correct?

Yes, extent_elem_idx annotation that was checked is removed and not used/set anywhere.

Ovidiu Maxiniuc wrote:

Stefanel,
I think 7020d is broken at this moment, RecordMeta probably fails to compile. The reason is the usage of older name nonuniqueIndices instead of nonUniqueIndices in several places.

Fixed, you can update the branch.

7020c & 7020d were rebased to trunk revision 15842.
7020d is now at revision 15858.

Ovidiu, please provide a review for #7020-131.

I have rebased 7020d to trunk revision 15884 and it's now at 15900.

I have one last question:
With denorm-extent, wordTableName was generated as table__extname_1 (since the field name used was extname_1). In expand-extents mode, wordTableName will be table_extname.

• Detect expand-extents mode and append _1 to wordTableName, or
• Make and run an SQL script to rename all existing tables and remove the _1 suffix?

Which do you recommend?

Apart from the situation described above, 7020d has been tested continuously with two projects I had on hand, but I will double-check them to be sure.

We don't want to leave behind any traces of the old modes.

I have rebased 7020d to trunk revision 15902 and it's now at rev.15918.

After the rebase with the latest trunk, I discovered another issue related with fields include/exclude functionality. When .include() is used, the properties are appended explicitly to the FQL, producing something like:
SELECT extTable.recid extTable.ext1 FROM ...
where ext1 is an extent. Previous 7020d didn’t handle this correctly.

To fix it, I updated the assembleSelectClause/assembleComponent logic so that any extent is expanded into its individual elements. For example, extTable.ext1 now becomes:
extTable.ext1[0] extTable.ext1[1] extTable.ext1[2] extTable.ext1[3] extTable.ext1[4]
(and so on, for as many elements as the extent contains).

I committed the fix to 7020d revision 15919.

Ovidiu, could you take a quick look at 7020d revision 15919?

Review of 7020d r15919

I see nothing wrong with the new code. In fact, I slightly remember writing code similar to changes in UnorderedRowStructure, but it may be for another RowStructure.
Good job!

Lorian helped me test 7020d on his project. During the tests, we discovered an issue related to the SQL generation for some datetime-tz extent fields.
The errors were obvious and have been fixed in 7020d, revision 15920.

No more issues found. Are there any other tests that should be done ?

Nice catch!
The changes in 7020d, r15920 are good.

If I am not mistaken, in the case of the GUI project with harness test, the application is provided to us prebuilt, in a docker container. But this task requires some changes in the converted code. Something does not feel right to me.

Ovidiu Maxiniuc wrote:

If I am not mistaken, in the case of the GUI project with harness test, the application is provided to us prebuilt, in a docker container. But this task requires some changes in the converted code. Something does not feel right to me.

Lorian manually performed a full conversion. After that, he updated the JAR files in the Docker container accordingly.

Cool. I presumed that, but needed a confirmation.

Task branch 7020d was rebased to trunk revision 15922.
7020d is now at revision 15940.

Are there any other tests that should be done for 7020d?

Are there any other tests that should be done for 7020d?

• I presume a reconversion is required
  • are there configurations required (remove/add things from p2j.cfg.xml?)
  • this will need to regenerate DMOs and maybe some converted code (?)
  • we need tests with all applications with this
• after, a script to port the database from non-expanded to expanded extent fields.
  • this applies to persistent databases
  • for temporary tables, this is not required
  • for _meta tables ... this is not required because they are in memory; except for meta_user I presume.
  • I presume Lorian's #7504 can be used for that - we need some tests with that.
• customer application configurations should be changed to accommodate this?
  • are there run-time configuration changes required (like in directory.xml)?
  • I presume one shall regenerate AppCDS artifacts or DMO archives for preloading.
• of course, we need a lot of:
  • run-time testing
  • performance testing
  • others like memory leak testing and concurrent testing

Greg, I think such process should be documented and create a task per customer (for us to test).

I think such process should be documented and create a task per customer (for us to test).

Agreed.

It is my understanding that all applications are already using denormalized extents except for these:

• ChUI Regression application
  • As far as I know, the only issue here will be migrating their test and production databases (applying the schema changes). We created the #7328 migration process for this purpose.
• Large GUI Application associated with #8982 and #9537
  • Issue #7020-40 which is fixed by 9537a. I think we still need to get that into trunk, right?
  • Issue #7020-122 (1600 column limit). This is not resolved yet. Until this is resolved, we can't remove support for normalized extents in this application.

Is there any reason to hold back 9537a? Has it been tested enough?

Does 7020d remove normalized extent support completely? If so, then we have to resolve #7020-122 before we merge. Otherwise, we could test and get it merged and then switch to expanded extents for these 2 applications when their database instances have been migrated and the 1600 column issue is resolved.

Greg Shah wrote:

I think such process should be documented and create a task per customer (for us to test).

Agreed.

It is my understanding that all applications are already using denormalized extents except for these:

• ChUI Regression application
  • As far as I know, the only issue here will be migrating their test and production databases (applying the schema changes). We created the #7328 migration process for this purpose.
• Large GUI Application associated with #8982 and #9537

I think i saw some normalized extents in ETF, but Constantin should also confirm that.

Does 7020d remove normalized extent support completely? If so, then we have to resolve #7020-122 before we merge. Otherwise, we could test and get it merged and then switch to expanded extents for these 2 applications when their database instances have been migrated and the 1600 column issue is resolved.

Isn't the _temp database also changed? So more then the ChUI and large customer should be tested, right?

Greg Shah wrote:

Is there any reason to hold back 9537a? Has it been tested enough?

Yes 9537a has been tested and should be safe.

Does 7020d remove normalized extent support completely? If so, then we have to resolve #7020-122 before we merge. Otherwise, we could test and get it merged and then switch to expanded extents for these 2 applications when their database instances have been migrated and the 1600 column issue is resolved.

#7020 doesn't remove normalized support and it is fully compatible as it was before.
The thing changed in 7020d is the denorm-extents from p2j.cfg.xml that was renamed with expand-extents, and became the default option. For normalized extent applications expand-extents should be set to false explicitly, this is one thing that should be communicated before merge for those kind of applications.

Other thing changing that is affecting applications using PostgreSQL and word indexes over extent fields: word table names were renamed (_1 suffix removed). For that I have a suggestion for a solution in #7020-140.
That should be all the changes.
7020d was already tested for conversion and runtime with some applications, except those with normalized extents.
No other configuration changes except what I said above.

Alexandru Lungu wrote:

Does 7020d remove normalized extent support completely? If so, then we have to resolve #7020-122 before we merge. Otherwise, we could test and get it merged and then switch to expanded extents for these 2 applications when their database instances have been migrated and the 1600 column issue is resolved.

Isn't the _temp database also changed? So more then the ChUI and large customer should be tested, right?

Definitely yes.

Stefanel Pezamosca wrote:

The thing changed in 7020d is the denorm-extents from p2j.cfg.xml that was renamed with expand-extents, and became the default option. For normalized extent applications expand-extents should be set to false explicitly, this is one thing that should be communicated before merge for those kind of applications.

So when this gets merged to trunk, the large GUI Application associated with #8982 and #9537 should contain an updated p2j.cfg.xml that includes:

<parameter name="expand-extents" value="false" />

Roger Borrello wrote:

Stefanel Pezamosca wrote:

The thing changed in 7020d is the denorm-extents from p2j.cfg.xml that was renamed with expand-extents, and became the default option. For normalized extent applications expand-extents should be set to false explicitly, this is one thing that should be communicated before merge for those kind of applications.

So when this gets merged to trunk, the large GUI Application associated with #8982 and #9537 should contain an updated p2j.cfg.xml that includes:
<parameter name="expand-extents" value="false" />
until the issues with #8982 and #9537 are fixed. Can you confirm?

Yes, that's correct.

Stefanel Pezamosca wrote:

Yes, that's correct.

Thanks. I have this in the p2j.cfg.xml I will check into the project:

      <!-- This will need to be set when:       -->
      <!-- * #7020 is merged to trunk or 9866a  -->
      <!-- And until:                           -->
      <!-- * #8982 and #9537 are fixed          -->
      <!-- <parameter name="expand-extents" value="false" /> -->

Will the addition of this parameter in p2j.cfg.xml break conversion if #7020 fix is not in place? Such as the special 9866a branch that the customer is using? Or do I need to maintain separate files for that case?

Roger Borrello wrote:

Will the addition of this parameter in p2j.cfg.xml break conversion if #7020 fix is not in place? Such as the special 9866a branch that the customer is using? Or do I need to maintain separate files for that case?

This can be tested but if #7020 fix is not in place this configuration option will just be ignored.

7020d was rebased to latest trunk (rev. 15941) and it's now at revision 15959.

Radu tested 7020d with <parameter name="expand-extents" value="false" /> in p2j.cfg.xml for the large GUI Application associated with #8982 and #9537.
Conversion and runtime testing passed!

I think I can also run ChUI regression tests with 7020d and this option until the end of the week.

Is expand-extents=true now the default and we need to configure projects which can't use this mode explicitly in p2j.cfg.xml?

Constantin Asofiei wrote:

Is expand-extents=true now the default and we need to configure projects which can't use this mode explicitly in p2j.cfg.xml?

Yes, 7020d branch makes expand-extents=true the default option.

Stefanel Pezamosca wrote:

Radu tested 7020d with <parameter name="expand-extents" value="false" /> in p2j.cfg.xml for the large GUI Application associated with #8982 and #9537.
Conversion and runtime testing passed!

I think I can also run ChUI regression tests with 7020d and this option until the end of the week.

Stefanel, did we ever get these ChUI test results? Is there any other testing needed?

Eric Faulhaber wrote:

Stefanel, did we ever get these ChUI test results? Is there any other testing needed?

Unfortunately, I haven't had a chance to test ChUI yet. We have two local machines for running tests, but they were occupied at the time.
I'll try to get the results as soon as possible.

Other apps were already tested.

Stefanel Pezamosca wrote:

Radu tested 7020d with <parameter name="expand-extents" value="false" /> in p2j.cfg.xml for the large GUI Application associated with #8982 and #9537.
Conversion and runtime testing passed!

I think I can also run ChUI regression tests with 7020d and this option until the end of the week.

Given that the large GUI application associated with #8982 and #9537 will need to have expand-extents=false added to the p2j.cfg.xml can I add that setting to the file now and be ready for when this branch is merged to trunk? Or will things break if it's in there beforehand?

Roger Borrello wrote:

Stefanel Pezamosca wrote:

Radu tested 7020d with <parameter name="expand-extents" value="false" /> in p2j.cfg.xml for the large GUI Application associated with #8982 and #9537.
Conversion and runtime testing passed!

Given that the large GUI application associated with #8982 and #9537 will need to have expand-extents=false added to the p2j.cfg.xml can I add that setting to the file now and be ready for when this branch is merged to trunk? Or will things break if it's in there beforehand?

You can add it. It will just be ignored without 7020d.

There may be a regression showing in the ChUI tests. I need to double check.

Stefanel Pezamosca wrote:

Roger Borrello wrote:

Stefanel Pezamosca wrote:

Radu tested 7020d with <parameter name="expand-extents" value="false" /> in p2j.cfg.xml for the large GUI Application associated with #8982 and #9537.
Conversion and runtime testing passed!

Given that the large GUI application associated with #8982 and #9537 will need to have expand-extents=false added to the p2j.cfg.xml can I add that setting to the file now and be ready for when this branch is merged to trunk? Or will things break if it's in there beforehand?

You can add it. It will just be ignored without 7020d.

I confirmed this with a build of the application, so I will commit that change in the customer project.

I am not sure if this is related, but I the ReportDriver gives a NPE error getting the "extent" annotation. Show NPEShow NPE

rpt-new:
     [java] Reflections took 80 ms to scan 1 urls, producing 10 keys and 24 values
     [java] Reflections took 16 ms to scan 1 urls, producing 5 keys and 7 values
     [java] Reflections took 14 ms to scan 1 urls, producing 8 keys and 10 values
     [java] Reflections took 16 ms to scan 1 urls, producing 10 keys and 29 values
     [java] Reflections took 111 ms to scan 1 urls, producing 255 keys and 1262 values
     [java] Reflections took 6 ms to scan 1 urls, producing 11 keys and 23 values
     [java] Reflections took 5 ms to scan 1 urls, producing 6 keys and 17 values
     [java] Reflections took 6 ms to scan 1 urls, producing 15 keys and 45 values
     [java] Reflections took 17 ms to scan 1 urls, producing 34 keys and 168 values
     [java] Reflections took 5 ms to scan 1 urls, producing 2 keys and 26 values
     [java] Reflections took 110 ms to scan 1 urls, producing 463 keys and 2177 values
     [java] Reflections took 88 ms to scan 1 urls, producing 356 keys and 1547 values
     [java] Reflections took 6 ms to scan 1 urls, producing 37 keys and 77 values
     [java] Reflections took 7 ms to scan 1 urls, producing 21 keys and 90 values
     [java] ./abl/rates-report.p.dict
     [java] ./abl/rates.p.dict
     [java] ./cvt/data/hotel.dict
     [java] Elapsed job time:  00:00:03.365
     [java] ./abl/checkout.p
     [java] ./abl/empty-db.p
     [java] ./abl/exit.p
     [java] ./abl/guests.p
     [java] ./abl/login.p
     [java] ./abl/main.p
     [java] ./abl/menu.p
     [java] ./abl/rates-report.p
     [java] ./abl/rates.p
     [java] ./abl/reservations.p
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] ext = target.getAnnotation("extent")
     [java]              ^  { java.lang.NullPointerException }
     [java] ---------------------------
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] tname = execLib("describe_extent", ref)
     [java]         ^  { Expression execution error @1:14 }
     [java] ---------------------------
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] execLib("describe_extent_lvalue", this)
     [java] ^  { Expression execution error @1:9 }
     [java] ---------------------------
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] statName = #(java.lang.String) exec(rpt.multiplexExpr)
     [java]                                ^  { Expression execution error @1:1 }
     [java] ---------------------------
     [java] 
     [java] com.goldencode.p2j.pattern.TreeWalkException: ERROR!  Active Rule:
     [java] -----------------------
     [java]       RULE REPORT      
     [java] -----------------------
     [java] Rule Type :   WALK
     [java] Source AST:  [ b-out-room.guest-ids ] BLOCK/INNER_BLOCK/BLOCK/STATEMENT/KW_CASE/KW_THEN/INNER_BLOCK/BLOCK/STATEMENT/KW_IF/KW_ELSE/INNER_BLOCK/BLOCK/INNER_BLOCK/BLOCK/INNER_BLOCK/BLOCK/ASSIGNMENT/ASSIGN/FIELD_INT/ @664:19 {60129548139}
     [java] Copy AST  :  [ b-out-room.guest-ids ] BLOCK/INNER_BLOCK/BLOCK/STATEMENT/KW_CASE/KW_THEN/INNER_BLOCK/BLOCK/STATEMENT/KW_IF/KW_ELSE/INNER_BLOCK/BLOCK/INNER_BLOCK/BLOCK/INNER_BLOCK/BLOCK/ASSIGNMENT/ASSIGN/FIELD_INT/ @664:19 {60129548139}
     [java] Condition :  ext = target.getAnnotation("extent")
     [java] Loop      :  false
     [java] --- END RULE REPORT ---
     [java] 
     [java] 
     [java] 
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1100)
     [java]     at com.goldencode.p2j.report.ReportDriver.patternEngine(ReportDriver.java:178)
     [java]     at com.goldencode.p2j.report.ReportDriver.doPass(ReportDriver.java:655)
     [java]     at com.goldencode.p2j.report.ReportDriver.newSyntax(ReportDriver.java:621)
     [java]     at com.goldencode.p2j.report.ReportDriver.main(ReportDriver.java:313)
     [java] Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:32 [FIELD_INT id=60129548139]
     [java]     at com.goldencode.p2j.pattern.AstWalker.walk(AstWalker.java:275)
     [java]     at com.goldencode.p2j.pattern.AstWalker.walk(AstWalker.java:210)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.apply(PatternEngine.java:1692)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1578)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1511)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1063)
     [java]     ... 4 more
     [java] Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:32
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:495)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:500)
     [java]     at com.goldencode.p2j.pattern.Rule.executeActions(Rule.java:751)
     [java]     at com.goldencode.p2j.pattern.Rule.coreProcessing(Rule.java:717)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:537)
     [java]     at com.goldencode.p2j.pattern.Rule.executeActions(Rule.java:751)
     [java]     at com.goldencode.p2j.pattern.Rule.coreProcessing(Rule.java:717)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:537)
     [java]     at com.goldencode.p2j.pattern.Rule.executeActions(Rule.java:751)
     [java]     at com.goldencode.p2j.pattern.Rule.coreProcessing(Rule.java:717)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:512)
     [java]     at com.goldencode.p2j.pattern.RuleContainer.apply(RuleContainer.java:597)
     [java]     at com.goldencode.p2j.pattern.RuleSet.apply(RuleSet.java:98)
     [java]     at com.goldencode.p2j.pattern.AstWalker.walk(AstWalker.java:262)
     [java]     ... 9 more
     [java] Elapsed job time:  00:00:16.739Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:1
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:495)
     [java] 
     [java]     at com.goldencode.p2j.pattern.AstSymbolResolver.execute(AstSymbolResolver.java:749)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.exec(CommonAstSupport.java:1941)
     [java]     at com.goldencode.expr.CE545.execute(Unknown Source)
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:398)
     [java]     ... 22 more
     [java] Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:9
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:495)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:500)
     [java]     at com.goldencode.p2j.pattern.NamedFunction.execute(NamedFunction.java:458)
     [java]     at com.goldencode.p2j.pattern.AstSymbolResolver.execute(AstSymbolResolver.java:799)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.execLib(CommonAstSupport.java:1901)
     [java]     at com.goldencode.expr.CE1721.execute(Unknown Source)
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:398)
     [java]     ... 26 more
     [java] Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:14
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:495)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:500)
     [java]     at com.goldencode.p2j.pattern.NamedFunction.execute(NamedFunction.java:458)
     [java]     at com.goldencode.p2j.pattern.AstSymbolResolver.execute(AstSymbolResolver.java:799)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.execLib(CommonAstSupport.java:1901)
     [java]     at com.goldencode.expr.CE1723.execute(Unknown Source)
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:398)
     [java]     ... 32 more
     [java] Caused by: java.lang.NullPointerException
     [java]     at com.goldencode.expr.CE1305.execute(Unknown Source)
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:398)
     [java]     ... 38 more
     [java] 

Perhaps the "extent" is just a red herring, but I thought I'd report it. I am using trunk-16003 and it occurs whether I have <parameter name="denorm-extents" value="true" /> or <parameter name="expand-extents" value="false" /> or nothing.

The front conversion works fine. Perhaps there's an annotation not added until a later phase that the ReportDriver is expecting?

And I apologize if this isn't related to expanded extents at all. I can post to #7143, if necessary.

Roger Borrello wrote:

An I apologize if this isn't related to expanded extents at all. I can post to #7143, if necessary.

I think this a know regression already reported, I don't remember when or where. Edit: I think #7143-1555.

Stefanel Pezamosca wrote:

Roger Borrello wrote:

An I apologize if this isn't related to expanded extents at all. I can post to #7143, if necessary.

I think this a know regression already reported, I don't remember when or where. Edit: I think #7143-1555.

I made a note in #6490-97

#10215 is the task handling this.

Stefanel Pezamosca wrote:

There may be a regression showing in the ChUI tests. I need to double check.

I might have found out what caused the regression. It was a conversion error that I should have seen earlier. It was so obvious after I checked multiple time the changes. In 7020d I removed references to denorm-extent stuff and in a place I have deleted more than I should ....
This scenario showed only for ChUI, so other projects aren't affected.

7020d was rebased to trunk revision 16013. Last revision is 16033.

I have committed migrate_word_table_names.sql to 7020d/tools/scripts and added copyright and history entries, etc.

Branch 7020d was rebased to trunk revsion 16023, now at revision 16045.

In revision 16044, I added the migrate_word_table_names.sql script in tools/scripts/.
In revision 16045, I added support for ScriptRunner to run scripts located in tools/scripts, along with other refactorings/reformatting.

Ovidiu/Constantin, please take a look at 7020d revisions 16043..16045.

Committed to 7020d revision 16046 a change to build.xml to include tools/scripts/*.sql into p2j.jar.

I have reviewed revisions 16043..16046 (LE) of 7020d. I see nothing wrong with the code.

The new file migrate_word_table_names.sql is PostgreSQL specific and probably needs to be named as such (see the other files from same location: fix_indexes_*.sql). OTOH, maybe we need to classify them in separate sub-folder specific for each dialects?

Ovidiu Maxiniuc wrote:

I have reviewed revisions 16043..16046 (LE) of 7020d. I see nothing wrong with the code.

The new file migrate_word_table_names.sql is PostgreSQL specific and probably needs to be named as such (see the other files from same location: fix_indexes_*.sql). OTOH, maybe we need to classify them in separate sub-folder specific for each dialects?

Should I change this now or is it ok to keep it that way at the moment?

Rename the file so that we know the dialect. We will eventually group them by dialects if the number of files in the directory increases drastically.

Ovidiu Maxiniuc wrote:

Rename the file so that we know the dialect. We will eventually group them by dialects if the number of files in the directory increases drastically.

I renamed it to rename_word_tables_postgresql.sql.

Is there anything else to do before merging 7020d to trunk?

Stefanel Pezamosca wrote:

Is there anything else to do before merging 7020d to trunk?

Yes... I know. Too late.

Is there anything ./tools/scripts/import.sh needs to have updated for this task? Should it be calling the rename script?

Roger Borrello wrote:

Stefanel Pezamosca wrote:

Is there anything else to do before merging 7020d to trunk?

Yes... I know. Too late.

Is there anything ./tools/scripts/import.sh needs to have updated for this task? Should it be calling the rename script?

No changes required there.

7020e was merged to trunk rev 16036 - it contained a fix related to loading extent datetime-tz fields (the offset in the result-set was not calculated considering the type's width).

7020e was pushed to xfer.

There is another regression in ETF - collision between the SQL name of an extent field and another field:

def temp-table tt1 field f as int extent 5 field f-1 as char.

create tt1.
tt1.f-1 = "abc".
tt1.f[1] = 1.
tt1.f[2] = 2.
tt1.f[3] = 3.
tt1.f[4] = 4.
tt1.f[5] = 5.
release tt1.
find first tt1.

message tt1.f-1 tt1.f[1] tt1.f[2] tt1.f[3] tt1.f[4] tt1.f[5].

There is also this NPE in ETF:

java.lang.NullPointerException
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter$3.ascent(FqlToSqlConverter.java:3209)
        at com.goldencode.ast.AnnotatedAst$1.notifyListenerLevelChanged(AnnotatedAst.java:3238)
        at com.goldencode.ast.AnnotatedAst$1.next(AnnotatedAst.java:3172)
        at com.goldencode.ast.AnnotatedAst$1.next(AnnotatedAst.java:3105)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.collectNormalizedExtentsAliases(FqlToSqlConverter.java:3369)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.processSelect(FqlToSqlConverter.java:1341)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.processStatement(FqlToSqlConverter.java:1076)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.generateExpression(FqlToSqlConverter.java:2852)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.generateWhere(FqlToSqlConverter.java:2523)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.processSelect(FqlToSqlConverter.java:1632)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.processStatement(FqlToSqlConverter.java:1076)
        at com.goldencode.p2j.persist.orm.FqlToSqlConverter.toSQL(FqlToSqlConverter.java:758)
        at com.goldencode.p2j.persist.orm.Query.lambda$createSqlQuery$0(Query.java:400)
        at com.goldencode.p2j.jmx.NanoTimer.timer(NanoTimer.java:132)
        at com.goldencode.p2j.persist.orm.Query.createSqlQuery(Query.java:415)
        at com.goldencode.p2j.persist.orm.Query.list(Query.java:342)
        at com.goldencode.p2j.persist.Persistence.list(Persistence.java:2169)
        at com.goldencode.p2j.persist.ProgressiveResults.getResults(ProgressiveResults.java:1343)
        at com.goldencode.p2j.persist.ProgressiveResults.moveTo(ProgressiveResults.java:1171)
        at com.goldencode.p2j.persist.ProgressiveResults.moveTo(ProgressiveResults.java:1019)
        at com.goldencode.p2j.persist.ProgressiveResults.next(ProgressiveResults.java:519)
        at com.goldencode.p2j.persist.ResultsAdapter.next(ResultsAdapter.java:162)
        at com.goldencode.p2j.persist.AdaptiveQuery.next(AdaptiveQuery.java:1863)
        at com.goldencode.p2j.persist.CompoundQuery.processComponent(CompoundQuery.java:3637)
        at com.goldencode.p2j.persist.orm.P2JQueryExecutor.executeImpl(P2JQueryExecutor.java:437)
        at com.goldencode.p2j.persist.orm.P2JQueryExecutor.execute(P2JQueryExecutor.java:332)
        at com.goldencode.p2j.persist.orm.P2JQueryExecutor.execute(P2JQueryExecutor.java:260)
        at com.goldencode.p2j.persist.CompoundQuery.retrieveImpl(CompoundQuery.java:3234)
        at com.goldencode.p2j.persist.CompoundQuery.retrieve(CompoundQuery.java:2517)
        at com.goldencode.p2j.persist.CompoundQuery.retrieve(CompoundQuery.java:2401)
        at com.goldencode.p2j.persist.CompoundQuery.next(CompoundQuery.java:1398)
        at com.goldencode.p2j.persist.CompoundQuery.next(CompoundQuery.java:1335)
        at com.goldencode.p2j.persist.AbstractQuery.wrapNext(AbstractQuery.java:4610)
        at com.goldencode.p2j.persist.AbstractQuery.getNext(AbstractQuery.java:2479)
        at com.goldencode.p2j.persist.QueryWrapper.getNext(QueryWrapper.java:5151)

Stefanel, the code in p2o.xml needs to be in an outer while true loop, which exits when no longer isCollision by the inner loop. This test still fails:

def temp-table tt1 field f-1 as char field f1-2 as date field f as int extent 5 .

create tt1.
tt1.f-1 = "abc".
tt1.f[1] = 1.
tt1.f[2] = 2.
tt1.f[3] = 3.
tt1.f[4] = 4.
tt1.f[5] = 5.
release tt1.
find first tt1.

message tt1.f-1 tt1.f[1] tt1.f[2] tt1.f[3] tt1.f[4] tt1.f[5].

Otherwise, about the NPE - is about a FOR EACH tt1, FIRST tt2 where tt2.f1 = tt1.f1[1] (I don't have a recreate, I'll send the full query in an email).

This patch it fixes the NPE and even with the 'wrong' fix in 7020f, ETF search tests now pass, but I don't know how correct it is. I wonder if this is not an existing bug in trunk before 7020d.

=== modified file 'src/com/goldencode/p2j/persist/orm/FqlToSqlConverter.java'
--- old/src/com/goldencode/p2j/persist/orm/FqlToSqlConverter.java       2025-05-20 11:19:03 +0000
+++ new/src/com/goldencode/p2j/persist/orm/FqlToSqlConverter.java       2025-07-15 05:41:32 +0000
@@ -3205,6 +3205,11 @@
                   else
                   {
                      DmoMeta dmoMeta = aliasesMap.get(grandFather.getText());
+                     if (dmoMeta == null)
+                     {
+                        // this is a reference to an alias from an outer SELECT
+                        return;
+                     }
                      String extentName = ast.getParent().getText();
                      Property extProperty = dmoMeta.propsByName.get(extentName);
                      if (extProperty == null)

Constantin Asofiei wrote:

Stefanel, the code in p2o.xml needs to be in an outer while true loop, which exits when no longer isCollision by the inner loop. This test still fails:
[...]

Fixed in 7020f revision 16039.

Otherwise, about the NPE - is about a FOR EACH tt1, FIRST tt2 where tt2.f1 = tt1.f1[1] (I don't have a recreate, I'll send the full query in an email).

This patch it fixes the NPE and even with the 'wrong' fix in 7020f, ETF search tests now pass, but I don't know how correct it is. I wonder if this is not an existing bug in trunk before 7020d.
[...]

I'll try to recreate and investigate.

I managed to reproduce.

def temp-table tt1 field f-1 as char field f1-2 as date field f as int extent 5 .
def temp-table tt2 field f as int index idx_f f.

create tt1.
tt1.f-1 = "abc".
tt1.f[1] = 1.
tt1.f[2] = 2.
tt1.f[3] = 3.
tt1.f[4] = 4.
tt1.f[5] = 5.
release tt1.
find first tt1.

create tt2.
assign tt2.f = 1.
release tt2.

message tt1.f-1 tt1.f[1] tt1.f[2] tt1.f[3] tt1.f[4] tt1.f[5].

FOR EACH tt1, FIRST tt2 where tt2.f = tt1.f[1]:
    display tt1.
END.

 @java.lang.NullPointerException: Cannot read field "propsByName" because "dmoMeta" is null
at com.goldencode.p2j.persist.orm.FqlToSqlConverter$3.ascent(FqlToSqlConverter.java:3209)@ 

I tested with 7020f and 8755c (the branch without 7020 changes). The error reproduces with both of them. So, is this an existing bug/regression in trunk before 7020d?

Ovidiu, please review the .sql and the FqlToSql changes