Comment #7

• Log in or register to post comments

What about #3?

Since CONCAT() doesn't work in PostgreSQL, we'd need to create our own (like here )

• Log in or register to post comments

Comment #9

Damien Tournoud Credit Attribution: Damien Tournoud commented 27 May 2010 at 19:56

@bojanz: CONCAT() has been the standard way of concatenating strings for years in any version of Drupal < 7. We already have a compatibility CONCAT() function in PostgreSQL and SQLite.

• Log in or register to post comments

OK, that makes sense. Thanks, DamZ. +1 on #5 then.

Do we know that any other databases we may want to support can be made to handle CONCAT() the way we do with Postgres?

• Log in or register to post comments

Two instances of "ANSI" left in the function descriptions.
Since test bot will test RTBC patches, leaving there.

• Log in or register to post comments

Just installed HEAD on PostgreSQL 8.3 and test failed:

PDOException: SQLSTATE[42883]: Undefined function: 7 ERROR: function concat(bigint, unknown) does not exist LINE 1: SELECT CONCAT($1, CONCAT(name, CONCAT($2, CONCAT(age, $3))))... ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts.: SELECT CONCAT(:a1, CONCAT(name, CONCAT(:a2, CONCAT(age, :a3)))) FROM {test} WHERE age = :age; Array ( [:a1] => The age of [:a2] => is [:a3] => . [:age] => 25 )  in DrupalTestCase->run() (line 442 of /home/josh/projects/drupal7/modules/simpletest/drupal_web_test_case.php).
Here is our existing code:

if (!db_query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'concat'")->fetchField()) {
   db_query('CREATE OR REPLACE FUNCTION "concat"(text, text) RETURNS text AS
      \'SELECT $1 || $2;\'
      LANGUAGE \'sql\''
The problem is for every argument data type you can pass to CONCAT you need to specify another function. Ugly.

• Log in or register to post comments

Comment #21

Damien Tournoud Credit Attribution: Damien Tournoud commented 14 June 2010 at 15:20

This bug was known, the new test only exposed it.

This should do it:

CREATE OR REPLACE FUNCTION "concat"(anynonarray, anynonarray) RETURNS text AS
      'SELECT CAST($1 AS text) || CAST($2 AS text);'
      LANGUAGE 'sql'
CREATE OR REPLACE FUNCTION "concat"(text, anynonarray) RETURNS text AS
      'SELECT $1 || CAST($2 AS text);'
      LANGUAGE 'sql'
CREATE OR REPLACE FUNCTION "concat"(anynonarray, text) RETURNS text AS
      'SELECT CAST($1 AS text) || $1;'
      LANGUAGE 'sql'
(yes, we need three functions, because all anynonarray must be of the same type when a polymorphic functions is called in PostgreSQL)

• Log in or register to post comments

Comment #22

Dave Reid
he/him
English
Nebraska USA
Credit Attribution: Dave Reid commented 14 June 2010 at 16:03

I have given up on using CONCAT() in SQL. I honestly think we need some kind of $query->concat() function that will return the proper concatenation SQL for each database driver. See #319070: Provide a wrapper for cross-db and cross-field-type concatenation (PosgreSQL) .

• Log in or register to post comments

Last I checked, Crell was against smaller functions like that, though personally, I'm for it.

• Log in or register to post comments

How many of the situations where CONCAT() is used can't be designed around? Looking at the bug issues involving it, I get the feeling that most can.

Most of the CAST() issues are related to the use of CONACT() too, so trying to move away from its use would reduce or eliminate two of the big areas of database compatibility problems.

I think that any documentation on this issue should suggest in strong terms you need to think hard, at least twice, before deciding that CONCAT() and therefore probably CAST() are part of the solution to your problem.

• Log in or register to post comments

Comment #25

Damien Tournoud Credit Attribution: Damien Tournoud commented 15 June 2010 at 12:38

I think we can design a type-insensitive CONCAT() in all the databases we support.

• Log in or register to post comments

Document that SQL strings cant use concat, add a concat to mapConditionOperator and live happily ever after.

• Log in or register to post comments

I don't think mapConditionOperator can handle concat in its current form. That's not really what it's there for.

• Log in or register to post comments

Comment #28

Damien Tournoud Credit Attribution: Damien Tournoud commented 15 June 2010 at 21:02

What we need is being able to replace a || b by something as complex as CAST(a AS text) + CAST(b AS text) . Could we use mapConditionOperator() for that?

• Log in or register to post comments

regex for drivers who don't support ||?

if (strpos($sql, '||')) {
    $sql = preg_replace('/([^,\s]+)?\s\|\|?\s([^,\s]+)/', 'CONCAT($1, $2)', $sql);
  
Log in or register to post comments
    
regex for drivers who don't support ||?
if (strpos($sql, '||')) {
    $sql = preg_replace('/([^,\s]+)?\s\|\|?\s([^,\s]+)/', 'CONCAT($1, $2)', $sql);
  
Log in or register to post comments
    
string parsing is not a solution. Damien is right -- we might need a separate concat method :(
      
  
Log in or register to post comments
    
Comment #32
    Damien Tournoud CreditAttribution: Damien Tournoud commented 15 June 2010 at 21:28  




    

    
Josh, do you really believe that you can parse a random SQL expression with a regexp? Remember that there might be *anything* on both sides of the operator, and that you have to take into account precedence rules.
      
  
Log in or register to post comments
    
No, I don't think REGEX are a good solution for SQL. But I knew it would stir people up! ;)
DamZ solution seems reasonable to me. 3 CONCAT functions. If it makes 'Basic SQL syntax tests" pass without an exception then I'm happy.
      

• Log in or register to post comments

So to summarize then:

1) Document that || is verbotten.

2) We need a new patch that makes all 3 variants of Concat() for Postgres. Who's doing that one?

3) Do we need anything for SQLite?

• Log in or register to post comments

MySQL CONCAT takes any number of parameters -- but then I guess we can document that plz do not use such CONCAT (good luck with people keeping that one...)

• Log in or register to post comments

Comment #37

Damien Tournoud Credit Attribution: Damien Tournoud commented 18 June 2010 at 09:25

@chx: I already documented (as a stub) that only CONCAT(string1, string2) is supported in the list of functions and operators wie support .

• Log in or register to post comments

The concat(text, text) function is already created several lines before the place where this patch is applied, so we are creating/replacing the function twice for the (text, text) arguments.

Also, in HEAD the patch applied with an offset of 32 lines.

• Log in or register to post comments
// PostgreSQL requires the function to be defined *foreach*
// different argument variation the function can handle.

I guess this foreach needs to read "for each" too.

• Log in or register to post comments

Tested #40 on Postgresql 8.3, works as intended (which is weird because generally on PostgreSQL nothing works the way you would expect them to).

• Log in or register to post comments

Comment #42

Damien Tournoud Credit Attribution: Damien Tournoud commented 18 June 2010 at 23:48

Note: SQLite is not affected, so with this patch the "Basic SQL" test passes on every database we support.

• Log in or register to post comments

Agreed, they're not critical. Where is that major priority anyway? :(

Committed to HEAD all the same, though.

• Log in or register to post comments