FWD » Core Development » Database

• schema generation;
• basic runtime support;
• a good percent of SQL server side UDFs. These are a bit time consuming as the language is a bit different than others and some time the result is not as expected (for example when handling null values). It is a known fact that Microsoft as its own way of doing things, especially with their SQL Server. They will be tracked by #9455.

The work was not very structured, I added fixes of the issues as they popped out. I used a local testcase and attempted to get the hotel project running, but I was not yet successful.

With my latest changes from 9456a, I was able to convert and import the hotel database into a MS SQL Server instance. The data looks fine on my visual inspection. I also updated and committed the build.xml and build_db.xml of the hotel_gui project so now the import into MSSQL (including drop/create database, installation of schema and UDFs) works. Of course, the dialect was added to p2j.cfg.xml.

Maybe the following are more schema-related, but the runtime ans the dialect should expect the information to honour a specific convention. I will continue to add support code in runtime after the we decide how the data is actually stored.

• the 1.7K hard limit of the index keys. A warning is emitted for nearly all indexes which contain character data;
• collations. As with MariaDB, we are forced to pick one from the list of available collations. The good pare is that the list seems to be more extensive. Most likely, the customer will have to make a decision here. Also, see below, (nearly) all collations have ci/cs variants.
• case sensitivity. By default, the CS of the columns in a table inherit the collation of the table/instance. We implemented previously the support for this as a computed column as upper(trim(<column>)). From my tests, the trim() looks superfluous, the trimming is automatically applied when sorting. The problem is that this is depending on the collation: if the collation is case insensitive, is really not needed. I am thinking whether we should apply the collation in DDL, at the time the schema is converted. The definitions would look like this:

      ci_text varchar(128) COLLATE sql_latin1_general_cp1_ci_as,
      cs_text varchar(128) COLLATE sql_latin1_general_cp1_cs_as,

  The only problem is that the collation should be known at the time of conversion and not when the database is set up. More than that, to change the collation a schema conversion should be performed in FWD (well, there is also the option to do a text replacement in the files). In spite of this I think we may want to go this way, instead of current solution of injecting the __i<column> computed column for index component;
• nulls last This dialect does not offer such syntax. Therefore we implemented (not currently supported by our FqlToSqlConverter) a workaround by injecting the following expression: case when <column> is null then 1 else 0 end. I was about to get this fixed, but then I realised that the indexes are not defined using this expression. Therefore, they will not be actually used. I experimented with a related idea: what if we define a computed column like this:

      <column>__null as (case when room_type is null then 1 else 0 end)

  This can be added as index component and the SQL to be emitted are not as complex as adding the full case expression. My question here is: while this looks more elegant for columns which are index components, needing to sort on other column will also require this computed column, reaching the point where all columns will have this pair. And if we stick to current case-sensitivity solution (using _i prefix), for the character fields we will end up having 3 columns. For example:

     guest varchar(128),                -- case sensitive value
     __iguest as upper(rtrim(guest)),   -- case insensitive index component
     guest__null as (case when guest is null then 1 else 0 end),-- helper for sorting 'null last'

• MSSQL does not allow multiple null in unique indexes.
  The solution we used to work around this issue was to force injection of PK field as the last index component. This way, all records become unique so allowing the records which otherwise would break the unique constraint to be stored in database. This is just temporary, NOT the final fix: since all records are seen now as unique, the index is not putting the unique constraint any more.

MSSQL has this syntax - see https://stackoverflow.com/questions/767657/how-do-i-create-a-unique-constraint-that-also-allows-nulls :

CREATE UNIQUE NONCLUSTERED INDEX idx_yourcolumn_notnull
ON YourTable(yourcolumn)
WHERE yourcolumn IS NOT NULL;

We can try and add non-mandatory fields in an unique index to the WHERE clause, but what happens if there are 3 non-mandatory fields in an unique index, and field 1 is null for 1/3rd of records, 1/3rd second field is null, the other the third field, with no overlap - then then index will not index anything.

A 'new' old issue: the MSSQL and lack of support for boolean literals. That is the reason the Dialect.supportsBooleanDatatype() was implemented. It generally return true, except for MSSQL dialect. There were fragments of code which handled this, but since the dialect support was not maintained, these were not running correctly.

So the data-type used for storing logical values in MSSQL is bit. However, this is a synonym of int(1) and not compatible with logical operators (and, or, not). There is no true or false literals. When an such a parameter is passed to one of out UDFs, it will be replaced with 0 or 1, respectively. However, when a logical expression requires a literal it must be replaced by a 'native' logical expression. The simplest solution we found was to test against 1 (aka true) in expressions like: (field = 1) and (boolFn(...) = 1), (?subst = 1) or (1 = 1), etc.

