Hello Jerome, On 10/13/2017 06:05 PM, jerome brauge wrote:
Hi Alexander, I agree, your code is more clearer. I've added your additional tests and done the pull request.
I've merged and closed the pull request. Thank you very much for cooperation!
Thank you very much. Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : vendredi 13 octobre 2017 14:34 À : jerome brauge; maria-developers Objet : Re: MDEV-14013
Hello Jerome,
Thank you very much for you contribution!
On 10/11/2017 02:32 PM, jerome brauge wrote:
Hello Alexander, Here is the patch for MDEV-14013, can you review it ?
The patch looks very good. I only found small things:
1. LEX::make_text_literal_concat()
thd->variables.character_set_client was used instread of thd->variables.collation_connection
2. LEX::make_text_literal_concat() was somewhat hard to read. I propose to change the logic to do:
if (item->type() == Item::NULL_ITEM) { }
instead of
if (item->type() != Item::NULL_ITEM || !(thd->variables.sql_mode & MODE_EMPTY_STRING_IS_NULL))
I think it looks clearer this way. Also, I added some DBUG_ASSERTs to make more sure that a non- Item_string item is not erroneously cast to Item_string.
3. Also, I found that it's easier to place the new methods into THD instead of LEX.
Please find a patch with changes 1,2,3 attached.
Do you agree with these changes?
Also, can you please add some tests:
- SHOW CREATE VIEW into "Test in a view" in empty_string_literal.result.diff
- EXPLAIN EXTENDED for the affected grammar:
EXPLAIN EXTENDED SELECT ''; EXPLAIN EXTENDED SELECT _latin1''; EXPLAIN EXTENDED SELECT N''; EXPLAIN EXTENDED SELECT '' '';
Thanks!
Thanks.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mercredi 11 octobre 2017 04:15 À : jerome brauge Objet : Re: Question about MDEV-10574
Hello Jerome,
On 10/10/2017 04:23 PM, jerome brauge wrote:
Hello Alexander, For sql_mode EMPTY_STRING_IS_NULL, do you think that it must cover call of statement with use of user variable initialized with an empty string ?
Ex : SET sql_mode=default; SET @param='';
SET sql_mode=EMPTY_STRING_IS_NULL; PREPARE test FROM 'select 1,?'; EXECUTE test USING @param;
Or
delimiter / CREATE OR REPLACE PROCEDURE p1(IN a VARCHAR(50)) BEGIN IF a is null THEN SELECT 'IS NULL'; ELSE select 'IS NOT NULL'; END IF; END; / delimiter ; SET sql_mode=default; SET @a='';
SET sql_mode=EMPTY_STRING_IS_NULL; EXECUTE IMMEDIATE 'CALL p1(?)' USING @a; CALL p1(@a);
For me, it's a study case and we can forget it.
I agree. Oracle doesn't have user variables.
Regards, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : samedi 7 octobre 2017 11:44 À : jerome brauge Objet : Re: Patch for MDEV-10574
Hello Jerome,
On 10/06/2017 01:41 PM, jerome brauge wrote: > Hello Alexander, > >> -----Message d'origine----- >> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 5 >> octobre 2017 21:26 À : jerome brauge Objet : Re: Patch for >> MDEV-10574 >> >> Hello Jerome, >> >> >> On 10/05/2017 12:45 PM, jerome brauge wrote: >>> Hello Alexander, >>> Have you gone ahead on your reflection about nulls and empty >>> strings
?
>> >> Yes, I think so. >> >> One addition: Monty suggested to rename flags to: >> - EMPTY_STRING_IS_NULL for literals >> - EMPTY_STRING_IS_NULL_RESULTS for functions. >> >> I'm fine with EMPTY_STRING_IS_NULL for literals, but think that >> EMPTY_STRING_IS_NULL_FUNCTIONS should be more clear than >> EMPTY_STRING_IS_NULL_RESULTS. >> Which name do you prefer for functions? > > I think like you. > For me, EMPTY_STRING_IS_NULL_RESULTS sounds like including RESULTSET > of select (which is perhaps included in point 4)
Okey, let's go with _FUNCTIONS then.
> >>> >>> Can I >>> - redo my pull request for substring_oracle function >>> - make a patch for first new sql_mode >> (MODE_EMPTY_AS_NULL_LITERALS) >> >> >> Yes, please. Let's move forward. >> >> Let's push the patch for substr_oracle first. >> Let's keep it return empty strings for the cases when the >> substring_length parameters is less than 1. >> It will start automatically returning NULL when we add the new >> sql_mode for functions. > > Pull request is done.
I have merged it. Thanks!
> >> Also, let's go ahead with the part for literals. > > Ok, I will make a separate patch. > Many thanks. > >> >> >> I have created MDEVs. Please use them in the commit comment and >> in the >> tests: >> >> MDEV-14012 sql_mode=Oracle: substr(): treat position 0 as >> position >> 1 >> MDEV-14013 sql_mode=EMPTY_STRING_IS_NULL >> >> >> Thanks! >> >>> >>> Regards, >>> Jérôme. >>> >>> >>>> -----Message d'origine----- >>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : >>>> mercredi >>>> 27 septembre 2017 08:50 À : jerome brauge Objet : Re: Patch for >>>> MDEV-10574 >>>> >>>> Hello Jerome, >>>> >>>> I told to Monty yesterday. >>>> >>>> We were going to implement the following transformations: >>>> >>>>> We are considering to implement both of the following options >>>>> (with a >>>>> switch) >>>>> >>>>> Transform Insert >>>>> - insert values ("") -> insert values (null) >>>>> >>>>> Transform Select >>>>> >>>>> where v=x => (v <> "" and V=X) >>>>> where v is null => (v="" or v is null) >>>>> >>>> >>>> See MDEV-10574 for details. >>>> >>>> Monty did not fully understand the idea of translating literals >>>> and >> functions >>>> from empty strings to null. He asked if you can explain use >>>> cases for these transformations. >>>> >>>> But we agreed that: >>>> >>>> 1. It's OK to have multiple NULL handling flags in sql_mode. >>>> >>>> 2. New flags will not be enabled in 10.3, neither in >>>> sql_mode=DEFAULT, >> nor in >>>> sql_mode=ORACLE. So one will have to specify them explicitly: >>>> set >>>> >>
sql_mode='ORACLE,MODE_null_transfrormation_flag1,MODE_null_transfor
>>>> mation_flag2'; >>>> >>>> >>>> >>>> So I propose we introduce the following flags: >>>> >>>> >>>> 1. MODE_EMPTY_AS_NULL_LITERALS - for literals and PS parameters, as >> in >>>> your patch. >>>> >>>> 2. MODE_EMPTY_AS_NULL_FUNCTIONS - for functions. >>>> This should be done as a separate patch. >>>> Note, your patch currently implements empty-to-null translation >>>> only for Item_str_func descendants that use make_empty_result(). >>>> This patch should be extended to cover Item_str_func descendants >>>> which do not use make_empty_result(), as well as >>>> all other Item_func_or_sum descendants >>>> (which are not Item_str_func descendants). >>>> >>>> >>>> 3. MODE_EMPTY_AS_NULL_UPDATES >>>> >>>> For INSERT and UPDATE transformation, as in MDEV: >>>> >>>> insert values ("") -> insert values (null) >>>> update set a='' -> update set a=null; >>>> >>>> >>>> 4. MODE_EMPTY_AS_NULL_PREDICATES >>>> >>>> For predicate transformation, as in MDEV: >>>> >>>> where v=x => (v <> "" and V=X) >>>> where v is null => (v="" or v is null) >>>> >>>> >>>> What do you think about this proposal with multiple flags? >>>> Note, we can do N1 and N2 now, and add N3 and N4 and later. >>>> >>>> Also, can you please write a description why you need N1 and N2. >>>> Monty thinks that N3 and N4 should cover most use cases. >>>> So we need a good rationale for Monty and Sergei explaining why >>>> we >> adding >>>> these flags N1 and N2. >>>> >>>> >>>> Thanks! >>>> >>>> >>>> On 09/26/2017 04:33 PM, jerome brauge wrote: >>>>> Hi Alexander, >>>>> I'm disappointed. >>>>> Can we at least keep this part in sql_prepare : >>>>> >>>>> /* >>>>> set_param_str_oracle : convert empty string to null value >>>>> This allow to bind empty string in JDBC as a null value. >>>>> Ex : >>>>> String sel="select coalesce(?,'null param') from dual"; >>>>> PreparedStatement ps_sel = m_cnx.prepareStatement(sel); >>>>> ps_sel.setString(1, ""); >>>>> ResultSet rs = ps_sel.executeQuery(); >>>>> while (rs.next()) >>>>> { >>>>> System.out.println(rs.getString(1)); >>>>> } >>>>> Returns : 'null param' >>>>> */ >>>>> static void set_param_str_oracle(Item_param *param, uchar **pos, >> ulong >>>>> len) { >>>>> ulong length= get_param_length(pos, len); >>>>> if (length == 0) >>>>> param->set_null(); >>>>> else >>>>> { >>>>> if (length > len) >>>>> length= len; >>>>> param->set_str((const char *) *pos, length); >>>>> *pos+= length; >>>>> } >>>>> } >>>>> >>>>> Regards, >>>>> Jérôme. >>>>> >>>>> >>>>>> -----Message d'origine----- >>>>>> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mardi >>>>>> 26 septembre 2017 13:34 À : jerome brauge Objet : Re: Patch >>>>>> for >>>>>> MDEV-10574 >>>>>> >>>>>> Hello Jérôme, >>>>>> >>>>>> I'm afraid we cannot accept this change at this point of time. >>>>>> >>>>>> The problem is that all around the code we assume that NULL >>>>>> is not equal to ''. >>>>>> >>>>>> Changing '' to NULL only in the visible part of the iceberg >>>>>> (such as in the parser and in >>>>>> Item_str_func::make_empty_result()) >>>>>> will likely introduce various anomalies in fundamental things >>>>>> like joins execution, optimizer, equality propagation, etc. >>>>>> >>>>>> Sorry, we don't have resources to dive into this topic deeper now. >>>>>> Moreover, Oracle's documentation suggests not to reply on the >>>>>> fact >> that '' >>>>>> and NULL are treated as same, as this can change in the future. >>>>>> >>>>>> >>>>>> >>>>>> On 09/20/2017 03:52 PM, jerome brauge wrote: >>>>>>> Hello Alexander, >>>>>>> Here is a new version of this patch (minor optimizations) >>>>>>> >>>>>>> Regards, >>>>>>> Jérôme. >>>>>>> >>>>>>> >>>>>>>> -----Message d'origine----- De : Maria-developers >>>>>>>> [mailto:maria-developers- >>>>>>>> bounces+j.brauge=qualiac.com@lists.launchpad.net] De la >>>>>>>> bounces+part de >>>>>>>> bounces+jerome >>>>>>>> brauge >>>>>>>> Envoyé : mardi 19 septembre 2017 16:09 À : 'Alexander Barkov' >>>>>>>> Cc : MariaDB Developers >>>>>>>> (maria-developers@lists.launchpad.net) >>>>>>>> Objet : [Maria-developers] Patch for MDEV-10574 >>>>>>>> >>>>>>>> Alexander, >>>>>>>> Here is the patch I was talking about. >>>>>>>> It not address all aspects of MDEV-10574, especially, empty >>>>>>>> strings in table column are not view as null. >>>>>>>> But as in oracle mode, no empty string in columns should be >>>>>>>> created, an fully oracle application will never fall in this case. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Jérôme. >>>>>>>> >>>>>>>> >>>>>>>>> -----Message d'origine----- De : jerome brauge Envoyé : >>>>>>>>> mardi 19 septembre 2017 14:13 À : 'Alexander Barkov' >>>>>>>>> Objet : RE: Oracle strings functions compatibility : >>>>>>>>> substr >>>>>>>>> >>>>>>>>> Hello Alexander, >>>>>>>>> Yes I'm aware. >>>>>>>>> This point will be treated by my next patch which will >>>>>>>>> also affect the others string functions (a part of MDEV- 10574). >>>>>>>>> I will refresh this new patch with the current development >>>>>>>>> branch, and I submit to you. >>>>>>>>> Regards. >>>>>>>>> Jérôme. >>>>>>>>> >>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : mardi >> 19 >>>>>>>>>> septembre 2017 13:16 À : jerome brauge Objet : Re: Oracle >>>>>>>>>> strings functions compatibility : substr >>>>>>>>>> >>>>>>>>>> Hi Jerome, >>>>>>>>>> >>>>>>>>>> I noticed one more difference in SUBSTR: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> SELECT SUBSTR('aaa',1,-1) FROM DUAL; >>>>>>>>>> >>>>>>>>>> MariaDB returns an empty string. >>>>>>>>>> Oracle returns NULL. >>>>>>>>>> >>>>>>>>>> Here's Oracle's documentation: >>>>>>>>>> >>>>>>>>>> >>>>>> >> https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions162. >>>>>>>>>> ht >>>>>>>>>> m >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I think we should do this in the same patch, to avoid >>>>>>>>>> behavior change in the future. >>>>>>>>>> >>>>>>>>>> Would you like to try doing this additional change? >>>>>>>>>> Or would you like me to make an incremental patch on top of >> yours? >>>>>>>>>> >>>>>>>>>> Thanks! >>>>>>>>>> >>>>>>>>>> On 09/19/2017 10:33 AM, jerome brauge wrote: >>>>>>>>>>> Hello Alexander, >>>>>>>>>>> It's done. >>>>>>>>>>> >>>>>>>>>>> Do you have news from Sergei for MDEV-12874, MDEV- 13417 >> and >>>>>>>> MDEV- >>>>>>>>>> 13418 ? >>>>>>>>>> >>>>>>>>>> I asked Sergei, awaiting for his answer. I'll let you know. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thank you very much. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : lundi >>>>>>>>>>>> 18 septembre 2017 18:12 À : jerome brauge Objet : Re: >>>>>>>>>>>> Oracle strings functions compatibility : substr >>>>>>>>>>>> >>>>>>>>>>>> Hello Jerome, >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 09/18/2017 05:14 PM, jerome brauge wrote: >>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>> I don't understand how I've missed such an obvious solution ! >>>>>>>>>>>>> Here is the new patch. >>>>>>>>>>>> >>>>>>>>>>>> Thanks. Now it looks simpler. >>>>>>>>>>>> >>>>>>>>>>>> Can you please do one small thing: >>>>>>>>>>>> move get_position() from "public:" to "protected:". >>>>>>>>>>>> >>>>>>>>>>>> Ok to make a pull request after this change. >>>>>>>>>>>> >>>>>>>>>>>> Thank you very much! >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Regards. >>>>>>>>>>>>> Jérôme. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>>> -----Message d'origine----- De : Alexander Barkov >>>>>>>>>>>>>> [mailto:bar@mariadb.org] Envoyé : >> lundi >>>>>>>>>>>>>> 18 septembre 2017 11:30 À : jerome brauge Objet : Re: >> Oracle >>>>>>>>>>>>>> strings functions compatibility : substr >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hello Jerome, >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 08/17/2017 04:43 PM, jerome brauge wrote: >>>>>>>>>>>>>>> Hello Alexander, >>>>>>>>>>>>>>> Here is a patch for function SUBSTR in sql_mode=oracle >>>>>>>>>>>>>>> (If position is 0, >>>>>>>>>>>>>> then it is treated as 1). >>>>>>>>>>>>>> >>>>>>>>>>>>>> I'm thinking of how to get a smaller patch. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Wouldn't it be possible to introduce: >>>>>>>>>>>>>> >>>>>>>>>>>>>> virtual longlong get_position(); >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> longlong Item_func_substr::get_position() { >>>>>>>>>>>>>> return args[1]->val_int(); } >>>>>>>>>>>>>> >>>>>>>>>>>>>> longlong Item_func_substr_oracle::get_position() >>>>>>>>>>>>>> { >>>>>>>>>>>>>> longlong pos= args[1]->val_int(); >>>>>>>>>>>>>> return pos == 0 ? 1 : pos; } >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> and just replace all calls for args[1]->val_int() to >> get_position() >>>> ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>> Jérôme. >>>>>>>>>>>>>>>