Hello Jerome, On 12/29/2016 08:15 PM, jerome brauge wrote:

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes. Please see inline.

Best regards, Jérôme.

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing. Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) +{ + return (((1ULL << (n - 1)) - 1) << 1) | 1; +} + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage. Something like this would be nice: CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value +# + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND','MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSECOND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE','IGNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND','MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSECOND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE','IGNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style: 1. In assignment operator: - there is no space before "=" - there is one space after "=". So the above line "bool is_const= 0;" should not be modified. 2. Don't use tabs, use spaces instead. 3. There should not be trailing white spaces. Please use "git diff --check". Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy. Please make sure that all new and modified line follows the coding style.

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i". Also, I noticed that args[0]->val_str() is called twice. One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again. I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors: class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... }; and then set it according to sql_mode in fix_length_and_dec(): void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... } After this, we change all tests like: if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF) to if (m_null_yields_not_null)

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable: ... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1

Hello Jerome, On 01/16/2017 02:45 PM, jerome brauge wrote:

Hello Alexander, According to your comments, this is the new version.

Thanks for addressing my suggestions. We're discussing the change with our architect Sergei Golubchik. Briefly, the idea is: Perhaps we should not change the function CONCAT(). This is a non-standard MySQL/MariaDB function, and it does not exist in Oracle. So probably it should stay sql_mode independent, while sql_mode should affect only the concatenation operator ||. Would this solution work for you? We're still discussing details. I'll get back to you soon. Greetings.

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 30 décembre 2016 10:36 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 12/29/2016 08:15 PM, jerome brauge wrote:

...

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes.

Please see inline.

...

Best regards, Jérôme.

...

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing.

Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

...

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) { + return (((1ULL << (n - 1)) - 1) << 1) | 1; } + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage.

Something like this would be nice:

CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

...

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value +# + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT +'' NOT NULL, name char(64) DEFAULT '' NOT NULL, type +enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT +'' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, +sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', +'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, +is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, +security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, +param_list blob NOT NULL, returns longblob NOT NULL, body longblob +NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, +created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 +00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', +'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', +'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', +'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', +'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', +'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', +'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', +'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', +'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', +'NO_ENGINE_SUBSTITUTION', +'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' +NOT NULL, comment text collate utf8_bin NOT NULL, +character_set_client char(32) collate utf8_bin, collation_connection +char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, +body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM +character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND','M ICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_MI NUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSECON D','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE','IG NORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION' ,'NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO_KE Y_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40', 'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TA BLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DA TES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGT H') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 +COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET +utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) +CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at +DATETIME default NULL, interval_value int(11) default NULL, +interval_field +ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' +MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_ +MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSE +COND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, +created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 +00:00:00', last_executed DATETIME default NULL, starts DATETIME +default NULL, ends DATETIME default NULL, status +ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default +'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default +'DROP', sql_mode +set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE','I +GNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTIO +N','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO +_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL +40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRA +NS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVA +LID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE +_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FU +LL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, +comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default +'', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) +CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client +char(32) collate utf8_bin, collation_connection char(32) collate +utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, +PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT +'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LE + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style:

1. In assignment operator: - there is no space before "=" - there is one space after "=".

So the above line "bool is_const= 0;" should not be modified.

2. Don't use tabs, use spaces instead.

3. There should not be trailing white spaces. Please use "git diff --check".

Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy.

Please make sure that all new and modified line follows the coding style.

...

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i".

Also, I noticed that args[0]->val_str() is called twice.

One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again.

I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

...

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors:

class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... };

and then set it according to sql_mode in fix_length_and_dec():

void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... }

After this, we change all tests like:

if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

to

if (m_null_yields_not_null)

...

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable:

... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

...

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LE + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1

Hi Jerome, On 01/19/2017 05:15 PM, jerome brauge wrote:

Hello Alexander, Sergei, Concat function exists in Oracle, but it accepts only two operand. Due to this, it's not very useful and we don't use it.

Thanks for clarification!

Changing only operator || is fine for us.

I created an MDEV for this: https://jira.mariadb.org/browse/MDEV-11880 and modified your patch to implement this approach. To safe on performance slightly, I created two different classes for the MariaDB style concatenation and for Oracle style || concatenation. Please find the new version attached. Do you think it's fine? Note, before we can push it, we'd like to fix this problem first: https://jira.mariadb.org/browse/MDEV-11848 Greetings.

Regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 19 janvier 2017 13:18 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 01/16/2017 02:45 PM, jerome brauge wrote:

...

Hello Alexander, According to your comments, this is the new version.

Thanks for addressing my suggestions.

We're discussing the change with our architect Sergei Golubchik.

Briefly, the idea is:

Perhaps we should not change the function CONCAT(). This is a non-standard MySQL/MariaDB function, and it does not exist in Oracle. So probably it should stay sql_mode independent, while sql_mode should affect only the concatenation operator ||.

Would this solution work for you?

We're still discussing details. I'll get back to you soon.

Greetings.

...

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 30 décembre 2016 10:36 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 12/29/2016 08:15 PM, jerome brauge wrote:

...

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes.

Please see inline.

...

Best regards, Jérôme.

...

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing.

Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

