Re: [Maria-developers] INSERT ... RETURNING Initial code review

2019-07-10 Thread Rucha Deodhar
Hello!

I worked on the review and here is a report for the same.

CODE:

I fixed indentations and removed with_returning_list. Now I am using only
the select_result pointer to know whether returning_list is empty (except
in mysql_prepare_insert(). This function is using bool with_returning_list
which is passed to it as the last argument, instead of passing SELECT_LEX
structure)

I am not passing with_returning_list as argument anymore like previously so
I removed it from sql_parse.cc file in SQLCOM_INSERT_SELECT

Fixed the logic of my_ok() and send_eof(). I Tried sending my_ok()
regardless, but it was crashing the server. So I changed the previous code
to:
if(result)
  result->send_eof()
else
  ::my_ok()
Now INSERT...IGNORE...RETURNING is working too.

Fixed the formatting in mysql_prepare_insert() (near setup_fileds() and
setup_wild())and made it more readable. For INSERT...SELECT, I am now
calling setup_fields() and setup_wild() only once-- in
select_insert::prepare(). I am passing the last argument = false in
mysql_prepare_insert() when it is called in mysql_insert_select_prepare(),
so that setup_field() and setup_wild() is only called once.

I moved returning_list to struct LEX so that current_select and
lex->first_select_lex() is not used.

Replaced the last argument in mysql_prepare_insert() (which was entire
SELECT_LEX structure) to bool with_returning_list. Now setup_field() and
setup_wild() is called only if with_returing_list is true.

Also restored the structure of SQL_I_List like it was before introducing
saved_first. I am now storing this table separately in class select_insert
as TABLE *insert_table

I also tried these queries. They are working fine.
CREATE table t1 (pk int);
CREATE table t2 (pk int);
INSERT INTO t2 (pk) values (1), (2);
INSERT INTO t1 SELECT * FROM t2 RETURNING pk;
and
INSERT INTO t1 SELECT * FROM t1;


TESTS:

I removed redundant testcases, merged the others and fixed typo. Also
removed from all the test cases:
--disable_warnings
drop table if exists t1,t2;
--enable_warnings
#End of testcase
comments like table created successfully

Also I am now using echo only like this:
--echo #
--echo #SIMPLE INSERT STATEMENT
--echo #

Tested queries that return multiple rows and got error:
INSERT INTO t2(id2) VALUES (1) RETURNING (SELECT id1 from t1);
Error: subquery returns more than one row

I also tested queries like:
INSERT INTO t2(id2,val2) VALUES(3,'c') RETURNING (SELECT id1 FROM t2);
Error: t2 specified twice both as target for INSERT and as a separate
source for data
I have added these in test cases too.

INSERT INTO t2(id2,val2) VALUES(4,'d') RETURNING (SELECT * FROM t1);
This query fails specifically because there are multiple columns in the
subselect.
Also when there is only one column AND more than one row being returned:
INSERT INTO t2(id2,val2) VALUES(4,'d') RETURNING (SELECT id1 FROM t1);
Error: Subquery returns more than one row
But works fine when:
INSERT INTO t2(id2,val2) VALUES(4,'d') RETURNING (SELECT id1 FROM t1 where
id1=1);
Because it was returning only one row and column.

Regards,
Rucha Deodhar

On Sun, Jul 7, 2019 at 7:16 PM Vicențiu Ciorbaru 
wrote:

> Hi Rucha!
>
>
> Here is the promised review. I have run the main test suite locally and have 
> noticed
>
> that some tests fail if you run with a debug build. The failure is an 
> assertion
>
> failure on the protocol level. This means you forgot to call a my_ok or 
> my_error
>
> at an appropriate time. I suggest you try and find which one of your commits
>
> introduces this bug. A handy tool is `git bisect`. It's a great time to learn
>
> about it, if you have not used it before.
>
>
> There are quite a few other things to fix, but first, let's tackle the
>
> initial review comments.
>
>
> Now, on to the actual code.
>
>
> Your text editor inserts tabs instead of spaces. Please configure it to only
>
> insert tabs and have the "tab" size be equal to 2 spaces.
>
>
>
> > diff --git a/mysql-test/main/insert_parser.test 
> > b/mysql-test/main/insert_parser.test
>
> > new file mode 100644
>
> > index 000..5c1f9408a96
>
> > --- /dev/null
>
> > +++ b/mysql-test/main/insert_parser.test
>
> > @@ -0,0 +1,64 @@
>
> > +#
>
> > +# Syntax check for INSERT...RETURNING
>
> > +#
>
> > +
>
> > +--echo # RETURNING * and RETURNING  should work.
>
> Trailing whitespace here, please fix.
>
> > +--echo # Other cases with RETURNING should output syntax error.
>
> > +
>
> > +--disable_warnings
>
> > +drop table if exists t1,t2;
>
> > +--enable_warnings
>
> Test cases in MariaDB are written such that they do not leave behind any
>
> tables or other elements from running the test. Thus, such statements are
>
> not required (and actively discouraged) in new test cases. If tables from
>
> previous tests are present and mysql-test-run (mtr) does not mark the test as
>
> failed then inserting such statements will hide the mysql-test-run bug.
>
> Please delete.
>
> > +
>
> > +CREATE TABLE t1(id1 INT, val1 VARCHAR(1));
>
> > 

Re: [Maria-developers] INSERT ... RETURNING Initial code review

2019-07-07 Thread Rucha Deodhar
Hello!

Thanks for the review. It will indeed help me write better code and test
cases.
To begin with, I' ll first get familiar with git bisect. I'll keep you
updated about my progress.

Regards,
Rucha Deodhar


On Sun, Jul 7, 2019, 7:16 PM Vicențiu Ciorbaru  wrote:

> Hi Rucha!
>
>
> Here is the promised review. I have run the main test suite locally and have 
> noticed
>
> that some tests fail if you run with a debug build. The failure is an 
> assertion
>
> failure on the protocol level. This means you forgot to call a my_ok or 
> my_error
>
> at an appropriate time. I suggest you try and find which one of your commits
>
> introduces this bug. A handy tool is `git bisect`. It's a great time to learn
>
> about it, if you have not used it before.
>
>
> There are quite a few other things to fix, but first, let's tackle the
>
> initial review comments.
>
>
> Now, on to the actual code.
>
>
> Your text editor inserts tabs instead of spaces. Please configure it to only
>
> insert tabs and have the "tab" size be equal to 2 spaces.
>
>
>
> > diff --git a/mysql-test/main/insert_parser.test 
> > b/mysql-test/main/insert_parser.test
>
> > new file mode 100644
>
> > index 000..5c1f9408a96
>
> > --- /dev/null
>
> > +++ b/mysql-test/main/insert_parser.test
>
> > @@ -0,0 +1,64 @@
>
> > +#
>
> > +# Syntax check for INSERT...RETURNING
>
> > +#
>
> > +
>
> > +--echo # RETURNING * and RETURNING  should work.
>
> Trailing whitespace here, please fix.
>
> > +--echo # Other cases with RETURNING should output syntax error.
>
> > +
>
> > +--disable_warnings
>
> > +drop table if exists t1,t2;
>
> > +--enable_warnings
>
> Test cases in MariaDB are written such that they do not leave behind any
>
> tables or other elements from running the test. Thus, such statements are
>
> not required (and actively discouraged) in new test cases. If tables from
>
> previous tests are present and mysql-test-run (mtr) does not mark the test as
>
> failed then inserting such statements will hide the mysql-test-run bug.
>
> Please delete.
>
> > +
>
> > +CREATE TABLE t1(id1 INT, val1 VARCHAR(1));
>
> > +--echo Table t1 created successfully. Fields: id1 INT, val1 VARCHAR(1);
>
> This comment is not necessary, please delete. The fact that mtr passed means
>
> the command was succesful. Also, when adding comments, I recommend you prefix
>
> them with #, like you did for the start of the test.
>
> > +
>
> > +CREATE TABLE t2(id2 INT, val2 VARCHAR(1));
>
> > +--echo Table t2 created successfully. Fields: id2 INT, val2 VARCHAR(1);
>
> Same as previous comment, not necessary, please delete.
>
> > +
>
> > +#
>
> > +#Simple insert statement
>
> > +#
>
> > +--echo #Simple insert statement
>
> Only keep one, I suggest the --echo variant, but put a space after #.
>
> I also like to add 2 extra --echo # like so:
>
> --echo #
>
> --echo # Simple insert statement
>
> --echo #
>
>
> This makes the result file easier to read.
>
> > +INSERT INTO t1 (id1, val1) VALUES (1, 'a');
>
> > +INSERT INTO t1 (id1, val1) VALUES (2, 'b') RETURNING *;
>
> > +INSERT INTO t1 (id1, val1) VALUES (3, 'c') RETURNING id1, val1;
>
> > +INSERT INTO t1 (id1, val1) VALUES (3, 'c') RETURNING id1 AS id;
>
> > +SELECT * FROM t1;
>
> > +TRUNCATE TABLE t1;
>
> > +SELECT * FROM t1;
>
> > +
>
> > +#
>
> > +#multiple values in one insert statement
>
> > +#
>
> > +--echo #multiple values in one insert statement
>
> Same as above.
>
> > +INSERT INTO t1 VALUES (1,'a'),(2,'b');
>
> > +INSERT INTO t1 VALUES (3,'c'),(4,'d') RETURNING *;
>
> > +INSERT INTO t1 VALUES (5,'e'),(6,'f') RETURNING id1, val1;
>
> > +INSERT INTO t1 VALUES (5,'e'),(6,'f') RETURNING id1 AS id;
>
> > +
>
> > +#
>
> > +#INSERT...ON DULPICATE KEY UPDATE
>
> > +#
>
> > +--echo # INSERT...ON DULPICATE KEY UPDATE
>
> Same as above. Typo too.
>
> > +CREATE TABLE ins_duplicate (id INT PRIMARY KEY, val VARCHAR(1));
>
> > +INSERT INTO ins_duplicate VALUES (1,'a');
>
> > +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' 
> > RETURNING id,val;
>
> > +INSERT INTO ins_duplicate VALUES (3,'b') ON DUPLICATE KEY UPDATE val='c' 
> > RETURNING *;
>
> > +INSERT INTO ins_duplicate VALUES (4,'b') ON DUPLICATE KEY UPDATE val='d' 
> > RETURNING id AS id1;
>
> > +#
>
> > +# INSERT...SET
>
> > +#
>
> > +
>
> > +--echo # INSERT...SET
>
> Same as above.
>
> > +INSERT INTO  t1 SET id1 = 1, val1 = 'a';
>
> > +INSERT INTO  t1 SET id1 = 2, val1 = 'b' RETURNING *;
>
> > +INSERT INTO  t1 SET id1 = 3, val1 = 'c' RETURNING id1,val1;
>
> > +INSERT INTO  t1 SET id1 = 3, val1 = 'c' RETURNING id1 AS id;
>
> > +
>
> > +DROP TABLE t1;
>
> > +DROP TABLE t2;
>
> > +DROP TABLE ins_duplicate;
>
> > +
>
> > +#
>
> > +--echo #End of test case
>
> > +#
>
> This is superfluous, please delete everything after the last DROP TABLE.
>
> > \ No newline at end of file
>
> > diff --git a/mysql-test/main/insert_parser_1.test 
> > b/mysql-test/main/insert_parser_1.test
>
> > new file mode 100644
>
> > index 000..f83c288a800
>
> > --- /dev/null
>
> >