I updated the FQL preprocessor so that usages of boolean values are processed accordingly, but this needs also testing and possible future adjustments for specific syntaxes.

A point Constantin highlighted is the occurrences of SQL statements hardcoded or composed in FWD code for internal management. These should be dialect independent, but should written in such a way to avoid the logical literals. The good part is that they are mostly used for temp-tables and metas (which always use H2).

One more thing, related to paging. We knew that MSSQL does not have a LIMIT / OFFSET syntax for query pagination, instead it uses TOP (like SELECT TOP 5 ... to select only the first 5 rows). Since then, they added a new option for ORDER BY. It looks like this (I simplified a bit):

ORDER BY <order_by_expressions> [ OFFSET <offset> ROWS [ FETCH NEXT <limit> ROWS ONLY ] ] 

• the order of limit and offset is reversed compared to other dialects;
• if the limit is desired, offset is mandatory (also reversed);
• both options require the presence of a parent ORDER BY clause.

In 9456a I have implemented most of the support for first two bullets, assuming that the ORDER BY will be added to all queries, anyway. However, last night, while smoke testing the hotel_gui project back-ended by SQL Server database I realised that the unique queries do not inject this clause to it (from a logical perspective, that is not necessary as the result is expected to be... unique, therefore nothing to sort). I am working on solving this issue, most likely the solution is to force the injection of an ORDER BY recid clause for UNIQUE queries, hopefully only for this dialect.

The ORDER BY issue was partially resolved. In case of UNIQUE finds, the HQL bundle was expressly setting the sorting to null. I enabled it to add sorting on PK for MSSQL. Although I am thinking whether pagination is necessary for these queries. Logically, limit and offset are pointless here. The reason to have a limit 2 is to check whether the result is indeed unique without fetching the full table. So yes, the current solution I implemented is the best.

Good news: now the hotel_gui starts and you can switch to all tabs to see the imported data.

• I hit another MSSQL implementation restriction, one we were not aware of in the iteration of several years ago: the releaseSavepoint() method in JDBC 3.0 is not supported by Microsoft JDBC Driver for SQL Server 🙁
• and one I could not find in documentation: the FORWARD_ONLY MSSQL queries cannot call ResultSet.isLast() because it is 'not scrollable'.

Related to my note #9456-8: with my current changes in 9456a, I encountered in hotel_runtime a query where predicate contains:

        dbo.matches_ssb(upper(reservatio0_.guest), upper(reservatio0_.guest), 1) = 1 or 
            not ((upper(rtrim(reservatio0_.guest)) is not null) = 1) and 1 = 1) and 

Of course, a bit of preprocessing can be done here and drop completely and 1 = 1, but the problem will still exist for not ((guest is null) = 1). This should be preprocessed and the ideal result would be as simple as (guest is not null).

• I hit another MSSQL implementation restriction, one we were not aware of in the iteration of several years ago: the releaseSavepoint() method in JDBC 3.0 is not supported by Microsoft JDBC Driver for SQL Server 🙁
• and one I could not find in documentation: the FORWARD_ONLY MSSQL queries cannot call ResultSet.isLast() because it is 'not scrollable'.

Please update the master list of SQLServer limitations. If there is no master list of SQLServer limitations, please create one.

Created a table with SQL Server specific issues in Limitations and Workarounds.

As reported previously, I encountered some issues while fixing the FWD support for logical/boolean expressions in MSSQL dialect.

• As we know, there is no boolean data type, instead bit (which is an integer type, synonym to int(1)) is used. Because of this, it is NOT compatible with AND, OR, NOT. Therefore if a logical field needs to be checked (find first book where book.isactive), the workaround is to rewrite it as a logical value: select * from book where book.isactive = 1. This is the simple case.

• A more complicated problem occurs when the result of a logical expression is used as parameter like in: find first book where string(book.isactive and p1) eq "yes". Without specific preprocessing, the SQL query will be
  

  select * from book where dbo.tostring_b((book.isactive = 1) and ?) = 'yes'

  The problem is that the result is of SQL-internal logical type and cannot be passed to tostring_b(bit). We cannot handle this using cast or convert. The only solution is to rewrite it as:

  select * from book where dbo.tostring_b(case when ((book.isactive = 1) and ?) then 1 else 0 end) = 'yes'