...

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) { + return (((1ULL << (n - 1)) - 1) << 1) | 1; } + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage.

Something like this would be nice:

CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

...

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value +# + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin +DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type +enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) +DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, +sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', +'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, +is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, +security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, +param_list blob NOT NULL, returns longblob NOT NULL, body longblob +NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, +created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 +00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', +'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', +'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', +'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', +'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', +'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', +'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', +'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', +'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', +'NO_ENGINE_SUBSTITUTION', +'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' +NOT NULL, comment text collate utf8_bin NOT NULL, +character_set_client char(32) collate utf8_bin, collation_connection +char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, +body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM +character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' M ICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_M I NUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSECO N D','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE','I G NORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION' ,'NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO_K E Y_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40' , 'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_T A BLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_D A TES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER' , 'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENG T H') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 +COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET +utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) +CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at +DATETIME default NULL, interval_value int(11) default NULL, +interval_field +ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' +MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR +_ +MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROS +E +COND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, +created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 +00:00:00', last_executed DATETIME default NULL, starts DATETIME +default NULL, ends DATETIME default NULL, status +ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default +'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default +'DROP', sql_mode +set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' +I +GNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTI +O +N','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','N +O +_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQ +L +40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TR +A +NS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INV +A +LID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREAT +E +_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_F +U +LL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, +comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL +default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) +CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client +char(32) collate utf8_bin, collation_connection char(32) collate +utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 +longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT +CHARSET=utf8 COMMENT 'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style:

1. In assignment operator: - there is no space before "=" - there is one space after "=".

So the above line "bool is_const= 0;" should not be modified.

2. Don't use tabs, use spaces instead.

3. There should not be trailing white spaces. Please use "git diff --check".

Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy.

Please make sure that all new and modified line follows the coding style.

...

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i".

Also, I noticed that args[0]->val_str() is called twice.

One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again.

I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

...

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors:

class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... };

and then set it according to sql_mode in fix_length_and_dec():

void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... }

After this, we change all tests like:

if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

to

if (m_null_yields_not_null)

...

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable:

... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

...

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1

Hi Jerome, On 01/23/2017 06:39 PM, jerome brauge wrote:

Hi Alexander, I think something is wrong in Item_func_concat_operator_oracle, if the first argument is empty and the second argument is null ,then res is null and res->set_charset fail. Example: select ''||null||'a'

Right, thanks for noticing this.

for (i++ ; i < arg_count ; i++) { if (res->length() == 0) { // See comments in Item_func_concat::val_str() -->> if (!(res= arg_val_str(i, str, &is_const)))

"res" should only be assigned if arg_val_str() returns a non-null value. Fixed the problem and extended the tests to cover this combination, and many other combinations. The new version is attached. Thanks for cooperation!

continue; } else { const String *res2; if (!(res2= args[i]->val_str(use_as_buff))) continue; if (!(res= append_value(res, is_const, str, &use_as_buff, res2))) goto null; is_const= 0; } } -->> res->set_charset(collation.collation);

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 janvier 2017 12:28 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hi Jerome,

On 01/19/2017 05:15 PM, jerome brauge wrote:

...

Hello Alexander, Sergei, Concat function exists in Oracle, but it accepts only two operand. Due to this, it's not very useful and we don't use it.

Thanks for clarification!

...

Changing only operator || is fine for us.

I created an MDEV for this: https://jira.mariadb.org/browse/MDEV-11880

and modified your patch to implement this approach.

To safe on performance slightly, I created two different classes for the MariaDB style concatenation and for Oracle style || concatenation.

Please find the new version attached. Do you think it's fine?

Note, before we can push it, we'd like to fix this problem first:

https://jira.mariadb.org/browse/MDEV-11848

Greetings.

...

Regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 19 janvier 2017 13:18 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 01/16/2017 02:45 PM, jerome brauge wrote:

...

Hello Alexander, According to your comments, this is the new version.

Thanks for addressing my suggestions.

We're discussing the change with our architect Sergei Golubchik.

Briefly, the idea is:

Perhaps we should not change the function CONCAT(). This is a non-standard MySQL/MariaDB function, and it does not exist in Oracle. So probably it should stay sql_mode independent, while sql_mode should affect only the concatenation operator ||.

Would this solution work for you?

We're still discussing details. I'll get back to you soon.

Greetings.

...

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 30 décembre 2016 10:36 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 12/29/2016 08:15 PM, jerome brauge wrote:

...

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes.

Please see inline.

...

Best regards, Jérôme.

...

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing.

Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

...

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) { + return (((1ULL << (n - 1)) - 1) << 1) | 1; } + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage.

Something like this would be nice:

CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

