Hello Alexander, Thanks for the review. I have already moved code of text_literal into lex in my last patch (empty_strings.diff). I will change it to use your new method.
Do you have news from Monty for the patch on stacktrace as notes ? Best regards, Jérôme. > -----Message d'origine----- > De : Alexander Barkov [mailto:[email protected]] > Envoyé : mardi 2 mai 2017 09:39 > À : jerome brauge > Cc : MariaDB Developers > Objet : Re: bb-10.2-ext / rpad lpad with 2 args > > Hi Jerome, > > Thanks for your contribution! > The patch generally looks good. > > This piece of code is not fully correct though: > > + Item *param_3= new (thd->mem_root) Item_string(thd, > + > param_1->collation.collation > + " ", 1); > > There are two problems here: > > 1. param_1->collation.collation is not precise at this point. > It will become valid later, after param_1 calls its fix_fields(). > > 2. In case is @@character_set_connections is UCS2, UTF16 OR UTF32 it will > not work fine, as space occupies two bytes (in UCS2 and UTF16) or four bytes > (in UTF32). We need to emulate the same behavior like when the user types > space as the last argument, so conversion from @@character_set_client to > @@character_set_connection is needed. > > To make this work correctly, we need to repeat this logic from sql_yacc.yy: > > > > text_literal: > > TEXT_STRING > > { > > LEX_CSTRING tmp; > > CHARSET_INFO *cs_con= thd->variables.collation_connection; > > CHARSET_INFO *cs_cli= thd->variables.character_set_client; > > uint repertoire= $1.repertoire(cs_cli); > > if (thd->charset_is_collation_connection || > > (repertoire == MY_REPERTOIRE_ASCII && > > my_charset_is_ascii_based(cs_con))) > > tmp= $1; > > else > > { > > LEX_STRING to; > > if (thd->convert_string(&to, cs_con, $1.str, $1.length, > > cs_cli)) > > MYSQL_YYABORT; > > tmp.str= to.str; > > tmp.length= to.length; > > } > > $$= new (thd->mem_root) Item_string(thd, tmp.str, tmp.length, > > cs_con, > > DERIVATION_COERCIBLE, > > repertoire); > > if ($$ == NULL) > > MYSQL_YYABORT; > > } > > > I'm going to move this code from sql_yacc.yy to a new method in THD: > > Item_string *THD::make_item_string(const char *str, size_t length) const; > > so you can reuse it in your patch. > > I'm starting working on it right now and will tell you when it's ready. > > Thanks. > > > > On 04/26/2017 11:45 AM, jerome brauge wrote: > > Hi Alexander, > > This is the patch for rpad/lpad with 2 args (call of make_empty_string() in > val_str() is to prepare MDEV-10574). > > > > Regard, > > Jérôme. > > > > PS : I share this code under terms of MCA. > > > > > >> -----Message d'origine----- > >> De : Alexander Barkov [mailto:[email protected]] Envoyé : mercredi 26 > >> avril 2017 06:12 À : jerome brauge Cc : MariaDB Developers Objet : > >> Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > >> > >> Hello Jerome, > >> > >> Thanks for working on this! > >> > >> On 04/25/2017 11:42 PM, jerome brauge wrote: > >>> Hello Alexander, > >>> Some times ago, we have had the below discussion. > >>> I made a patch following my first idea (it's enough from our point of > view) : > >> replace empty string by null in literals and in make_empty_string. > >>> I think this patch can be complementary to your suggestion of adding > >>> an Item_func_eq_oracle (for data inserted in default sql_mode) > >>> > >>> This patch also contains following some functions changes: > >>> - replace ('ab','a',null) returns 'a' instead null > >>> - trim / ltrim / rtrim returns null when input only contains spaces > >>> - lpad /rpad can accept 2 args (default padding char is space) and > >>> if length equals to 0, returns null -substring : if start index is > >>> equal to 0, act as if it's equals to 1 -add a function chr() as a > >>> synonym of > >>> char() (but accept only one arg) -add a function lengthb() as a > >>> synonym of lentgh() -change the semantic of length for oracle (where > >>> length() is the character length and not the byte length) > >>> > >>> What do you think about this patch ? > >>> > >>> (Perhaps I have to make 2 patchs, one for empty string, and one for > >>> functions changes with separate test file for each modified > >>> function) > >> > >> Right, can you please split the patch into pieces? > >> I suggest even more pieces than two. One patch for one function. > >> I'll create MDEVs, one MDEV per function and put examples how Oracle > >> works. > >> > >> Can we do it in the following order: > >> > >> 1. LPAD / RPAD - making the third parameter optional. > >> This extension will be good for all sql_modes. > >> I checked IBM DB2, Oracle PostgreSQL, Firebird - they all have > >> the third parameter to LPAD/RPAD optional. > >> > >> 2. CHR(). > >> > >> 3. LENGTH() and LENGTHB(). > >> > >> > >> Then we can continue with REPLACE, TRIM. SUBSTRING in no particular > >> order, one patch per function. > >> > >> > >> Some general notes: > >> > >> > >> 1. Unify *.yy files > >> > >> Please put sql_mode dependent code into LEX rather than *.yy files > directly. > >> We're going to unify the two files soon. See here for details: > >> MDEV-12518) Unify sql_yacc.yy and sql_yacc_ora.yy > >> > >> So instead of doing: > >> > >> | SUBSTRING '(' expr ',' expr ',' expr ')' > >> { > >> $$= new (thd->mem_root) Item_func_substr(thd, $3, $5, $7); > >> if ($$ == NULL) > >> MYSQL_YYABORT; > >> } > >> > >> > >> Please do something like this in both sql_yacc.yy and sql_yacc_ora.yy: > >> > >> > >> | SUBSTRING '(' expr ',' expr ',' expr ')' > >> { > >> if (!($$= Lex->make_item_func_substr(thd, $3, $5, $7))) > >> MYSQL_YYABORT; > >> } > >> > >> where LEX::make_item_func_substr() is a new method, creating either > >> Item_func_substr or Item_func_substr_oracle depending on sql_mode. > >> > >> The same applies to all other functions, and to text_literal. > >> > >> > >> 2. Don't use Item::is_null() when you call val_str() anyway. > >> > >> There are two reasons that: > >> - Performance > >> is_null() involves the second val_str() call, which can be resource > >> consuming in case of complex functions and subselect. > >> > >> - Side effect > >> Expressions can have some side effects. > >> For example, in case if the argument is a stored function which > >> counts how many times it was called and stores the counter in a user > >> variable, or writes something to a table on every execution. > >> If we do is_null() followed by val_str(), the side effect will done two > times. > >> We need one time only. > >> > >> So instead of: > >> > >> String *Item_func_replace_oracle::val_str(String *str) { > >> String * res; > >> if (res = replace_value(str, > >> args[1]->is_null() ? &tmp_emtpystr : > >> rgs[1]->val_str(&tmp_value2), > >> args[2]->is_null() ? &tmp_emtpystr : > >> rgs[2]->val_str(&tmp_value3)) > >> > >> return (res->length() == 0) ? make_empty_result () : res; > >> else > >> return 0; > >> } > >> > >> > >> Please do something like this: > >> > >> String *Item_func_replace_oracle::val_str(String *str) { > >> String * res; > >> String *str1= args[1]->val_str(&tmp_value1); > >> String *str2= args[2]->val_str(&tmp_value2); > >> if (res= replace_value(str, > >> str1 ? str1 : &tmp_emptystr, > >> str2 ? str2 : &tmp_emptystr) > >> return (res->length() == 0) ? make_empty_result () : res; > >> else > >> return 0; > >> } > >> > >> > >> Thanks! > >> > >> > >> > >>> > >>> Best regards, > >>> Jérôme. > >>> > >>>> -----Message d'origine----- > >>>> De : Alexander Barkov [mailto:[email protected]] Envoyé : lundi 23 > >>>> janvier 2017 12:49 À : jerome brauge Cc : MariaDB Developers Objet : > >>>> Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > >>>> > >>>> Hi Jerome, > >>>> > >>>> On 01/18/2017 01:22 PM, jerome brauge wrote: > >>>>> Hello Alexander, > >>>>> Sometime ago, when I ask you about plan for MDEV-10574, you > replied : > >>>>> > >>>>>> The current plan is to do these transformations: > >>>>>> > >>>>>> 1. Transform Insert > >>>>>> - insert values ("") -> insert values (null) > >>>>>> > >>>>>> 2. Transform Select > >>>>>> > >>>>>> - where v=x => (v <> "" and V=X) > >>>>>> - where v is null => (v="" or v is null) > >>>>>> > >>>>>> We didn't plan to change functions yet. Thanks for bringing this up. > >>>>>> We'll discuss this. > >>>>> > >>>>> I've done some tests just by changing : > >>>>> - insert an Item_null instead of an Item_string when $1.length==0 > >>>>> in rule text_literal of sql_yacc_ora.yy > >>>>> - return null instead of an empty string in > >>>>> Item_str_func::make_empty_result > >>>>> > >>>>> My first tests seem promising. > >>>>> > >>>>> Of course this solution does not allow to "see" the records > >>>>> created with > >>>> empty strings as null values. > >>>>> I don't see the importance of being able to do this in a transparent > way. > >>>>> We can explicitly select these row by adding rtrim on these columns. > >>>>> > >>>>> If you are interesting, I can begin to write a test to evaluate > >>>>> the coverage of > >>>> this solution. > >>>> > >>>> I'm afraid this will fix the behavior only for literals. > >>>> > >>>> This script: > >>>> > >>>> DROP TABLE t1; > >>>> CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10)); INSERT INTO t1 > >> VALUES > >>>> ('',''); SELECT COUNT(*) FROM t1 WHERE a=''; SELECT COUNT(*) FROM > >>>> t1 WHERE a=b; > >>>> > >>>> returns 0 for both SELECT queries in Oracle, and returns 1 for both > >>>> SELECT queries in MariaDB, Your approach will fix the first SELECT only. > >>>> The second will still return 1. > >>>> > >>>> > >>>> For now, I'm not sure how to do this correctly. Some ways possible: > >>>> > >>>> 1. We could add new classes, e.g. Item_func_eq_oracle as a pair for > >>>> Item_func_eq, which will handle both empty strings and NULLs inside > >>>> their > >>>> val_int() implementations. > >>>> > >>>> 2. Or there could be some after-processing on the created Item tree > >>>> which will replace all things like Item_func_eq to Item_cond_and > >>>> with Item_func_eq and Item_func_null_predicate passed as > arguments. > >>>> > >>>> Currently I'm inclined to #1, because we don't have a good > >>>> mechanism to clone items to implement #2 correctly. We'd need such > >>>> cloning to pass arguments both to Item_func_eq and > >>>> Item_func_null_predicate > >> correctly. > >>>> Some places in the code don't expect that the same arguments can be > >>>> passed to two different Item_func instances. > >>>> > >>>> On the other hand, implementing #1 will need more changes in the > >>>> optimizer. > >>>> > >>>> We were going to think on this later. > >>>> > >>>>> > >>>>> Best regard. > >>>>> Jérôme. > >>>>> _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