• There is also a 3rd level issue: the native logical values are not comparable. Let's take the following example: find first book where (book.book-id eq 101) = (book.isbn eq "102") no-error. For other dialects, this would be converted as:

  select * from book where (book.book-id = 101) = (book.isbn = '102')

  Analysing the predicate, there are two boolean expression tested for equality. But SQL Server refuses to compare them (using the other relational operators !=, >, <=, etc will not work as well). For = we could have replace it with xor operator, if that existed. The only solution to this problem is, again, to use inline case so that the query will become:

  select * from book where (case when (book.book-id = 101) then 1 else 0 end) = (case when (book.isbn = '102') then 1 else 0 end)

I created the following table with nodes and their parents when doing the FQL preprocessing:

I think applying all rules according to the above table will generate correct queries for SQL Server. The big problem is the resulting query complexity which directly (may) affects performance.

Ovidiu, can 9456a be moved to review?

About the data/dump/ database folder missing: if you haven't already fixed it, or you think it takes more time, please postpone it. I'll change the scripts to automatically create the folder when the app is setup.

Ovidiu, first results on conversion: nulls last is added on indexes for H2 dialect (this includes the meta schema and I assume all temp-table DDL). Why this change?

This is not intentional. I wonder why I did not notice it. It is a regression. Thank you for reporting it. I'm fixing it ASAP.

Constantin Asofiei wrote:

Ovidiu, first results on conversion: nulls last is added on indexes for H2 dialect (this includes the meta schema and I assume all temp-table DDL). Why this change?