...

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value +# + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin +DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type +enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) +DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, +sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', +'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, +is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, +security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, +param_list blob NOT NULL, returns longblob NOT NULL, body longblob +NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, +created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 +00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', +'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', +'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', +'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', +'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', +'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', +'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', +'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', +'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', +'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', +'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' +NOT NULL, comment text collate utf8_bin NOT NULL, +character_set_client char(32) collate utf8_bin, +collation_connection +char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, +body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM +character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' M ICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR_ M I NUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSEC O N D','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' I G NORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION' ,'NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO_ K E Y_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40' , 'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_ T A BLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_ D A TES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER' , 'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LEN G T H') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 +COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET +utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) +CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at +DATETIME default NULL, interval_value int(11) default NULL, +interval_field +ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' +MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOU +R +_ +MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICRO +S +E +COND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, +created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 +00:00:00', last_executed DATETIME default NULL, starts DATETIME +default NULL, ends DATETIME default NULL, status +ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default +'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default +'DROP', sql_mode +set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' +I +GNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACT +I +O +N','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB',' +N +O +_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYS +Q +L +40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_T +R +A +NS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','IN +V +A +LID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREA +T +E +_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_ +F +U +LL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, +comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL +default '', originator INTEGER UNSIGNED NOT NULL, time_zone +char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', +character_set_client +char(32) collate utf8_bin, collation_connection char(32) collate +utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 +longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT +CHARSET=utf8 COMMENT 'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style:

1. In assignment operator: - there is no space before "=" - there is one space after "=".

So the above line "bool is_const= 0;" should not be modified.

2. Don't use tabs, use spaces instead.

3. There should not be trailing white spaces. Please use "git diff --check".

Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy.

Please make sure that all new and modified line follows the coding style.

...

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i".

Also, I noticed that args[0]->val_str() is called twice.

One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again.

I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

...

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors:

class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... };

and then set it according to sql_mode in fix_length_and_dec():

void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... }

After this, we change all tests like:

if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

to

if (m_null_yields_not_null)

...

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable:

... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

...

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1

Hello Jerome, On 01/24/2017 07:33 PM, jerome brauge wrote:

Hello Alexander, This patch works fine. Thanks.

Thanks for confirmation. The patch is now in review. I'll push it as soon as it gets reviewed, then will let you know.

But I have a question: why do you want to preserve behavior of concat ? If it's for compatibility reason with older version, how do you think manage the required changes in some functions? For example, select substr('ab',0,1) return an empty string on Mariadb but 'a' on Oracle.

Perhaps, in this case an new sql_mode can preserve older semantic.

We haven't thought about it yet.

Best regards. Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mardi 24 janvier 2017 08:05 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hi Jerome,

On 01/23/2017 06:39 PM, jerome brauge wrote:

...

Hi Alexander, I think something is wrong in Item_func_concat_operator_oracle, if the first argument is empty and the second argument is null ,then res is null and res->set_charset fail. Example: select ''||null||'a'

Right, thanks for noticing this.

...

for (i++ ; i < arg_count ; i++) { if (res->length() == 0) { // See comments in Item_func_concat::val_str() -->> if (!(res= arg_val_str(i, str, &is_const)))

"res" should only be assigned if arg_val_str() returns a non-null value. Fixed the problem and extended the tests to cover this combination, and many other combinations.

The new version is attached.

Thanks for cooperation!

...

continue; } else { const String *res2; if (!(res2= args[i]->val_str(use_as_buff))) continue; if (!(res= append_value(res, is_const, str, &use_as_buff, res2))) goto null; is_const= 0; } } -->> res->set_charset(collation.collation);

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 janvier 2017 12:28 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hi Jerome,

On 01/19/2017 05:15 PM, jerome brauge wrote:

...

Hello Alexander, Sergei, Concat function exists in Oracle, but it accepts only two operand. Due to this, it's not very useful and we don't use it.

Thanks for clarification!

...

Changing only operator || is fine for us.

I created an MDEV for this: https://jira.mariadb.org/browse/MDEV-11880

and modified your patch to implement this approach.

To safe on performance slightly, I created two different classes for the MariaDB style concatenation and for Oracle style || concatenation.

Please find the new version attached. Do you think it's fine?

Note, before we can push it, we'd like to fix this problem first:

https://jira.mariadb.org/browse/MDEV-11848

Greetings.

...

Regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 19 janvier 2017 13:18 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 01/16/2017 02:45 PM, jerome brauge wrote:

...

Hello Alexander, According to your comments, this is the new version.

Thanks for addressing my suggestions.

We're discussing the change with our architect Sergei Golubchik.

Briefly, the idea is:

Perhaps we should not change the function CONCAT(). This is a non-standard MySQL/MariaDB function, and it does not exist in Oracle. So probably it should stay sql_mode independent, while sql_mode should affect only the concatenation operator ||.

Would this solution work for you?

We're still discussing details. I'll get back to you soon.

Greetings.

...

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 30 décembre 2016 10:36 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 12/29/2016 08:15 PM, jerome brauge wrote:

...

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes.

Please see inline.

...

Best regards, Jérôme.

...

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing.

Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

...

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) { + return (((1ULL << (n - 1)) - 1) << 1) | 1; } + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage.

Something like this would be nice:

CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

