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 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. >>>>>>>>>>>>>