This still puzzles me. My question is why the option was not added previously. In Dialect.getCreateIndexString() we call orderByNulls() for each component and the implementation of orderByNulls() in P2JH2Dialect it calls the super method then, unconditionally, sb.append(asc ? " nulls last" : " nulls first");. This hasn`t changed.
Yet, the trunk code seems to ignore this line.

I can easily drop the overriding orderByNulls() method from P2JH2Dialect, but I still need a reason for why it was not executed in trunk.

• Dialect has this:
  

     public int orderByNulls(StringBuilder sb,
                             String expression,
                             String field,
                             boolean asc,
                             boolean forceAsc,
                             boolean mandatory)
  

  • this is called from Dialect.getCreateIndexString
  • P2JH2Dialect overrides only this:
    

       public int orderByNulls(StringBuilder sb, String expression, String field, boolean asc, boolean forceAsc)
       {
    

So you either changed the overrides in P2JH2Dialect or now you are calling the orderByNulls which is overriden in P2JH2Dialect.

Ovidiu, looking at the udfs tests I found a couple of problems:

1) When creating the udfs aliases, we get with reflection the functions from Functions.java and we add the suffix based on the method parameters.
However there are cases when the method has as parameter a primitive data type (int, long, etc), but this cases aren't handled by SQLServerHelper.getSqlDecoration or SQLServerHelper.getSqlType

=== modified file 'src/com/goldencode/p2j/persist/dialect/SQLServerHelper.java'
--- old/src/com/goldencode/p2j/persist/dialect/SQLServerHelper.java    2025-05-31 00:17:02 +0000
+++ new/src/com/goldencode/p2j/persist/dialect/SQLServerHelper.java    2025-07-15 14:51:18 +0000
@@ -199,9 +199,11 @@
             return "bit";

          case "java.lang.Integer":
+         case "int":
             return "int";

          case "java.lang.Long":
+         case "long":
             return "bigint";

          case "java.lang.String":
@@ -238,9 +240,11 @@
             return 'b';

          case "java.lang.Integer":
+         case "int":
             return 'i';

          case "java.lang.Long":
+         case "long":
             return 'l';

          case "java.lang.String":

Thank you for noticing this issue.

The SQLServerHelper class has more issues than this. It creates a list of DDLs for a set of CLR/IKVM function aliases which we co not support any more. I have some pending changes to resolve that.

There's a second problem here. The parameters of Functions (and Operators) are usually values from columns of H2 database. This will work correctly only those columns are NOT NULL. Since this assumption does not always hold, the parameters of the UDFs should be nullable (wrappers). The Functions must be updated to reflect that or we will face NPEs when that happens (H2 only). Due to this, I've chosen to keep your changes in getSqlDecoration() (getSqlType() is no longer present).

Is there a second one?

Branch 9456a was rebase to trunk r16048. Current revision is 16078.

Ovidiu, is src/com/goldencode/p2j/persist/dialect/P2JSQLServer2008Dialect.java.THIS.THIS supposed to exist?

No. bzr added it without my knowledge. I never use add during rebase. I think it was confused because the file was updated in trunk, but it was removed in the branch.
Removed in r16079.

Eduard, please double-check your pending UDF changes for SqlServer and after that commit them to 9456a. We will merge 9456a after that.

Committed on 9456a, rev. 16089. Added some udfs functions overloads, and small functions fix ups. Only sqlserver/udfs.sql changes.

I peeked at r16089. There are some updates which do not seem logical (although they will work with the default CI collation):

-          WHEN LOWER(@interval_unit) IN (N'year', N'years') THEN DATEADD(YEAR, @interval_amount, @datetime)
+          WHEN LOWER(@interval_unit) IN (N'YEAR', N'YEARS') THEN DATEADD(YEAR, @interval_amount, @datetime)
-          WHEN LOWER(@interval_unit) IN (N'month', N'months') THEN DATEADD(MONTH, @interval_amount, @datetime)
+          WHEN LOWER(@interval_unit) IN (N'MONTH', N'MONTHS') THEN DATEADD(MONTH, @interval_amount, @datetime)
[...]

Branch 9456a was merged into trunk as 16062 then it was archived.

I have created 9456b from trunk 16062 for subsequent changes.

Ovidiu, the following statement does not work after 9456a changes (with tstcasesdb database):
- CURRENT-VALUE (customsequence10) = 10

Test error:

╷
├─ JUnit Jupiter ✔
├─ JUnit Vintage ✔
├─ JUnit Platform Suite ✔
└─ FWD Test ✔
   └─ tests.TestCurrentValueSequenceSet ✔
      └─ testCurrentValueSequence ✘ [00000001:00000005:bogus-->local/tstcasesdb/primary] Error executing SQL statement:  select setval(customsequence10, 10)

ant importdb error:

     [exec] CREATE SEQUENCE
     [exec] DROP SEQUENCE
     [exec] CREATE SEQUENCE
     [exec] DROP SEQUENCE
     [exec] CREATE SEQUENCE
     [exec] DROP SEQUENCE
     [exec] CREATE SEQUENCE
     [exec] DROP SEQUENCE
     [exec] CREATE SEQUENCE
     [exec] psql:ddl/schema_table_tstcasesdb_postgresql.sql:237: NOTICE:  sequence "customsequence10" does not exist, skipping
     [exec] psql:ddl/schema_table_tstcasesdb_postgresql.sql:239: NOTICE:  sequence "customsequence100" does not exist, skipping
     [exec] psql:ddl/schema_table_tstcasesdb_postgresql.sql:241: NOTICE:  sequence "customsequencecycle" does not exist, skipping
     [exec] psql:ddl/schema_table_tstcasesdb_postgresql.sql:243: NOTICE:  sequence "maxcustomerseq" does not exist, skipping

Complete codeComplete code

BLOCK-LEVEL ON ERROR UNDO, THROW.

CLASS tests.TestCurrentValueSequenceSet:
    @Test.
    METHOD PUBLIC VOID TestCurrentValueSequence():
        CURRENT-VALUE (customsequence10) = 10.
    END METHOD.
END CLASS.

This happens both on SQL Server and Postgresql.

I have fixed syntax for set sequence value in PostgreSQL dialect in 9457c/16098. I think we need to get this patch into trunk, as well.
The SQL Server dialect does not seem to be affected, in my tests.

Committed on 9456b, rev. 16071.

Revision 16071 contains changes from 10283a, this fixes the coalesce, and fix for the problem from #10283-18.

Eduard - there are usages of 'lower' in udf/sqlserver/udfs.sql where it matches with an uppercase text, see:

          WHEN LOWER(@interval_unit) IN (N'YEAR', N'YEARS') THEN DATEADD(YEAR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MONTH', N'MONTHS') THEN DATEADD(MONTH, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'WEEK', N'WEEKS') THEN DATEADD(WEEK, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'DAY', N'DAYS') THEN DATEADD(DAY, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'HOUR', N'HOURS') THEN DATEADD(HOUR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MINUTE', N'MINUTES') THEN DATEADD(MINUTE, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'SECOND', N'SECONDS') THEN DATEADD(SECOND, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MILLISECOND', N'MILLISECONDS') THEN DATEADD(MILLISECOND, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'YEAR', N'YEARS') THEN DATEADD(YEAR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MONTH', N'MONTHS') THEN DATEADD(MONTH, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'WEEK', N'WEEKS') THEN DATEADD(WEEK, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'DAY', N'DAYS') THEN DATEADD(DAY, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'HOUR', N'HOURS') THEN DATEADD(HOUR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MINUTE', N'MINUTES') THEN DATEADD(MINUTE, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'SECOND', N'SECONDS') THEN DATEADD(SECOND, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MILLISECOND', N'MILLISECONDS') THEN DATEADD(MILLISECOND, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'YEAR', N'YEARS') THEN DATEADD(YEAR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MONTH', N'MONTHS') THEN DATEADD(MONTH, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'WEEK', N'WEEKS') THEN DATEADD(WEEK, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'DAY', N'DAYS') THEN DATEADD(DAY, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'HOUR', N'HOURS') THEN DATEADD(HOUR, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MINUTE', N'MINUTES') THEN DATEADD(MINUTE, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'SECOND', N'SECONDS') THEN DATEADD(SECOND, @interval_amount, @datetime)
          WHEN LOWER(@interval_unit) IN (N'MILLISECOND', N'MILLISECONDS') THEN DATEADD(MILLISECOND, @interval_amount, @datetime)

Please check the same for 'upper' (to not match with an lowercase text)

Please fix this in 9456b these cases.

All function names look to be lowercase, except this:

grep "CREATE FUNCTION" udf/sqlserver/udfs.sql | cut -d' ' -f 3 | cut -d'(' -f 1  | grep -E "[A-Z]" 
dbo.getFWDVersion_

Committed on 9456b, rev. 16076.

Convert booleans for dirty database in case primary database does not support boolean type. Refs. #10283-28. Issue related to dirty database fql bundle issue.

For #9456-45, I remember there was a weird case in testcases/udfs/4gl testcases. Looking into it.

Committed on 9456b, rev. 16077. AddInterval and GetInterval function fix-ups

Eduard Soltan wrote:

Committed on 9456b, rev. 16076.
Convert booleans for dirty database in case primary database does not support boolean type. Refs. #10283-28. Issue related to dirty database fql bundle issue.

Eduard,
I saw you tried to fix the issues related to dirty database.

I am not sure that is the correct approach. In all cases the dialect and its convertBoolean parameters are passed along stack-trace except for one case: in FQLHelper:434: the FQLPreprocessor.get() is invoked with dirtyDialect, but convertBoolean from primaryDialect. This is most likely incorrect: if the primary is MSSQL, this will cause the booleans to be converted for the dirty database which is always H2, causing invalid statements to be generated.

I understand that there is an issue with the number of parameters #10283-28 (DIRTY DATABASE BUNDLE ISSUE), but IMO, this is not the right solution.

Ovidiu Maxiniuc wrote:

This is most likely incorrect: if the primary is MSSQL, this will cause the booleans to be converted for the dirty database which is always H2, causing invalid statements to be generated.

I thought that the generated FQLs by convertBoolean are syntactically different, but identical from a semantic point of view.

And saw that majority of statements generated by convertBoolean are (1 = 0, 1 = 1, case ? then 1 else 0, etc) which are also valid in other dialects.

What is the statement which would not be valid in other dialects?

Yes, that is only a particular case of transformation. Also, many of the transformations, although more complicated (therefore possible having negative effects on performance) are still syntactically correct for other dialects (H2 in this case).

But that is not always the case. There are cases when an UDF (in H2 that is a method from Functions.java) is expecting a boolean value and convertBoolean will transform the logical expression into a numeric (BIT is actually identical to int(1)). In these cases the execution will fail. If we are lucky, H2 will use an implicit conversion from int to boolean. I did not give it a try, but taking into consideration H2 is Java and Java is strong-typed, chances are not good.

Ovidiu Maxiniuc wrote:

But that is not always the case. There are cases when an UDF (in H2 that is a method from Functions.java) is expecting a boolean value and convertBoolean will transform the logical expression into a numeric (BIT is actually identical to int(1)). In these cases the execution will fail. If we are lucky, H2 will use an implicit conversion from int to boolean. I did not give it a try, but taking into consideration H2 is Java and Java is strong-typed, chances are not good.

Understood, I found something in FqlHelper.postProcessBundle,

// Store query substitution parameter indices.
// TODO:  currently, these are shared, and not dialect specific, but only because we do not inline
//        parameters from this class.  Were this to change, we would need to maintain separate
//        [ParameterIndices] objects for the primary and dirty database statements.

I think I should take the following approach, and maintain separate ParameterIndices for the primary and dirty database statements.

Committed on 9456b, rev. 16081.

Reverted boolean conversion for dirty database, separated parameters for FQL query into primary and dirty database.

Ovidiu, 9456b/16097 looks good (I've committed some formatting fixes).

Please commit the dialect NPE fix from #7143-1637, after that we will start testing.

• the "FirstNumber".equals(firstColumnLabel) approach (appears twice now in the code) I think needs to be in the dialect.
• the releaseSavepoint part - for non-local dbs, that should also check via JDBC for the dialect.

Constantin Asofiei wrote:

• the releaseSavepoint part - for non-local dbs, that should also check via JDBC for the dialect.

In other words: if the dialect can't be resolved, get it from JDBC, regardless if local or non-local

OK, I will remove the outer if.

• I see plenty of isLimitBeforeOffset checks that alter the behavior of persistence code. Sometimes it is not clear (at least to me) how the relative position of LIMIT vs OFFSET actually related to the conditional.
  • isOrderByRequired returns true if !isLimitBeforeOffset(). Reading this code alone doesn't make much sense to me. The default it to use ORDER BY, but if the query has at most 1 record, then this should return 1 (this is documented in the javadoc). I can't tell how isLimitBeforeOffset relates to this flow.
  • FQLHelper generated a different clause in whereClause (that apparently seems null safe) depending on isLimitBeforeOffset. This is again a bit confusing. I can't tell why toWhereExpendeNullExpression would be favored vs toWhereExpression if isLimitBeforeOffset holds.
  • others
  • I personally can infer an intent of differentiating SQLServer from other dialects when dealing with SQL composition, but I think some other (more suggestive) API should be used instead of isLimitBeforeOffset. Something like forceOrderBy or forceExpandedNullExpressionInWhere (though I prefer a shorter name:).
• I like the idea of moving code out of RAQ.executeImpl in smaller methods (i.e. populateParameters). Hooray!
• typo in toWhereExpendeNullExpression?

Is toWhereExpendeNullExpression a AND / OR based null check vs toWhereExpression that is a COALESCE based null check? In other words, will SQL Server use AND / OR to make where clauses null safe, while other dialects will still use COALESCE? This is how I interpret the code. If that is the case, note that toWhereExpendeNullExpression is the implementation in FWD before #6667 that eventually changed that in COALESCE based SQL. See #8914-5.

If yes, then are all these changes regarding parameter indices and bundles done to accommodate COALESCE for H2 and AND/OR SQL for SQLServer? In this case, I really think this is a huge over-complication with little to no gain.

My conclusion: If COALESCE is something that is not supported by SQL Server, then I really think we should rather drop it entirely and unify the SQL generation across all dialects. It doesn't make sense to have COALESCE for PG, MariaDB and H2, but AND/OR clauses for SQLServer. Personally, I even think that (as mentioned in #8914-22):

Performance-wise: doing "null-check augmentation" is a performance constraint we need to live with to be able to replicate the 4GL behavior 100%. However, I am inclined to think that the planning engine is more tolerant with OR instead of COALESCE. PostgreSQL is able to do OR merge part of the planner, especially as there is a small number of clauses. Your example in #8914-12 os a bit opaque, because I don't know what fields are actually indexed in your example. I would go ahead with a slight more investigation on this.

I added Tomasz as I watcher. Tomasz, can you review 9456b (SortCriterion.toWhereExpendeNUllExpression)? Can we rather use such AND/OR approach for all dialects and eventually replace the OG SortCriterion.toWhereExpression with this implementation entirely? The over-complication of having different parameter indices and different bundles for different dialects (especially for dirty H2 vs persistent SQLServer) seems unreasonable.

Alexandru Lungu wrote:

Is toWhereExpendeNullExpression a AND / OR based null check vs toWhereExpression that is a COALESCE based null check? In other words, will SQL Server use AND / OR to make where clauses null safe, while other dialects will still use COALESCE?

That is right.

If yes, then are all these changes regarding parameter indices and bundles done to accommodate COALESCE for H2 and AND/OR SQL for SQLServer? In this case, I really think this is a huge over-complication with little to no gain.

Not only for that, there are cases when user defined where clause might be different in SqlServer dialect and H2 bundles.

def var ch1 as char.
ch1 = ''.

find first book2 where ch1 = ''.

Active bundle generated is:

PRIMARY STATEMENTS
   select book2.recid from Book2__Impl__ as book2 where 1 = 1 order by book2.recid asc
DIRTY STATEMENTS
   select book2.recid from Book2__Impl__ as book2 where ?0 order by book2.recid asc
GETTERS

Because we can't have such sql statements in SqlServer select * from table where true, whilst in H2 or other dialects we can.

In fact the COALESCE OR EXTENDED NULL CHECK part that is generated by fwd will be exactly the same for both dialects in the bundle (COALESCE if primary database is pg, mariadb, etc and EXTENDED NULL CHECK if primary database is SQLServer).

Alexandru Lungu wrote:

Review of 9456b:

• I see plenty of isLimitBeforeOffset checks that alter the behavior of persistence code. Sometimes it is not clear (at least to me) how the relative position of LIMIT vs OFFSET actually related to the conditional.
• isOrderByRequired returns true if !isLimitBeforeOffset(). Reading this code alone doesn't make much sense to me. The default it to use ORDER BY, but if the query has at most 1 record, then this should return 1 (this is documented in the javadoc). I can't tell how isLimitBeforeOffset relates to this flow.

To use the following syntax offset ? row fetch next ? rows only in a sql query in SQLServer, we must have an order by defined before that otherwise we get a syntax error.

FQLHelper generated a different clause in whereClause (that apparently seems null safe) depending on isLimitBeforeOffset. This is again a bit confusing. I can't tell why toWhereExpendeNullExpression would be favored vs toWhereExpression if isLimitBeforeOffset holds.

Oops! Here the intent was to use !primaryDialect.supportsBooleanDatatype(), but from behaviour point of view they are the same thing since the only class that overrides supportsBooleanDatatype and isLimitBeforeOffset is P2JSQLServer2012Dialect.

Not only for that, there are cases when user defined where clause might be different in SqlServer dialect and H2 bundles.

Hmm, I understand. There was a TODO as well (that can be removed in #9456).

      // TODO:  currently, these are shared, and not dialect specific, but only because we do not inline
      //        parameters from this class.  Were this to change, we would need to maintain separate
      //        [ParameterIndices] objects for the primary and dirty database statements.

So we should definitely have different indexing of parameters for dirty vs primary statements anyway - and this is way more important for SQLServer.

I am OK then with the changes to RAQ to distinguish between dirty and primary parameters. But this still doesn't justify the need of two different implementations (with and without COALESCE). It just motivates the need of different parameters for the base WHERE. In order words, can't we just always use your toWhereExpendeNUllExpression for good? IMHO, it won't hurt H2, MariaDB or PostgreSQL. But this will require a bit more testing and performance analysis.

To use the following syntax offset ? row fetch next ? rows only in a sql query in SQLServer, we must have an order by defined before that otherwise we get a syntax error.

isLimitBeforeOffset doesn't imply that the ORDER BY is mandatory. Other (unsupported) dialects might allow limit before offset without enforcing order by. From my POV, these two principles are distinct. Please use a isOrderByMandatory or something like that instead - with the risk of bloating the dialect, I favor clarity :)

Alexandru Lungu wrote:

I am OK then with the changes to RAQ to distinguish between dirty and primary parameters. But this still doesn't justify the need of two different implementations (with and without COALESCE). It just motivates the need of different parameters for the base WHERE. In order words, can't we just always use your toWhereExpendeNUllExpression for good? IMHO, it won't hurt H2, MariaDB or PostgreSQL. But this will require a bit more testing and performance analysis.

That was more a decision taken in order to not affect other dialects.

Eduard, in toWhereExpendedNullExpression, isn't the last parenthesis unbalanced? You have whereClause.append(" or (");, but whereClause.append(")"); once and whereClause.append(")"); a second time.

Ah nevermind, is the one added at the beginning for withNull.

I reviewed the changes and toWhereExpendedNullExpression seems safe for all dialects. The things that drove the COALESCE change were that ? > p or <augment> may be an undefined operation on NULL data type. I mean, ? > p technically evaluates to NULL if ? or p are NULL, but NULL or <augment> is quite strangely evaluating to <augment> (just like NULL is false). This applies to AND as well.

• TRUE OR NULL should be TRUE
• FALSE OR NULL should be FALSE or NULL
• NULL OR NULL should be FALSE or NULL
• TRUE AND NULL should be FALSE or NULL
• FALSE AND NULL should be FALSE or NULL
• NULL AND NULL should be FALSE or NULL
• Bonus: where null should work, but not select that respective row.
• Do not rely on NOT.

Is this something that is guaranteed by SQL standards; I don't think so. But it is quite widely supported.

For SQL Server (https://learn.microsoft.com/en-us/sql/t-sql/language-elements/or-transact-sql?view=sql-server-ver17), OR and AND seems to respect this requirement.
For PostgreSQL (https://www.postgresql.org/docs/current/functions-logical.html), OR and AND seem to respect this requirement.
For MariaDB (https://mariadb.com/docs/server/reference/sql-structure/operators/logical-operators/or), OR and AND seem to respect this requirement.
For H2, I don't have the required documentation, but the ConditionAndOr code seems not to respect this requirement:

case OR: {
   if (l.getBoolean()) {
      return ValueBoolean.TRUE;
   }
   r = right.getValue(session);
   if (r.getBoolean()) {
      return ValueBoolean.TRUE;
   }
   if (l == ValueNull.INSTANCE || r == ValueNull.INSTANCE) {
      return ValueNull.INSTANCE;
   }
   return ValueBoolean.FALSE;
}

For COALESCE, we don't assume any behaviors of AND and OR, but just the sole existence of such construct and of the boolean expressions. PostgreSQL, MariaDB, SQLServer and H2 do support this construct, but SQLServer does not support boolean expressions.

The good part is that FWD-H2 is in-house and we can change the AND/OR behavior to match the the "null as falsy" invariant we need. Do we have time for that?

Alexandru, is this something which prevents 9456b to be merged to trunk? Does it affect other dialects than SQLServer?

Constantin Asofiei wrote:

Alexandru, is this something which prevents 9456b to be merged to trunk? Does it affect other dialects than SQLServer?

It is functionally OK from my POV. I am OK to postpone work on #9456-67. In the current state, we will have a slower job in testing invalidation of queries mostly because SQLServer will generate different SQL then other dialects. And I do not mean the 1 = 1 instead of true due to the lack of boolean, but because of COALESCE vs OR/AND syntax.

isLimitBeforeOffset doesn't imply that the ORDER BY is mandatory. Other (unsupported) dialects might allow limit before offset without enforcing order by. From my POV, these two principles are distinct. Please use a isOrderByMandatory or something like that instead - with the risk of bloating the dialect, I favor clarity :)

This is the only point to still be assessed.

• FQLHelper.createBundle() -> isOrderByMandatory(); (new method)
• FQLHelper.fillBundleLists() -> supportsBooleanDatatype(); (coalesce issue)
• PreselectQuery.isOrderByRequired -> isOrderByMandatory(); (new, same as above)
• RandomAccessQuery.executeImpl() -> supportsBooleanDatatype()
• Query.createSqlQuery() -> remains as it is.

Changes applied. Pushed up to revision 16106 after rebase to latest trunk.

Please review. I think we need to redo the runtime regression tests again, just to be on safe side.

Ovidiu Maxiniuc wrote:

Please review. I think we need to redo the runtime regression tests again, just to be on safe side.

Run again chui regression tests again, and it went successfully.

Alexandru Lungu wrote:

I added Tomasz as I watcher. Tomasz, can you review 9456b (SortCriterion.toWhereExpendeNUllExpression)? Can we rather use such AND/OR approach for all dialects and eventually replace the OG SortCriterion.toWhereExpression with this implementation entirely? The over-complication of having different parameter indices and different bundles for different dialects (especially for dirty H2 vs persistent SQLServer) seems unreasonable.

I've reviewed SortCriterion.toWhereExpendeNUllExpression and logically it seems identical to SortCriterion.toWhereExpression, except it requires tripling of column reference - instead of doubling. I guess it should not be a problem for a modern database.

Which one is better in the end - I dont know :), I liked COALESCE as it allows for a special comparison which includes NULLs - for databases supported until now.
I guess it needs to be checked with each database engine's query optimization engine - COALESCE had this problem where indices were not selected when expected and full table scan was executed instead.

Ovidiu Maxiniuc wrote:

I am about to alter 9456b and address the following 5 occurrences of current isLimitBeforeOffset():

Ovidiu, do we still need 9456b?

No. I checked the branch and all changes are found at least in 9457c. I archived the branch as 'dead'.

Ovidiu Maxiniuc wrote:

No. I checked the branch and all changes are found at least in 9457c. I archived the branch as 'dead'.

Please unarchive it, rebase it and if there is nothing else remaining (all changes are in trunk), then it can be archived as 'dead'. Otherwise we need to keep it.

OK, I reverted the archive and rebased the branch. After fixing a lots of conflicts (some revisions were reverted inside 9456b), the final revision (16361) is identical to current trunk.
Therefore it was re-archived again as dead.