...

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value +# + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin +DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type +enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) +DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, +sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', +'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, +is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, +security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT +NULL, param_list blob NOT NULL, returns longblob NOT NULL, body +longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' +NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON +UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT +'0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', +'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', +'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', +'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', +'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', +'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', +'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', +'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', +'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', +'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', +'NO_ENGINE_SUBSTITUTION', +'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' +NOT NULL, comment text collate utf8_bin NOT NULL, +character_set_client char(32) collate utf8_bin, +collation_connection +char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, +body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM +character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' M ICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HOUR _ M I NUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICROSE C O N D','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' I G NORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION' ,'NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB','NO _ K E Y_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40' , 'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS _ T A BLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID _ D A TES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER' , 'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LE N G T H') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 +COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET +utf8 NOT NULL default '', body longblob NOT NULL, definer +char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', +execute_at DATETIME default NULL, interval_value int(11) default +NULL, interval_field +ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' +MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HO +U +R +_ +MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICR +O +S +E +COND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, +created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 +00:00:00', last_executed DATETIME default NULL, starts DATETIME +default NULL, ends DATETIME default NULL, status +ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default +'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default +'DROP', sql_mode +set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' +I +GNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRAC +T +I +O +N','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB',' +N +O +_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MY +S +Q +L +40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_ +T +R +A +NS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','I +N +V +A +LID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CRE +A +T +E +_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO +_ +F +U +LL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, +comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL +default '', originator INTEGER UNSIGNED NOT NULL, time_zone +char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', +character_set_client +char(32) collate utf8_bin, collation_connection char(32) collate +utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 +longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT +CHARSET=utf8 COMMENT 'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL + _ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style:

1. In assignment operator: - there is no space before "=" - there is one space after "=".

So the above line "bool is_const= 0;" should not be modified.

2. Don't use tabs, use spaces instead.

3. There should not be trailing white spaces. Please use "git diff --check".

Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy.

Please make sure that all new and modified line follows the coding style.

...

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i".

Also, I noticed that args[0]->val_str() is called twice.

One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again.

I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

...

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors:

class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... };

and then set it according to sql_mode in fix_length_and_dec():

void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... }

After this, we change all tests like:

if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

to

if (m_null_yields_not_null)

...

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable:

... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

...

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL + _ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1

Hello Alexander, Thanks . I will clone again. Can I work on MDEV-10697 ? (GOTO statement). It's a hard blocking point for us. I have others suggestions for Oracle compatibility: - adding support of "naming notation" when calling functions or procedures (https://docs.oracle.com/database/121/LNPLS/subprograms.htm#LNPLS00825) - adding support of default value if a parameters is omitted (only with IN parameter on Oracle but Sybase ASE and SQLServer can do this with OUT parameter too and it's very useful) https://docs.oracle.com/database/121/LNPLS/subprograms.htm#LNPLS666 - suppress some limitations of function (essentially executing dynamic sql and recursive call). Best regards, Jérôme. -----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mercredi 1 février 2017 13:40 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility Hello Jerome, The patch for the Oracle style || operator is now pushed to bb-10.2-compatibility. Note, we rebase bb-10.2-compatibility on top of the latest 10.2 periodically. So if "git pull" rejects to do fast forward, please clone again. Thank you very much for your contribution! On 01/26/2017 07:44 PM, Alexander Barkov wrote:

Hello Jerome,

On 01/24/2017 07:33 PM, jerome brauge wrote:

...

Hello Alexander, This patch works fine. Thanks.

Thanks for confirmation. The patch is now in review. I'll push it as soon as it gets reviewed, then will let you know.

...

But I have a question: why do you want to preserve behavior of concat ? If it's for compatibility reason with older version, how do you think manage the required changes in some functions? For example, select substr('ab',0,1) return an empty string on Mariadb but 'a' on Oracle.

Perhaps, in this case an new sql_mode can preserve older semantic.

We haven't thought about it yet.

...

Best regards. Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mardi 24 janvier 2017 08:05 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hi Jerome,

On 01/23/2017 06:39 PM, jerome brauge wrote:

...

Hi Alexander, I think something is wrong in Item_func_concat_operator_oracle, if the first argument is empty and the second argument is null ,then res is null and res->set_charset fail. Example: select ''||null||'a'

Right, thanks for noticing this.

...

for (i++ ; i < arg_count ; i++) { if (res->length() == 0) { // See comments in Item_func_concat::val_str() -->> if (!(res= arg_val_str(i, str, &is_const)))

"res" should only be assigned if arg_val_str() returns a non-null value. Fixed the problem and extended the tests to cover this combination, and many other combinations.

The new version is attached.

Thanks for cooperation!

...

continue; } else { const String *res2; if (!(res2= args[i]->val_str(use_as_buff))) continue; if (!(res= append_value(res, is_const, str, &use_as_buff, res2))) goto null; is_const= 0; } } -->> res->set_charset(collation.collation);

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 janvier 2017 12:28 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hi Jerome,

On 01/19/2017 05:15 PM, jerome brauge wrote:

...

Hello Alexander, Sergei, Concat function exists in Oracle, but it accepts only two operand. Due to this, it's not very useful and we don't use it.

Thanks for clarification!

...

Changing only operator || is fine for us.

I created an MDEV for this: https://jira.mariadb.org/browse/MDEV-11880

and modified your patch to implement this approach.

To safe on performance slightly, I created two different classes for the MariaDB style concatenation and for Oracle style || concatenation.

Please find the new version attached. Do you think it's fine?

Note, before we can push it, we'd like to fix this problem first:

https://jira.mariadb.org/browse/MDEV-11848

Greetings.

...

Regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 19 janvier 2017 13:18 À : jerome brauge Cc : MariaDB Developers; Sergei Golubchik Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 01/16/2017 02:45 PM, jerome brauge wrote:

...

Hello Alexander, According to your comments, this is the new version.

Thanks for addressing my suggestions.

We're discussing the change with our architect Sergei Golubchik.

Briefly, the idea is:

Perhaps we should not change the function CONCAT(). This is a non-standard MySQL/MariaDB function, and it does not exist in Oracle. So probably it should stay sql_mode independent, while sql_mode should affect only the concatenation operator ||.

Would this solution work for you?

We're still discussing details. I'll get back to you soon.

Greetings.

...

Best regards, Jérôme.

-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 30 décembre 2016 10:36 À : jerome brauge Cc : MariaDB Developers Objet : Re: MDEV-10142 - bb-10.2-compatibility

Hello Jerome,

On 12/29/2016 08:15 PM, jerome brauge wrote:

...

Hello Alexander, I built a patch for concat of strings and nulls when sql_mode=oracle. Can you review it ?

Thank you very much for your patch. It generally looks fine. I suggest only small changes.

Please see inline.

...

Best regards, Jérôme.

...

From 6250ce0bcfe563491b25d2e1b4c879f697623537 Mon Sep 17 00:00:00 2001 From: jb <jb@JB-PC> Date: Mon, 26 Dec 2016 15:03:41 +0100 Subject: [PATCH] Adding sql_mode=CONCAT_NULL_YIELDS_NULL_OFF which is added by

I'm not sure about _OFF at the end. It sounds confusing.

Perhaps CONCAT_NULL_YIELDS_NOT_NULL would be a better name.

...

default to sql_mode=ORACLE. Concatenating a zero-length character string with another operand always results in the other operand, so null can result only from the concatenation of two null strings.

--- include/my_bit.h | 5 +++ .../suite/compat/oracle/r/func_concat.result | 16 ++++++++ mysql-test/suite/compat/oracle/t/func_concat.test | 17 ++++++++ scripts/mysql_system_tables.sql | 4 +- scripts/mysql_system_tables_fix.sql | 3 +- sql/event_db_repository.cc | 2 +- sql/item_strfunc.cc | 47 +++++++++++++++++----- sql/sp.cc | 2 +- sql/sql_class.h | 1 + sql/sys_vars.cc | 5 ++- sql/sys_vars.ic | 8 ++-- 11 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 mysql-test/suite/compat/oracle/r/func_concat.result create mode 100644 mysql-test/suite/compat/oracle/t/func_concat.test

diff --git a/include/my_bit.h b/include/my_bit.h index a50403c..4918815 100644 --- a/include/my_bit.h +++ b/include/my_bit.h @@ -130,6 +130,11 @@ static inline uint32 my_set_bits(int n) return (((1UL << (n - 1)) - 1) << 1) | 1; }

+static inline uint64 my_set_bits64(int n) { + return (((1ULL << (n - 1)) - 1) << 1) | 1; } + C_MODE_END

#endif /* MY_BIT_INCLUDED */ diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result new file mode 100644 index 0000000..d055192 --- /dev/null +++ b/mysql-test/suite/compat/oracle/r/func_concat.result @@ -0,0 +1,16 @@ +SET sql_mode=ORACLE; +SELECT 'a'||'b'; +'a'||'b' +ab +SELECT 'a'||'b'||NULL; +'a'||'b'||NULL +ab +SELECT NULL||'b'||NULL; +NULL||'b'||NULL +b +SELECT NULL||NULL||'c'; +NULL||NULL||'c' +c +SELECT NULL||NULL; +NULL||NULL +NULL

Can you please add tests with table fields? The implementation of Item_func_concat:val_str() use args[x]->const_item(). So using non-const arguments would improve coverage.

Something like this would be nice:

CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10), c VARCHAR(10)); INSERT INTO t1 VALUES (NULL,NULL,NULL); INSERT INTO t1 VALUES ('a',NULL,NULL); INSERT INTO t1 VALUES (NULL,'b',NULL); INSERT INTO t1 VALUES (NULL,NULL,'c'); INSERT INTO t1 VALUES ('a','b',NULL); INSERT INTO t1 VALUES (NULL,'b','c'); INSERT INTO t1 VALUES ('a',NULL,'c'); SELECT a||b||c FROM t1; DROP TABLE t1;

...

diff --git a/mysql-test/suite/compat/oracle/t/func_concat.test b/mysql-test/suite/compat/oracle/t/func_concat.test new file mode 100644 index 0000000..8d84707 --- /dev/null +++ b/mysql-test/suite/compat/oracle/t/func_concat.test @@ -0,0 +1,17 @@ +# +# Testing CONCAT with null value # + +SET sql_mode=ORACLE; + +SELECT 'a'||'b'; + +SELECT 'a'||'b'||NULL; + +SELECT NULL||'b'||NULL; + +SELECT NULL||NULL||'c'; + +SELECT NULL||NULL; + + diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql index 7b61416..eb2c97f 100644 --- a/scripts/mysql_system_tables.sql +++ b/scripts/mysql_system_tables.sql @@ -80,7 +80,7 @@ CREATE TABLE IF NOT EXISTS time_zone_transition_type ( Time_zone_id int unsign

