Re: [Maria-developers] Aggregate Stored Functions
Hi, I have added the AGGREGATE keyword to the parser . Here is the link to the repository https://github.com/varunraiko/aggregate-functions . On Wed, May 18, 2016 at 9:50 PM, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
If we get it automatically then of course it should be done, but I doubts... We will see.
On Wed, May 18, 2016 at 6:18 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sanja!
On May 18, Sanja wrote:
No, sorry, this doesn't work. It works for procedures, but not for functions. See:
CREATE FUNCTION f1 (a INT) RETURNS INT BEGIN RETURN SELECT f2(val) FROM t1 WHERE id=a; END;
CREATE FUNCTION f2 (b INT) RETURNS INT BEGIN ... FETCH GROUP NEXT ROW; ... RETURN something; END;
Here, depending on what function is declared aggregate you will have different results.
I think in the first implementation we can have only one level functions. if we will have time we can then expand it for calls of other functions. But first the mechanism of temporary leaving then entering functions should be created (then it can be reused for recursive calls.
First implementation - may be (althought I don't understand why - this requires no extra coding, nested function calls will just work automatically). But the first implementation should not choose to use the syntax that makes this extension impossible.
For example, Varun's project does not include window function support. At all. But we will be able to add it later without redoing everything, exising syntax can accomodate this new feature.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi! Have you read our discussion about automatic aggregate functions detection? Am 20.05.2016 07:15 schrieb "Varun Gupta" <varungupta1803@gmail.com>:
Hi, I have added the AGGREGATE keyword to the parser . Here is the link to the repository https://github.com/varunraiko/aggregate-functions .
On Wed, May 18, 2016 at 9:50 PM, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
If we get it automatically then of course it should be done, but I doubts... We will see.
On Wed, May 18, 2016 at 6:18 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sanja!
On May 18, Sanja wrote:
No, sorry, this doesn't work. It works for procedures, but not for functions. See:
CREATE FUNCTION f1 (a INT) RETURNS INT BEGIN RETURN SELECT f2(val) FROM t1 WHERE id=a; END;
CREATE FUNCTION f2 (b INT) RETURNS INT BEGIN ... FETCH GROUP NEXT ROW; ... RETURN something; END;
Here, depending on what function is declared aggregate you will have different results.
I think in the first implementation we can have only one level functions. if we will have time we can then expand it for calls of other functions. But first the mechanism of temporary leaving then entering functions should be created (then it can be reused for recursive calls.
First implementation - may be (althought I don't understand why - this requires no extra coding, nested function calls will just work automatically). But the first implementation should not choose to use the syntax that makes this extension impossible.
For example, Varun's project does not include window function support. At all. But we will be able to add it later without redoing everything, exising syntax can accomodate this new feature.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sanja, Since we have not officially started the coding period, I figured that an exercise in adding a parser syntax would be a good first step. We can do automatic detection by looking at the cursor without to much extra work. Vicentiu On Fri, 20 May 2016 at 08:29, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
Have you read our discussion about automatic aggregate functions detection? Am 20.05.2016 07:15 schrieb "Varun Gupta" <varungupta1803@gmail.com>:
Hi, I have added the AGGREGATE keyword to the parser . Here is the link to the repository https://github.com/varunraiko/aggregate-functions .
On Wed, May 18, 2016 at 9:50 PM, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
If we get it automatically then of course it should be done, but I doubts... We will see.
On Wed, May 18, 2016 at 6:18 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sanja!
On May 18, Sanja wrote:
No, sorry, this doesn't work. It works for procedures, but not for functions. See:
CREATE FUNCTION f1 (a INT) RETURNS INT BEGIN RETURN SELECT f2(val) FROM t1 WHERE id=a; END;
CREATE FUNCTION f2 (b INT) RETURNS INT BEGIN ... FETCH GROUP NEXT ROW; ... RETURN something; END;
Here, depending on what function is declared aggregate you will have different results.
I think in the first implementation we can have only one level functions. if we will have time we can then expand it for calls of other functions. But first the mechanism of temporary leaving then entering functions should be created (then it can be reused for recursive calls.
First implementation - may be (althought I don't understand why - this requires no extra coding, nested function calls will just work automatically). But the first implementation should not choose to use the syntax that makes this extension impossible.
For example, Varun's project does not include window function support. At all. But we will be able to add it later without redoing everything, exising syntax can accomodate this new feature.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, I have added FETCH GROUP NEXT ROW to the parser and have tested it. I have also made changes according to the review to my first. New commit https://github.com/MariaDB/server/commit/65782a2a6b5ad3db466953b6ea46f87de26... On Fri, May 20, 2016 at 11:18 AM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Sanja,
Since we have not officially started the coding period, I figured that an exercise in adding a parser syntax would be a good first step.
We can do automatic detection by looking at the cursor without to much extra work.
Vicentiu
On Fri, 20 May 2016 at 08:29, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
Have you read our discussion about automatic aggregate functions detection? Am 20.05.2016 07:15 schrieb "Varun Gupta" <varungupta1803@gmail.com>:
Hi, I have added the AGGREGATE keyword to the parser . Here is the link to the repository https://github.com/varunraiko/aggregate-functions .
On Wed, May 18, 2016 at 9:50 PM, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
If we get it automatically then of course it should be done, but I doubts... We will see.
On Wed, May 18, 2016 at 6:18 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sanja!
On May 18, Sanja wrote:
> > No, sorry, this doesn't work. It works for procedures, but not for > functions. See: > > CREATE FUNCTION f1 (a INT) RETURNS INT > BEGIN > RETURN SELECT f2(val) FROM t1 WHERE id=a; > END; > > CREATE FUNCTION f2 (b INT) RETURNS INT > BEGIN > ... > FETCH GROUP NEXT ROW; > ... > RETURN something; > END; > > Here, depending on what function is declared aggregate you will have > different results.
I think in the first implementation we can have only one level functions. if we will have time we can then expand it for calls of other functions. But first the mechanism of temporary leaving then entering functions should be created (then it can be reused for recursive calls.
First implementation - may be (althought I don't understand why - this requires no extra coding, nested function calls will just work automatically). But the first implementation should not choose to use the syntax that makes this extension impossible.
For example, Varun's project does not include window function support. At all. But we will be able to add it later without redoing everything, exising syntax can accomodate this new feature.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :).
Hi Varun, Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also. Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself. Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea. Great job so far! Vicentiu On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi, I had been going through the LEX struct and could not find any flag member there which could be used to specify if a function is aggregate or not. So i created the new flag inside sp_head, so as to make sure it could be used for stored procedures too in the future. I have committed the changes on GitHub :) On Mon, May 23, 2016 at 4:21 PM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also.
Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself.
Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea.
Great job so far! Vicentiu
On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Yes, the decision is right. I'll check later the code on github. On Tue, May 24, 2016 at 10:27 AM, Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, I had been going through the LEX struct and could not find any flag member there which could be used to specify if a function is aggregate or not. So i created the new flag inside sp_head, so as to make sure it could be used for stored procedures too in the future. I have committed the changes on GitHub :)
On Mon, May 23, 2016 at 4:21 PM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also.
Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself.
Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea.
Great job so far! Vicentiu
On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi Varun, I've reviewed your patch. Looks good from my side. Just stylistic comments. Feel free to keep your own version if you don't agree with them. I think that you could have used the m_flags field, but having a specific member makes things a lot clearer in my opinion. Perhaps Sanja has a different opinion. Next step is to figure out how to use this new flag from sp_head in the execution part. If you get completely stuck, let us know. :) Vicentiu On Tue, 24 May 2016 at 11:30 Sanja <sanja.byelkin@gmail.com> wrote:
Yes, the decision is right. I'll check later the code on github.
On Tue, May 24, 2016 at 10:27 AM, Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, I had been going through the LEX struct and could not find any flag member there which could be used to specify if a function is aggregate or not. So i created the new flag inside sp_head, so as to make sure it could be used for stored procedures too in the future. I have committed the changes on GitHub :)
On Mon, May 23, 2016 at 4:21 PM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also.
Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself.
Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea.
Great job so far! Vicentiu
On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi! I reviewed yet another time the code and have following toughts: a) if we store the tag separately (like deterministic or not) then separate field is better b) If we get the value form function content then flag is what we need. for now we are going by a) i.e. we clearly in the header of the function state that it is aggregate function independently of the function body. The other question where to store the tag: a) to have different type (function/procedure/trigger/aggregate function) b) to add new field c) use deterministic fiield a) IMHO it is bad way, at the moment we find the fucntion name we know that it is function name and nothing more b) good way, also require adding upgrade procedure c) a bit confusing and alsi we probably could have aggregate non-deterministic functions On Tue, May 24, 2016 at 10:59 AM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
I've reviewed your patch. Looks good from my side. Just stylistic comments. Feel free to keep your own version if you don't agree with them.
I think that you could have used the m_flags field, but having a specific member makes things a lot clearer in my opinion. Perhaps Sanja has a different opinion.
Next step is to figure out how to use this new flag from sp_head in the execution part. If you get completely stuck, let us know. :)
Vicentiu
On Tue, 24 May 2016 at 11:30 Sanja <sanja.byelkin@gmail.com> wrote:
Yes, the decision is right. I'll check later the code on github.
On Tue, May 24, 2016 at 10:27 AM, Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, I had been going through the LEX struct and could not find any flag member there which could be used to specify if a function is aggregate or not. So i created the new flag inside sp_head, so as to make sure it could be used for stored procedures too in the future. I have committed the changes on GitHub :)
On Mon, May 23, 2016 at 4:21 PM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also.
Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself.
Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea.
Great job so far! Vicentiu
On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi, Regarding the plan for pausing and restarting the execution of the function when we have to fetch a row, 1) is that we escape from the function when we come across a FETCH GROUP statement , a state could be defined in thd->lex->sphead which would tell that the function is paused and then we proceed with fetching the next row . 2) After we fetch the next row we return to the function and change the state to running and then with the help of instruction pointer fetch the next instruction to be executed . 3) We continue with this as long as their are rows available to be fetched. As soon as there are no more rows available we return the result for the aggregate function. This is the basic design of what I am thinking and now I am going through the code to see where all do I have to add code to get the desired results :) . On Tue, May 24, 2016 at 3:15 PM, Sanja <sanja.byelkin@gmail.com> wrote:
Hi!
I reviewed yet another time the code and have following toughts:
a) if we store the tag separately (like deterministic or not) then separate field is better
b) If we get the value form function content then flag is what we need.
for now we are going by a) i.e. we clearly in the header of the function state that it is aggregate function independently of the function body.
The other question where to store the tag:
a) to have different type (function/procedure/trigger/aggregate function) b) to add new field c) use deterministic fiield
a) IMHO it is bad way, at the moment we find the fucntion name we know that it is function name and nothing more b) good way, also require adding upgrade procedure c) a bit confusing and alsi we probably could have aggregate non-deterministic functions
On Tue, May 24, 2016 at 10:59 AM, Vicențiu Ciorbaru <cvicentiu@gmail.com> wrote:
Hi Varun,
I've reviewed your patch. Looks good from my side. Just stylistic comments. Feel free to keep your own version if you don't agree with them.
I think that you could have used the m_flags field, but having a specific member makes things a lot clearer in my opinion. Perhaps Sanja has a different opinion.
Next step is to figure out how to use this new flag from sp_head in the execution part. If you get completely stuck, let us know. :)
Vicentiu
On Tue, 24 May 2016 at 11:30 Sanja <sanja.byelkin@gmail.com> wrote:
Yes, the decision is right. I'll check later the code on github.
On Tue, May 24, 2016 at 10:27 AM, Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, I had been going through the LEX struct and could not find any flag member there which could be used to specify if a function is aggregate or not. So i created the new flag inside sp_head, so as to make sure it could be used for stored procedures too in the future. I have committed the changes on GitHub :)
On Mon, May 23, 2016 at 4:21 PM, Vicențiu Ciorbaru <cvicentiu@gmail.com
wrote:
Hi Varun,
Getting the parser to accept the syntax is a good first step. Writing tests is the correct way to go also.
Now we need to have a way to pass this extra information to the part of the code that stores / executes this procedure. When we encounter this AGGREGATE_SYM syntax we have to record it somewhere. We generally use the LEX structure for this. See if there is any flag member within it that you can use for this purpose. If you can't find any, you can potentially create one yourself.
Now, it would be a good time to try and familiarize yourself with how we get from having a regular parsed function to storing it and afterwards executing it. This is the main logic that we have to deal with. I'm not going to suggest you any specific thing to do right now as there are multiple ways to do this. Try and come up with a simple plan on how to extend this functionality for our use case. You don't have to code it all, just yet :). We'll improve (or perhaps change it) afterwards. It doesn't have to be perfect the first time, but this way you'll get a try at designing an implementation idea.
Great job so far! Vicentiu
On Mon, 23 May 2016 at 09:04 Varun Gupta <varungupta1803@gmail.com> wrote:
Hi, As in my previous mail I have added the FETCH statement to the parser and have tested it, when the syntax is correct . Now I am writing test that would also give an error for incorrect syntax. Also I would like how to proceed further :). _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (3)
-
Sanja -
Varun Gupta -
Vicențiu Ciorbaru