Re: [Maria-developers] INSERT ... RETURNING intermediate review
Hello! Intermediate review was very helpful. It has made things more clear. I worked on it and here is the report for the same. I committed the result file for insert_parser. Here is the link: https://github.com/rucha174/server/blob/10.4/mysql-test/main/insert_parser.result I have made changes according to the diff file and also fixed REPLACE...RETURNING and have pushed the latest changes on my repo. I also ran the tests related to INSERT...RETURNING. They are passing. Also, I am willing to work on fixing some of the hacks once GSoC project implementation is perfect. Regards, Rucha Deodhar On Sun, Jul 21, 2019, 12:05 AM Vicențiu Ciorbaru wrote: > Hi Rucha! > > I've spent quite a bit of time looking at the implementation. It is > working and passes almost all test cases, although you forgot to also > commit the insert_parser updated result file. Good job so far. > > I want to give you something to work on and think about for a bit before > we do another overall overview of the whole project. Hopefully this will > help you across all your coding endeavours, not just now. What I want you > to learn is that it is important to think about how your code can and will > evolve in the future. Just getting things to work is ok up to a certain > point, but not implementing things in a decoupled fashion is what we call > "hacks". These hacks are to be avoided as much as possible as it leads to > much more difficult work down the line. Let's elaborate on the topic. > > There are a number of problems with our current code (hacks). I am not > referring to yours in particular. All these hacks are a bit difficult to > overcome in one go. I do not expect you to fix them yourself, not during > this GSoC at least, but it is something to consider and if you are willing > to work towards fixing some of them, excellent! > > Historically our codebase evolved from a simple parser to the "monster" > that you see now. Sometimes due to time constraints, things were not > implemented in the most extendable way, rather they made use of certain > particular constructs, only valid in the contexts they were written in. > Keyword here is context, as you'll soon see. Some of this contain things > you have interacted with so far in your project. Here is an example that > you've had to deal with: > > A SELECT statement has what we call a SELECT LIST, this list is > traditionally stored in SELECT_LEX::item_list. This item_list however is > overloaded at times. For example DELETE ... RETURNING uses it to store the > expressions part of the RETURNING clause. This overloading generally works > ok, unless you run into situations like you did with your INSERT ... > RETURNING project. > > INSERT ... RETURNING clauses already made use of the item_list to store > something else. Now you were forced to introduce another list in > SELECT_LEX, which we called returning_list. Everything ok so far. But then, > you ran into another problem, the bison grammar rules do not put Items in > the list you want. They *always* used item_list, so you could not use > your *returning_list*. So what did we do then? Well, we came up with > another hack! We suggested this hack to you because it is something that > will work quickly and get you unstuck and that is to use the same grammar > rules like before, but swap the item_list with the returning list > temporarily, such that the grammar rules will put the right items in the > right list. This works, but it masks the underlying problem. The problem is > that we have a very rigid implementation for *select_item_list *grammar > rule. > > What we should do is to make *select_item_list* grammar return a list > using the bison stack. That means no relying on SELECT_LEX::item_list to > store our result temporarily. You have done steps towards fixing this, > which brings us to where your code's current state. The problem with your > implementation is you have not lifted the restriction of the grammar rules > making use of SELECT_LIST::item_list. Instead, you have masked it by > returning the *address* of that member on the bison stack. The funny part > is that this *works*, but it still a hack. It is still a hack, because > these grammar rules have a side effect of modifying this member behind the > scenes. A very, very, very (!) good rule is to consider all side effects as > bad, especially now that you are starting out. With experience you might > get away with them every now-and-then, but for now avoid them like the > plague and try to remove them whenever possible. > > The current code makes use of a function I really don't like. The inline > function add_item_list(). It is one of those hacky functions that strictly > relies on context. The context is: take the thd->lex->current_select and > put the item passed as a parameter in the item_list. The diff I've attached > shows an implementation of how to use the bison stack properly. I took > inspiration from the "expr" rule. Unfortunately the hack chain goes deeper,
Re: [Maria-developers] INSERT ... RETURNING intermediate review
Hello!
The review is indeed very helpful. I'll start working on the review today
and keep you updated about the progress.
Regards,
Rucha Deodhar
On Sun, Jul 21, 2019, 12:05 AM Vicențiu Ciorbaru
wrote:
> Hi Rucha!
>
> I've spent quite a bit of time looking at the implementation. It is
> working and passes almost all test cases, although you forgot to also
> commit the insert_parser updated result file. Good job so far.
>
> I want to give you something to work on and think about for a bit before
> we do another overall overview of the whole project. Hopefully this will
> help you across all your coding endeavours, not just now. What I want you
> to learn is that it is important to think about how your code can and will
> evolve in the future. Just getting things to work is ok up to a certain
> point, but not implementing things in a decoupled fashion is what we call
> "hacks". These hacks are to be avoided as much as possible as it leads to
> much more difficult work down the line. Let's elaborate on the topic.
>
> There are a number of problems with our current code (hacks). I am not
> referring to yours in particular. All these hacks are a bit difficult to
> overcome in one go. I do not expect you to fix them yourself, not during
> this GSoC at least, but it is something to consider and if you are willing
> to work towards fixing some of them, excellent!
>
> Historically our codebase evolved from a simple parser to the "monster"
> that you see now. Sometimes due to time constraints, things were not
> implemented in the most extendable way, rather they made use of certain
> particular constructs, only valid in the contexts they were written in.
> Keyword here is context, as you'll soon see. Some of this contain things
> you have interacted with so far in your project. Here is an example that
> you've had to deal with:
>
> A SELECT statement has what we call a SELECT LIST, this list is
> traditionally stored in SELECT_LEX::item_list. This item_list however is
> overloaded at times. For example DELETE ... RETURNING uses it to store the
> expressions part of the RETURNING clause. This overloading generally works
> ok, unless you run into situations like you did with your INSERT ...
> RETURNING project.
>
> INSERT ... RETURNING clauses already made use of the item_list to store
> something else. Now you were forced to introduce another list in
> SELECT_LEX, which we called returning_list. Everything ok so far. But then,
> you ran into another problem, the bison grammar rules do not put Items in
> the list you want. They *always* used item_list, so you could not use
> your *returning_list*. So what did we do then? Well, we came up with
> another hack! We suggested this hack to you because it is something that
> will work quickly and get you unstuck and that is to use the same grammar
> rules like before, but swap the item_list with the returning list
> temporarily, such that the grammar rules will put the right items in the
> right list. This works, but it masks the underlying problem. The problem is
> that we have a very rigid implementation for *select_item_list *grammar
> rule.
>
> What we should do is to make *select_item_list* grammar return a list
> using the bison stack. That means no relying on SELECT_LEX::item_list to
> store our result temporarily. You have done steps towards fixing this,
> which brings us to where your code's current state. The problem with your
> implementation is you have not lifted the restriction of the grammar rules
> making use of SELECT_LIST::item_list. Instead, you have masked it by
> returning the *address* of that member on the bison stack. The funny part
> is that this *works*, but it still a hack. It is still a hack, because
> these grammar rules have a side effect of modifying this member behind the
> scenes. A very, very, very (!) good rule is to consider all side effects as
> bad, especially now that you are starting out. With experience you might
> get away with them every now-and-then, but for now avoid them like the
> plague and try to remove them whenever possible.
>
> The current code makes use of a function I really don't like. The inline
> function add_item_list(). It is one of those hacky functions that strictly
> relies on context. The context is: take the thd->lex->current_select and
> put the item passed as a parameter in the item_list. The diff I've attached
> shows an implementation of how to use the bison stack properly. I took
> inspiration from the "expr" rule. Unfortunately the hack chain goes deeper,
> but we need to do baby steps and some things are best left outside your
> GSoC project, or we risk going down a rabbit whole we might not get out of.
>
> One other issue I've observed is with the REPLACE code here:
>
>
> insert_field_spec
>
> {
>
> if ($6)
>
> {
>
> List list;
>
> list=*($6);
>
> Lex->current_select->item_list.empty();
>
> $6=
>
>
[Maria-developers] INSERT ... RETURNING intermediate review
Hi Rucha!
I've spent quite a bit of time looking at the implementation. It is working and
passes almost all test cases, although you forgot to also commit the
insert_parser updated result file. Good job so far.
I want to give you something to work on and think about for a bit before we do
another overall overview of the whole project. Hopefully this will help you
across all your coding endeavours, not just now. What I want you to learn is
that it is important to think about how your code can and will evolve in the
future. Just getting things to work is ok up to a certain point, but not
implementing things in a decoupled fashion is what we call "hacks". These hacks
are to be avoided as much as possible as it leads to much more difficult work
down the line. Let's elaborate on the topic.
There are a number of problems with our current code (hacks). I am not referring
to yours in particular. All these hacks are a bit difficult to overcome in one
go. I do not expect you to fix them yourself, not during this GSoC at least, but
it is something to consider and if you are willing to work towards fixing some
of them, excellent!
Historically our codebase evolved from a simple parser to the "monster" that you
see now. Sometimes due to time constraints, things were not implemented in the
most extendable way, rather they made use of certain particular constructs, only
valid in the contexts they were written in. Keyword here is context, as you'll
soon see. Some of this contain things you have interacted with so far in your
project. Here is an example that you've had to deal with:
A SELECT statement has what we call a SELECT LIST, this list is traditionally
stored in SELECT_LEX::item_list. This item_list however is overloaded at times.
For example DELETE ... RETURNING uses it to store the expressions part of the
RETURNING clause. This overloading generally works ok, unless you run into
situations like you did with your INSERT ... RETURNING project.
INSERT ... RETURNING clauses already made use of the item_list to store
something else. Now you were forced to introduce another list in SELECT_LEX,
which we called returning_list. Everything ok so far. But then, you ran into
another problem, the bison grammar rules do not put Items in the list you want.
They always used item_list, so you could not use your returning_list. So what
did we do then? Well, we came up with another hack! We suggested this hack to
you because it is something that will work quickly and get you unstuck and that
is to use the same grammar rules like before, but swap the item_list with the
returning list temporarily, such that the grammar rules will put the right items
in the right list. This works, but it masks the underlying problem. The problem
is that we have a very rigid implementation for select_item_list grammar rule.
What we should do is to make select_item_list grammar return a list using the
bison stack. That means no relying on SELECT_LEX::item_list to store our result
temporarily. You have done steps towards fixing this, which brings us to where
your code's current state. The problem with your implementation is you have not
lifted the restriction of the grammar rules making use of
SELECT_LIST::item_list. Instead, you have masked it by returning the address of
that member on the bison stack. The funny part is that this works, but it still
a hack. It is still a hack, because these grammar rules have a side effect of
modifying this member behind the scenes. A very, very, very (!) good rule is to
consider all side effects as bad, especially now that you are starting out. With
experience you might get away with them every now-and-then, but for now avoid
them like the plague and try to remove them whenever possible.
The current code makes use of a function I really don't like. The inline
function add_item_list(). It is one of those hacky functions that strictly
relies on context. The context is: take the thd->lex->current_select and put the
item passed as a parameter in the item_list. The diff I've attached shows an
implementation of how to use the bison stack properly. I took inspiration from
the "expr" rule. Unfortunately the hack chain goes deeper, but we need to do
baby steps and some things are best left outside your GSoC project, or we risk
going down a rabbit whole we might not get out of.
One other issue I've observed is with the REPLACE code here:
insert_field_spec {if
($6){ List
list; list=*($6); Lex->current_select-
>item_list.empty(); $6=} } opt_
select_expressions {if ($8) Lex-
>returning_list=*($8);if ($6) Lex->current_select-
>item_list=*($6);Lex->pop_select(); //main selectif
(Lex->check_main_unit_semantics()) MYSQL_YYABORT; }
This is really problematic and your probably wrote it because of