CREATE TABLE IF NOT EXISTS time_zone_leap_second ( Transition_time bigint signed NOT NULL, Correction int signed NOT NULL, PRIMARY KEY TranTime (Transition_time) ) engine=MyISAM CHARACTER SET utf8 comment='Leap seconds information for time zones';

-CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', 'READS_SQL_DATA', 'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT NULL, param_list blob NOT NULL, returns longblob NOT NULL, body longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT '0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', 'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', 'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', 'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', 'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', 'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', 'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', 'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', 'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', 'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', 'PAD_CHAR_TO_FULL_LENGTH') DEFAULT '' NOT NULL, comment text collate utf8_bin NOT NULL, character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM character set utf8 comment='Stored Procedures'; +CREATE TABLE IF NOT EXISTS proc (db char(64) collate utf8_bin +DEFAULT '' NOT NULL, name char(64) DEFAULT '' NOT NULL, type +enum('FUNCTION','PROCEDURE') NOT NULL, specific_name char(64) +DEFAULT '' NOT NULL, language enum('SQL') DEFAULT 'SQL' NOT +NULL, sql_data_access enum( 'CONTAINS_SQL', 'NO_SQL', +'READS_SQL_DATA', +'MODIFIES_SQL_DATA') DEFAULT 'CONTAINS_SQL' NOT NULL, +is_deterministic enum('YES','NO') DEFAULT 'NO' NOT NULL, +security_type enum('INVOKER','DEFINER') DEFAULT 'DEFINER' NOT +NULL, param_list blob NOT NULL, returns longblob NOT NULL, body +longblob NOT NULL, definer char(141) collate utf8_bin DEFAULT '' +NOT NULL, created timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP +ON UPDATE CURRENT_TIMESTAMP, modified timestamp NOT NULL DEFAULT +'0000-00-00 00:00:00', sql_mode set( 'REAL_AS_FLOAT', +'PIPES_AS_CONCAT', 'ANSI_QUOTES', 'IGNORE_SPACE', +'IGNORE_BAD_TABLE_OPTIONS', 'ONLY_FULL_GROUP_BY', +'NO_UNSIGNED_SUBTRACTION', 'NO_DIR_IN_CREATE', 'POSTGRESQL', +'ORACLE', 'MSSQL', 'DB2', 'MAXDB', 'NO_KEY_OPTIONS', +'NO_TABLE_OPTIONS', 'NO_FIELD_OPTIONS', 'MYSQL323', 'MYSQL40', +'ANSI', 'NO_AUTO_VALUE_ON_ZERO', 'NO_BACKSLASH_ESCAPES', +'STRICT_TRANS_TABLES', 'STRICT_ALL_TABLES', 'NO_ZERO_IN_DATE', +'NO_ZERO_DATE', 'INVALID_DATES', 'ERROR_FOR_DIVISION_BY_ZERO', +'TRADITIONAL', 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', +'NO_ENGINE_SUBSTITUTION', +'PAD_CHAR_TO_FULL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' +NOT NULL, comment text collate utf8_bin NOT NULL, +character_set_client char(32) collate utf8_bin, +collation_connection +char(32) collate utf8_bin, db_collation char(32) collate +utf8_bin, +body_utf8 longblob, PRIMARY KEY (db,name,type)) engine=MyISAM +character set utf8 comment='Stored Procedures';

CREATE TABLE IF NOT EXISTS procs_priv ( Host char(60) binary DEFAULT '' NOT NULL, Db char(64) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Routine_name char(64) COLLATE utf8_general_ci DEFAULT '' NOT NULL, Routine_type enum('FUNCTION','PROCEDURE') NOT NULL, Grantor char(141) DEFAULT '' NOT NULL, Proc_priv set('Execute','Alter Routine','Grant') COLLATE utf8_general_ci DEFAULT '' NOT NULL, Timestamp timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (Host,Db,User,Routine_name,Routine_type), KEY Grantor (Grantor) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Procedure privileges';

@@ -101,7 +101,7 @@ PREPARE stmt FROM @str; EXECUTE stmt; DROP PREPARE stmt;

-CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', name char(64) CHARACTER SET utf8 NOT NULL default '', body longblob NOT NULL, definer char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', execute_at DATETIME default NULL, interval_value int(11) default NULL, interval_field ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' M ICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND','HO UR _ M I NUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MICRO SE C O N D','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT '0000-00-00 00:00:00', last_executed DATETIME default NULL, starts DATETIME default NULL, ends DATETIME default NULL, status ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default 'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL default 'DROP', sql_mode set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' I G NORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTRACTION' ,'NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB',' NO _ K E Y_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323','MYSQL40' , 'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRA NS _ T A BLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVAL ID _ D A TES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER' , 'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_ LE N G T H') DEFAULT '' NOT NULL, comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default '', originator INTEGER UNSIGNED NOT NULL, time_zone char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', character_set_client char(32) collate utf8_bin, collation_connection char(32) collate utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT CHARSET=utf8 COMMENT 'Events'; +CREATE TABLE IF NOT EXISTS event ( db char(64) CHARACTER SET +utf8 COLLATE utf8_bin NOT NULL default '', name char(64) +CHARACTER SET +utf8 NOT NULL default '', body longblob NOT NULL, definer +char(141) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL default +'', execute_at DATETIME default NULL, interval_value int(11) +default NULL, interval_field +ENUM('YEAR','QUARTER','MONTH','DAY','HOUR','MINUTE','WEEK','SECOND',' +MICROSECOND','YEAR_MONTH','DAY_HOUR','DAY_MINUTE','DAY_SECOND',' +HO +U +R +_ +MINUTE','HOUR_SECOND','MINUTE_SECOND','DAY_MICROSECOND','HOUR_MI +CR +O +S +E +COND','MINUTE_MICROSECOND','SECOND_MICROSECOND') default NULL, +created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE +CURRENT_TIMESTAMP, modified TIMESTAMP NOT NULL DEFAULT +'0000-00-00 00:00:00', last_executed DATETIME default NULL, +starts DATETIME default NULL, ends DATETIME default NULL, status +ENUM('ENABLED','DISABLED','SLAVESIDE_DISABLED') NOT NULL default +'ENABLED', on_completion ENUM('DROP','PRESERVE') NOT NULL +default 'DROP', sql_mode +set('REAL_AS_FLOAT','PIPES_AS_CONCAT','ANSI_QUOTES','IGNORE_SPACE',' +I +GNORE_BAD_TABLE_OPTIONS','ONLY_FULL_GROUP_BY','NO_UNSIGNED_SUBTR +AC +T +I +O +N','NO_DIR_IN_CREATE','POSTGRESQL','ORACLE','MSSQL','DB2','MAXDB',' +N +O +_KEY_OPTIONS','NO_TABLE_OPTIONS','NO_FIELD_OPTIONS','MYSQL323',' +MY +S +Q +L +40','ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRIC +T_ +T +R +A +NS_TABLES','STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE', +'I +N +V +A +LID_DATES','ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_C +RE +A +T +E +_USER','HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_ +TO +_ +F +U +LL_LENGTH','CONCAT_NULL_YIELDS_NULL_OFF') DEFAULT '' NOT NULL, +comment char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL +default '', originator INTEGER UNSIGNED NOT NULL, time_zone +char(64) CHARACTER SET latin1 NOT NULL DEFAULT 'SYSTEM', +character_set_client +char(32) collate utf8_bin, collation_connection char(32) collate +utf8_bin, db_collation char(32) collate utf8_bin, body_utf8 +longblob, PRIMARY KEY (db, name) ) ENGINE=MyISAM DEFAULT +CHARSET=utf8 COMMENT 'Events';

SET @create_innodb_table_stats="CREATE TABLE IF NOT EXISTS innodb_table_stats ( database_name VARCHAR(64) NOT NULL, diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql index ea1059c..b3c0ce9 100644 --- a/scripts/mysql_system_tables_fix.sql +++ b/scripts/mysql_system_tables_fix.sql @@ -442,7 +442,8 @@ ALTER TABLE proc MODIFY name char(64) DEFAULT '' NOT NULL, 'NO_AUTO_CREATE_USER', 'HIGH_NOT_PRECEDENCE', 'NO_ENGINE_SUBSTITUTION', - 'PAD_CHAR_TO_FULL_LENGTH' + 'PAD_CHAR_TO_FULL_LENGTH', + 'CONCAT_NULL_YIELDS_NULL_OFF' ) DEFAULT '' NOT NULL, DEFAULT CHARACTER SET utf8;

diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 9de55c4..c279a85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -123,7 +123,7 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FU + LL + _ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, {NULL, 0} }, { diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc index fe1c470..7b22f4b 100644 --- a/sql/item_strfunc.cc +++ b/sql/item_strfunc.cc @@ -619,19 +619,42 @@ String *Item_func_concat::val_str(String *str) DBUG_ASSERT(fixed == 1); String *res,*res2,*use_as_buff; uint i; - bool is_const= 0; + bool is_const = 0;

Please follow the MariaDB coding style:

1. In assignment operator: - there is no space before "=" - there is one space after "=".

So the above line "bool is_const= 0;" should not be modified.

2. Don't use tabs, use spaces instead.

3. There should not be trailing white spaces. Please use "git diff --check".

Note, the old code may not always be following the coding style. So don't be confused :) Some lines are old enough, older than the coding style policy.

Please make sure that all new and modified line follows the coding style.

...

+ uint firstarg = 0; + uint nextarg = 1;

These two variables basically repeat each other, and repeat "i".

Also, I noticed that args[0]->val_str() is called twice.

One time in the "// Search first non null argument" loop, and then before the "for (i= nextarg; i < arg_count ; i++)" again.

I suggest to remove firstarg and nextarg and always use "i" instead, and to make sure that val_str() is not called twice for the same argument.

...

- null_value=0; - if (!(res=args[0]->val_str(str))) + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

current_thd is a heavy function. It's better to avoid its execution per row. Please add a new boolean member and initialize it to false in constructors:

class Item_func_concat :public Item_str_func { m_null_yierds_not_null; String tmp_value; public: Item_func_concat(THD *thd, List<Item> &list): Item_str_func(thd, list), m_null_yield_not_null(false) {} Item_func_concat(THD *thd, Item *a, Item *b): Item_str_func(thd, a, b), m_null_yield_not_null(false) {} ... };

and then set it according to sql_mode in fix_length_and_dec():

void Item_func_concat::fix_length_and_dec() { m_null_yields_not_null= current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF; ... }

After this, we change all tests like:

if (current_thd->variables.sql_mode & MODE_CONCAT_NULL_YIELDS_NULL_OFF)

to

if (m_null_yields_not_null)

...

+ { + // Search first non null argument + i = 0; + while (!(res = args[i]->val_str(str))) + { + if (++i == arg_count) + break; + } + if (res) + { + firstarg=i++; + nextarg = i; + } + }

I find something like this more readable:

... // Search first non null argument for (i= 0; i < arg_count; i++) { if ((res= args[i]->val_str(str)) break; } if (i == arg_count) goto null; ...

...

+ + null_value = 0; + if (!(res = args[firstarg]->val_str(str)))

There are tabs in the above two lines. Please use spaces.

...

goto null; - use_as_buff= &tmp_value; - is_const= args[0]->const_item(); - for (i=1 ; i < arg_count ; i++) + use_as_buff = &tmp_value; + is_const = args[firstarg]->const_item(); + + for (i= nextarg; i < arg_count ; i++) { if (res->length() == 0) { - if (!(res=args[i]->val_str(str))) - goto null; + if (!(res = args[i]->val_str(str))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } /* CONCAT accumulates its result in the result of its the first non-empty argument. Because of this we need is_const to be @@ -641,8 +664,12 @@ String *Item_func_concat::val_str(String *str) } else { - if (!(res2=args[i]->val_str(use_as_buff))) - goto null; + if (!(res2 = args[i]->val_str(use_as_buff))) + { + if (current_thd->variables.sql_mode & + MODE_CONCAT_NULL_YIELDS_NULL_OFF)

if (m_null_yields_not_null)

...

+ continue; + goto null; + } if (res2->length() == 0) continue; if (res->length()+res2->length() > diff --git a/sql/sp.cc b/sql/sp.cc index eb542a6..288b7a9 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -130,7 +130,7 @@ TABLE_FIELD_TYPE proc_table_fields[MYSQL_PROC_FIELD_COUNT] = "'ANSI','NO_AUTO_VALUE_ON_ZERO','NO_BACKSLASH_ESCAPES','STRICT_TRANS_TABLES'," "'STRICT_ALL_TABLES','NO_ZERO_IN_DATE','NO_ZERO_DATE','INVALID_DATES'," "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER'," - "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FULL_LENGTH')") }, + + "'HIGH_NOT_PRECEDENCE','NO_ENGINE_SUBSTITUTION','PAD_CHAR_TO_FU + LL + _ + L + E + NGTH','CONCAT_NULL_YIELDS_NULL_OFF')") }, { NULL, 0 } }, { diff --git a/sql/sql_class.h b/sql/sql_class.h index 6f8e884..c754092 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -139,6 +139,7 @@ enum enum_binlog_row_image { #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) +#define MODE_CONCAT_NULL_YIELDS_NULL_OFF (1ULL << 32)

/* Bits for different old style modes */ #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 27e6463..c51b1de3 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -3032,7 +3032,8 @@ export sql_mode_t expand_sql_mode(sql_mode_t sql_mode) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | MODE_NO_KEY_OPTIONS | MODE_NO_TABLE_OPTIONS | - MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER); + MODE_NO_FIELD_OPTIONS | MODE_NO_AUTO_CREATE_USER | + MODE_CONCAT_NULL_YIELDS_NULL_OFF); if (sql_mode & MODE_MSSQL) sql_mode|= (MODE_PIPES_AS_CONCAT | MODE_ANSI_QUOTES | MODE_IGNORE_SPACE | @@ -3097,7 +3098,7 @@ static const char *sql_mode_names[]= "STRICT_ALL_TABLES", "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE", "NO_ENGINE_SUBSTITUTION", - "PAD_CHAR_TO_FULL_LENGTH", + "PAD_CHAR_TO_FULL_LENGTH","CONCAT_NULL_YIELDS_NULL_OFF", 0 };

diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index c7f148a..fc93534 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -1228,7 +1228,7 @@ class Sys_var_flagset: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 1); SYSVAR_ASSERT(typelib.count <= 65); - SYSVAR_ASSERT(def_val < my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val < my_set_bits64(typelib.count)); SYSVAR_ASSERT(strcmp(values[typelib.count-1], "default") == 0); SYSVAR_ASSERT(size == sizeof(ulonglong)); } @@ -1276,7 +1276,7 @@ class Sys_var_flagset: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; @@ -1337,7 +1337,7 @@ class Sys_var_set: public Sys_var_typelib global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); - SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); + SYSVAR_ASSERT(def_val <= my_set_bits64(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } bool do_check(THD *thd, set_var *var) @@ -1375,7 +1375,7 @@ class Sys_var_set: public Sys_var_typelib { longlong tmp=var->value->val_int(); if ((tmp < 0 && ! var->value->unsigned_flag) - || (ulonglong)tmp > my_set_bits(typelib.count)) + || (ulonglong)tmp > my_set_bits64(typelib.count)) return true; else var->save_result.ulonglong_value= tmp; -- 2.6.3.windows.1