Handle database-specific SQL functions/operators by simple abstraction

@killes: it is also a critical issue for using SUBSTRING: for both Oracle and DB2, it is called as SUBSTR. BTW, comment.module is the only place that we use SUBSTRING, among all core module. maybe we should consider re-factor its programming logic, and implement this sorting by other method?

according to indeed studying about ADOdb, i found that this is something can be overcome by using database specific abstract function. e.g. we can provide a function call as db_substr(), which will return a string for substring function, according to database specific syntax (MySQL: substr() or substring(); Oracle: substr(); MSSQL: substring()). please take these as reference (http://phplens.com/lens/adodb/docs-adodb.htm#substr and http://phplens.com/lens/adodb/tips_portable_sql.htm):

substr

This is not a function, but a property. Some databases have "substr" and others "substring" as the function to retrieve a sub-string. To use this property:

<?php
  $sql = "SELECT ".$db->substr."(field, $offset, $length) from table";
  $rs = $db->Execute($sql);
?>

For all databases, the 1st parameter of substr is the field, the 2nd is the offset (1-based) to the beginning of the sub-string, and the 3rd is the length of the sub-string.

Portable Native SQL

ADOdb provides the following functions for portably generating SQL functions as strings to be merged into your SQL statements (some are only available since ADOdb 3.92): ...

and so i will postpone this issue into D7. BTW, i will also include this research result in http://drupal.org/node/172541 as D7 database abstract layer technical preview, so we will able to judge its needs by using code, testing and benchmarking, on the other hand correct this idea as soon as we can. i don't hope to let this become another pity issue for D7 ;p

@chx: once and once again: are you core committer? please don't close or set others issue as won't fix as you like, it is not your duty :(

@chx: please don't be too childish: the need of this patch is listed in my previous research remark (http://edin.no-ip.com/html/?q=node/310, http://phplens.com/lens/adodb/docs-adodb.htm#substr), and proved as function within my latest D7 database abstraction layer research progress result (http://drupal.org/node/172541#comment-305267). please feel free to comment about the correctness of my research result; BTW, striking such little footnote won't help about you overall propose (http://www.drupal4hu.com/node/64)

@Pasqualle: i guess i am not proposing something new other than existing ADOdb research result. as they have researched for more than 7 years, with totally 19 different databases supporting. their research result should be very solid.

on the other hand, before i notice about the handling of ADOdb, i try to implement a pre-defined random function for oracle driver once before (integrated with progress of http://drupal.org/node/39260): it is totally functioning:

<?php
    db_query('CREATE OR REPLACE FUNCTION rand RETURN FLOAT AS
        retval FLOAT;
      BEGIN
        SELECT dbms_random.value INTO retval FROM DUAL;
        RETURN retval;
      END;');
?>

BTW, isn't this the BEST handling? seems not to be, when you compare it with ADOdb implementation for oracle:

<?php
  var $random = "abs(mod(DBMS_RANDOM.RANDOM,10000001)/10000000)";
?>

and let's have a look about pgsql implementation, too:

<?php
  var $random = 'random()';		/// random function
?>

according to ADOdb's suggestion (http://phplens.com/lens/adodb/tips_portable_sql.htm): the numbers of additional abstraction function are limited. according to my studying on their different backend handling, i found that most of them are very reasonable and meaningful, and easy to implement :)

finally, we are not always using those special and specific SQL level functions: there is only around 5~7 lines existing within our latest drupal-6.x.dev CVS HEAD. as we are talking about a huge project as drupal core, you may simply guess about how many lines should be changed within contribution modules :)

@Pasqualle: thanks for your help about exploration on phpBB, and your positive supporting :)

According to ADOdb's document (http://phplens.com/lens/adodb/tips_portable_sql.htm), I try to summarize those database specific special SQL functions which required for abstraction:

1. DB_CONCAT_OPERATOR (defined constant)
2. DB_LENGTH (defined constant)
3. DB_UPPER (defined constant)
4. DB_RANDOM (defined constant)
5. DB_SUBSTR (defined constant)
6. db_concat() (function wrapper)
7. db_if_null() (function wrapper)

Some of them are really unusual, e.g. DB_UPPER and db_if_null(). BTW, as we are now fully implement those abstract functions which are required to be abstract, based on cross database compatibility concern (but not only based on today's needs, which means: trial and error), our developers may fully utilize most powerful database level functions as possibile. This also means that: increase our database abstraction flexibility and encourage our developers creativity.

Progress will integrate with my D7 database abstraction layer technical preview (http://drupal.org/node/172541), which will not duplicate in this issue once again.

P.S. need not to mention, time related handling is also a huge topic for database abstraction: ADOdb provide totally 6 different function wrappers for abstracting those differences (http://phplens.com/lens/adodb/tips_portable_sql.htm). BTW, we shouldn't require for these abstraction, as Drupal always use PHP-level date and time handling, which is very powerful and standardized, and database independence :)

Just as example, the following is code snippet from D6 OCI8 implementation, which demo how abstraction is needed for portable code:

<?php
/**
 * Indicates the string to use to quote identifiers and names.
 */
define('DB_QUOTE_OPERATOR', '"');

/**
 * Indicates the concatenation operator.
 */
define('DB_CONCAT_OPERATOR', '||');

/**
 * Indicates the default string placeholder.
 */
define('DB_DEFAULT', '_OCI8_DEFAULT_');

/**
 * Indicates the name of the SQL strtoupper function.
 */
define('DB_UPPER', 'UPPER');

/**
 * Indicates the SQL to generate a random number between 0.00 and 1.00.
 */
define('DB_RAND', 'ABS(MOD(DBMS_RANDOM.RANDOM,10000001)/10000000)');

/**
 * Indicates the name of the SQL strlen function.
 */
define('DB_STRLEN', 'LENGTH');

/**
 * Indicates the name of the SQL substr function.
 */
define('DB_SUBSTR', 'SUBSTR');

/**
 * Replace all escape characters in a query, based on the database specific
 * implementation.
 *
 * queries sent to Drupal should wrap all identifiers and names in square
 * brackets. This function will search for this syntax and replace it as
 * corresponding escape characters, based on the database specific
 * requirement.
 */
function db_escape_quote($sql) {
  return preg_replace_callback('/\[([A-Za-z0-9_]+)\]/', '_db_escape_quote_callback', $sql);
}

/**
 * Helper function for db_query().
 */
function _db_escape_quote_callback($matches) {
  $match = $matches[1];
  if (strlen($match) > 30) {
    // Try to fetch mapping, if not exists, set it up.
    $trim = variable_get('oci8_' . $match, NULL);
    if (!$trim) {
      // Fetch last counter, and +1 for next mapping.
      $count = variable_get('oci8_max30', 0) + 1;
      variable_set('oci8_max30', $count);
      // Setup mapping and reverse mapping.
      $trim = substr($match[1], 0, 30 - strlen($count)) . $count;
      variable_set('oci8_' . $match[1], $trim);
      variable_set('oci8_' . $trim, $match[1]);
      $match = $trim;
    }
  }
  return DB_QUOTE_OPERATOR . $match . DB_QUOTE_OPERATOR;
}

/**
 * Return a portably concatenate strings.
 *
 * @param ...
 *   Variable number of string parameters.
 * @return
 *   Portably concatenate strings.
 */
function db_concat() {
  $args = func_get_args();
  return implode(DB_CONCAT_OPERATOR, $args);
}

/**
 * Returns a string that is the equivalent of MySQL IFNULL or Oracle NVL.
 *
 * @return
 *   If $expr1 is not NULL, returns $expr1; otherwise it returns $expr2.
 */
function db_if_null($expr1, $expr2) {
  return " NVL($expr1, $expr2) ";
}
?>

This is just a very simple abstraction: replace most LENGTH and SUBSTRING, into DB_STRLEN and DB_SUBSTR. On the other hand, I also collect some other important abstraction, based on above research founding, and progress of my personal research project Siren.

Patch via latest CVS HEAD, and test with both MySQL and PostgreSQL.

Thanks for TNG DB layer with OOP style, we can now clone ADOdb handling in more ideal implementation. As mentioned in http://drupal.org/node/167335#comment-636082, these abstraction is very useful for other DB driver development.

This patch handling the following abstraction: quote identifier (e.g. "), concatenate operator (e.g. +), uppercase function, random function, string length function, substring operator, string concatenate function and if NULL benching. Each DB driver will override their required change, where a friendly wrapper function is provided as like as escapeTable => db_escape_table(). 3 set of examples are available (comment.module, locale.module and statistics.admin.inc), too.

Patch via CVS HEAD, tested with MySQL and PostgreSQL. MySQL work fine with either case, where PostgreSQL will buggy for: 1. add comment (but thread view work fine); 2. access top visitors page.

P.S. This patch is mainly target for the missing abstraction which is now missing from TNG DB layer, and try to minimize change to examples if possible. TNG DB syntax change is not its main concern, where we may handle afterward.

@Crell: Do we have any schedule for fixing this issue within our new query builders? Because we may use those abstraction all around ANY type of queries and ANYWHERE within SQL, I would like to have your suggestion about how to work it out...

The idea of abstract with SELECT builder is good, but I would like to give some love for RAW SQL syntax, too. Most likely we don't need to use SELECT builder for every cases because it is coming with performance degrade (for sure...), some simple abstraction may still needed for direct call.

Just for brainstorming: maybe we can handle this issue in 3 different levels?

1. Implement database-specific abstraction within each driver connection class.
2. Simply wrap the abstractions as db_*() function for RAW SQL directly call.
3. Handy wrap the abstractions within SELECT builder.

Any idea?

Implement within db_select() can simplify some abstraction, but not always truth. E.g. How to implement the RAND(), UPPER, etc? Even the ifNll() is not in the same format as above. Don't forget that we are still missing date related abstraction within DBTNG, when compare with that of ADOdb. There are too many variation and so we may need to implement it as isolated for more flexibility.

On the other hand, I am agree with catch's previous comment for other issue that "around 95% of SELECT queries will still use db_query()". It is not simple for revamp everything with query builder (and not really needed for), RAW SQL will always work around with us. We hate it, but we can't ignore the needs of it :S

Patch enhance with:

1. Update forum.module, replace the use of IF as db_if_null(). Pass MySQL simpletest.
2. Update system.install, remove all legacy PostgreSQL function abstractions (with drupal upgrade function).
3. Document the example use and syntax of new db_*() function with phpdoc.

Besides this patch, we only need to handle legacy PostgreSQL unsigned integer with another issue, and so legacy PostgreSQL abstraction will be cleaned up :D

@chx: agree that patch from #23 is not perfect; BTW, at least they are now working, and able to help the code cleanup of legacy PostgreSQL implementation, therefore simplify cross database maintenance effort.

Maybe we can first let this issue go into core for RAW SQL direct call, and further abstract them within query builders for more elegant look-and-feel with another issue?

May I have some hints about the suggested API look-and-feel? I am still studying our new query builders and so don't have clear idea about how it should look like :-S

On the other hand, I just discover that we give a revamp of DBTNG syntax for includes/session.inc (line 191) as:

function drupal_session_count($timestamp = 0, $anonymous = TRUE) {
  $query = db_select('sessions');
  $query->addExpression('COUNT(sid)', 'count');
  $query->condition('timestamp', $timestamp, '>=');
  $query->condition('uid', 0, $anonymous ? '=' : '>');
  return $query->execute()->fetchField();
}

So COUNT is used as SQL string manipulation... If patch from #23 get committed, we will have code as:

...
  $query->addExpression(db_rand(), 'randon_num'); // No args is needed. Well... looks fine.
  $query->addExpression(db_strcat('field1', 'field2'), 'field1n2'); // With args but still looks fine?
  $query->addExpression(db_strlen() . 'sid', 'length'); // Oooops! This looks really ugly!
...

Maybe we can also revamp db_strtoupper(), db_strlen() and db_substr() as function call, so with similar coding style? E.g.

...
  $query->addExpression(db_strtoupper('field1'), 'field1_upper');
  $query->addExpression(db_strlen('field2'), 'field2_length');
  $query->addExpression(db_substr('field3', 1, 3) . 'field3_sub');
...

Any idea?

chx: Sorry for the COUNT([sid]) syntax. It is a mistyping, as I copy it directly from my own CVS (including patch from #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements). Please forget it, and I have remove that [] from the above example :S

I give a try for the revamp. Most abstraction is now handle with function call but not string manipulation. Detail:

• comment.module: revamp with db_substr() and db_strlen().
• forum.module: revamp with db_if_null().
• locale.module: revamp with db_strlen().
• statistics.admin.inc: revamp with db_strcat().

One exception. comment.module coming with following SQL:

SUBSTRING(thread, 1, (LENGTH(thread) - 1))

In order to rewrite with db_strlen() and db_substr() we should use the following syntax:

db_substr('thread', 1, db_strlen('thread') . ' - 1')

It is difficult for remove the . ' - 1' string manipulation, because we are now having string replacement for SQL syntax, but not real integer operation. In case of DBTNG syntax, it should looks like:

...
  $query->addExpression(db_strtoupper('field1'), 'field1_upper');
  $query->addExpression(db_strlen('field2'), 'field2_length');
  $query->addExpression(db_substr('field3', 1, db_strlen('field3') . ' - 1'), 'field3_sub');
...

Ooops... Patch reroll via CVS HEAD. Update sequence number for update in system.install.

Patch revamp based on above suggestion, including:

• Most are now implement as method, so can call directly.
• Function wrapper update with $options and target.
• db_strcat() clone db_query() handling, for input $args in array format.

Patch tested with MySQL simpletest and looks good. For the docblocks I would like to have some help from other... My English is too poor...

P.S. I keep class internal method names as sync with ADOdb version (http://phplens.com/lens/adodb/tips_portable_sql.htm#native) in order to simplify debug and code trace.

It is not easy to judge if we should implement in the database engine, because we need to abstract a simple native MySQL SQL function, with numbers of PostgreSQL custom SQL functions.

For example, MySQL's IF() support the following syntax (http://dev.mysql.com/doc/refman/5.0/en/if-statement.html):

IF search_condition THEN statement_list
    [ELSEIF search_condition THEN statement_list] ...
    [ELSE statement_list]
END IF

It is too complicated if we hope to implement such logic in perfect, with PostgreSQL custom SQL functions. That's why we just implement 2 interfaces of IF() within Drupal's PostgreSQL driver.

BTW, a simple abstraction in PHP as string manipulation is much flexible and complete.

Some demonstration for combine use with DBTNG syntax (code snippet from comment.module):

-    $result_count = db_query("SELECT COUNT(*) FROM {comments} WHERE nid = %d AND status = 0 AND SUBSTRING(thread, 1, (LENGTH(thread) - 1)) < '" . $thread . "'", $node->nid);
-    $count = db_result($result_count);
+    $query = db_select('comments');
+    $query->condition('nid', $node->nid);
+    $query->condition('status', 0);
+    $query->condition(db_substr('thread', 1, db_strlen('thread') . ' - 1'), $thread, '<');
+    $count = $query->countQuery()->execute()->fetchField();

SUBSTRING and LENGTH (and so other additional abstractions) are able to use within SELECT ... FROM or WHERE ..., so as flexible as AND and OR. Therefore the implementation of db_substr() and db_strlen() should be similar as db_and() and db_or(): able called within condition() or addExpression() (i.e. we shouldn't implement as $query->strlen() syntax).

Patch reroll, with some SQL revamp with DBTNG syntax. For those I don't know how to handle, I give a remark with @todo: Revamp with DBTNG syntax. Test with MySQL + simpletest, all related tests pass.

Reroll based on comment.module update.

Crell and chx: should we have some more discussion about this issue? I am really looking for this feature build into Drupal core ;-)

Reroll via CVS HEAD.

I still think that either the database engine should solve this like postgresql does in HEAD

This is not a good approach because we always implement a subset of original SQL functions only. E.g. substring in case of MySQL can be written as: SUBSTRING(str,pos), SUBSTRING(str FROM pos), SUBSTRING(str,pos,len), SUBSTRING(str FROM pos FOR len). Abstract syntax variation with manual SQL function will result as a trim of functionality, or a complicated and duplicated handling.

BTW, the different of syntax (substr vs. substring) is the only variation that we must duel with. Therefore abstract with a simple function call should be the most direct approach. It is not about "benefit" to all/any developers, but how to solve the problem with the most simplest and elegant format. I am not encourage/discourage the use of these SQL functions or MySQL/Oracle, but trying to provide options for developers when they do required for.

you should write operator-like support for the SELECT builder

In case of MySQL, the following 3 queries are both in correct syntax:

SELECT SUBSTR(name, 1, 4) FROM system;
SELECT name FROM system WHERE SUBSTR(name, 1, 4) = 'blog';
SELECT name FROM system ORDER BY SUBSTR(name, 1, 4);

If we implement with SELECT builder, we will need to handle similar abstraction for both addExpression(), condition() and orderBy(). For sure that is duplicated. SUBSTR(), LENGTH(), RAND(), ifnull(), etc are generic operator/operations which much similar as AND and OR, but not similar as COUNT() (i.e. addExpression()) or LIKE (i.e. condition()).

mo sense is to have alternate methods on the SELECT builder that parallel addField(). Vis, addSubstring($table, $field, $alias, $start, $length), or something like that.

This is not simple, too. E.g. comment module is now using SUBSTR() and LENGTH() together, so implement with addSubstring() will not able to cascade with LENGTH(). We need something more flexible.