Hi Davi
Congratulations on finding a fix for this issue which has been plaguing
the grammar for so long.
I am asking for minor changes, see below,
so a new patch needs to be prepared for 5.1 (and will be approved right
away).
Regards,
Marc
Davi Arnaut wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 repository of davi. When davi does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-11-26 10:28:57-02:00, davi@stripped +3 -0
> Bug#22312 Syntax error in expression with INTERVAL()
>
> Parser rejects valid INTERVAL() expressions when associated with
> arithmetic operators. The problem is the way in which the expression
> and interval grammar rules were organized caused shift/reduce conflicts,
> partly due to the expression parenthesis recursion.
>
> The solution is to tweak the interval() and expression recursion rules
> to avoid the shift/reduce conflicts. The changes are kept small enough
> because the expression rule is fragile but very crucial.
>
> mysql-test/r/parser.result@stripped, 2007-11-26 10:28:51-02:00, davi@stripped +37 -0
> Add test case result for Bug#22312
>
> mysql-test/t/parser.test@stripped, 2007-11-26 10:28:51-02:00, davi@stripped +23 -0
> Add test case for Bug#22312
>
> sql/sql_yacc.yy@stripped, 2007-11-26 10:28:51-02:00, davi@stripped +11 -15
> Resolve shift/reduce conflicts by reorganizing the interval
> expression and expression recursion rules.
>
> diff -Nrup a/mysql-test/r/parser.result b/mysql-test/r/parser.result
> --- a/mysql-test/r/parser.result 2007-08-09 20:30:56 -03:00
> +++ b/mysql-test/r/parser.result 2007-11-26 10:28:51 -02:00
> @@ -484,3 +484,40 @@ select atan(10, 20 "p2");
> ERROR 42000: Incorrect parameters in the call to native function 'atan'
> select atan(10 AS p1, 20 AS p2);
> ERROR 42000: Incorrect parameters in the call to native function 'atan'
> +DROP TABLE IF EXISTS t1;
> +SELECT STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL 10 MINUTE;
> +STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL 10 MINUTE
> +NULL
> +SELECT STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL (INTERVAL(1,2,3) + 1) MINUTE;
> +STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL (INTERVAL(1,2,3) + 1) MINUTE
> +NULL
> +SELECT "1997-12-31 23:59:59" + INTERVAL 1 SECOND;
> +"1997-12-31 23:59:59" + INTERVAL 1 SECOND
> +1998-01-01 00:00:00
> +SELECT 1 + INTERVAL(1,0,1,2) + 1;
> +1 + INTERVAL(1,0,1,2) + 1
> +4
> +SELECT INTERVAL(1,0,1,2) + 1;
> +INTERVAL(1,0,1,2) + 1
> +3
> +SELECT INTERVAL(1,0,2,3) * 5.5;
> +INTERVAL(1,0,2,3) * 5.5
> +5.5
> +SELECT INTERVAL(3,3,1,4) / 0.5;
> +INTERVAL(3,3,1,4) / 0.5
> +4.0000
> +SELECT (INTERVAL(1,0,1,2) + 5) * 7 + INTERVAL(1,0,1,2) / 2;
> +(INTERVAL(1,0,1,2) + 5) * 7 + INTERVAL(1,0,1,2) / 2
> +50.0000
> +SELECT INTERVAL(1,0,1,2) + 1, 5 * INTERVAL(1,0,1,2);
> +INTERVAL(1,0,1,2) + 1 5 * INTERVAL(1,0,1,2)
> +3 10
> +SELECT INTERVAL(0,1) + INTERVAL(5,4,3);
> +INTERVAL(0,1) + INTERVAL(5,4,3)
> +2
> +CREATE TABLE t1 (a INT, b DATETIME);
> +INSERT INTO t1 VALUES (INTERVAL(3,2,1) + 1, "1997-12-31 23:59:59" + INTERVAL 1
> SECOND);
> +SELECT * FROM t1 WHERE a = INTERVAL(3,2,1) + 1;
> +a b
> +3 1998-01-01 00:00:00
> +DROP TABLE t1;
> diff -Nrup a/mysql-test/t/parser.test b/mysql-test/t/parser.test
> --- a/mysql-test/t/parser.test 2007-08-09 20:30:56 -03:00
> +++ b/mysql-test/t/parser.test 2007-11-26 10:28:51 -02:00
> @@ -629,3 +629,26 @@ select atan(10, 20 "p2");
> -- error ER_WRONG_PARAMETERS_TO_NATIVE_FCT
> select atan(10 AS p1, 20 AS p2);
>
> +#
> +# Bug#22312 Syntax error in expression with INTERVAL()
> +#
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
> +
> +SELECT STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL 10 MINUTE;
> +SELECT STR_TO_DATE('10:00 PM', '%h:%i %p') + INTERVAL (INTERVAL(1,2,3) + 1) MINUTE;
> +SELECT "1997-12-31 23:59:59" + INTERVAL 1 SECOND;
> +SELECT 1 + INTERVAL(1,0,1,2) + 1;
> +SELECT INTERVAL(1,0,1,2) + 1;
> +SELECT INTERVAL(1,0,2,3) * 5.5;
> +SELECT INTERVAL(3,3,1,4) / 0.5;
> +SELECT (INTERVAL(1,0,1,2) + 5) * 7 + INTERVAL(1,0,1,2) / 2;
> +SELECT INTERVAL(1,0,1,2) + 1, 5 * INTERVAL(1,0,1,2);
> +SELECT INTERVAL(0,1) + INTERVAL(5,4,3);
> +
> +CREATE TABLE t1 (a INT, b DATETIME);
> +INSERT INTO t1 VALUES (INTERVAL(3,2,1) + 1, "1997-12-31 23:59:59" + INTERVAL 1
> SECOND);
> +SELECT * FROM t1 WHERE a = INTERVAL(3,2,1) + 1;
> +DROP TABLE t1;
> diff -Nrup a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> --- a/sql/sql_yacc.yy 2007-11-23 11:37:37 -02:00
> +++ b/sql/sql_yacc.yy 2007-11-26 10:28:51 -02:00
> @@ -512,10 +512,10 @@ bool my_yyoverflow(short **a, YYSTYPE **
>
> %pure_parser /* We have threads */
> /*
> - Currently there are 279 shift/reduce conflicts.
> + Currently there are 219 shift/reduce conflicts.
> We should not introduce new conflicts any more.
> */
> -%expect 279
> +%expect 219
>
Brilliant.
I tried myself to resolve this bug earlier, as did others,
but could not pass the 55 reduce/reduce conflicts lurking in the code.
> /*
> Comments for TOKENS.
> @@ -6796,12 +6796,6 @@ simple_expr:
> {
> $$= new (YYTHD->mem_root) Item_singlerow_subselect($2);
> }
> - | '(' expr ')' { $$= $2; }
> - | '(' expr ',' expr_list ')'
> - {
> - $4->push_front($2);
> - $$= new (YYTHD->mem_root) Item_row(*$4);
> - }
> | ROW_SYM '(' expr ',' expr_list ')'
> {
> $5->push_front($3);
> @@ -6863,14 +6857,16 @@ simple_expr:
> | interval_expr interval '+' expr
> /* we cannot put interval before - */
> { $$= new (YYTHD->mem_root) Item_date_add_interval($4,$1,$2,0); }
> - | interval_expr
> + | INTERVAL_SYM '(' expr ',' expr_list ')'
> {
> - if ($1->type() != Item::ROW_ITEM)
> - {
> - my_parse_error(ER(ER_SYNTAX_ERROR));
> - MYSQL_YYABORT;
> - }
> - $$= new (YYTHD->mem_root) Item_func_interval((Item_row *)$1);
> + $5->push_front($3);
> + Item_row *item= new (YYTHD->mem_root) Item_row(*$5);
> + $$= new (YYTHD->mem_root) Item_func_interval((Item_row *)item);
> + }
Agreed with the principle:
the conflicts are resolved by in lining the rules for the INTERVAL
function here, instead of using the (broken) interval_expr
There is however more to it, see below.
> + | '(' expr ')' { $$= $2; }
Ok, Scalar expressions (code was just moved)
> + | '(' expr_list ')'
> + {
> + $$= new (YYTHD->mem_root) Item_row(*$2);
> }
This rule should not have been changed:
- "(a)" is always a scalar expression, only "(a, b)" are ROW expressions
- it conflicts with the previous rule, since the parser has 2 choices
for "(a)"
Ok, changing it back the original code (expr ',' expr_list),
the conflicts are back ...
After a couple of tweaks, I came up with this alternate patch instead,
which pushes further your idea of in lining rules in the INTERVAL()
function grammar.
What I propose for grammar change is :
> malff@lambda:~/TREE/mysql-5.1-review-22312/sql> bk diffs -u *.yy
> ===== sql_yacc.yy 1.610 vs edited =====
> --- 1.610/sql/sql_yacc.yy 2007-11-28 09:08:26 -07:00
> +++ edited/sql_yacc.yy 2007-11-29 16:56:33 -07:00
> @@ -508,10 +508,10 @@ bool my_yyoverflow(short **a, YYSTYPE **
>
> %pure_parser /* We have threads */
> /*
> - Currently there are 280 shift/reduce conflicts.
> + Currently there are 219 shift/reduce conflicts.
> We should not introduce new conflicts any more.
> */
> -%expect 280
> +%expect 219
>
> /*
> Comments for TOKENS.
> @@ -6725,15 +6725,6 @@ simple_expr:
> | interval_expr interval '+' expr
> /* we cannot put interval before - */
> { $$= new (YYTHD->mem_root) Item_date_add_interval($4,$1,$2,0); }
> - | interval_expr
> - {
> - if ($1->type() != Item::ROW_ITEM)
> - {
> - my_parse_error(ER(ER_SYNTAX_ERROR));
> - MYSQL_YYABORT;
> - }
> - $$= new (YYTHD->mem_root) Item_func_interval((Item_row *)$1);
> - }
> ;
>
> /*
> @@ -6761,6 +6752,23 @@ function_call_keyword:
> { $$= new (YYTHD->mem_root) Item_func_hour($3); }
> | INSERT '(' expr ',' expr ',' expr ',' expr ')'
> { $$= new (YYTHD->mem_root) Item_func_insert($3,$5,$7,$9); }
> + | INTERVAL_SYM '(' expr ',' expr ')' %prec INTERVAL_SYM
> + {
> + THD *thd= YYTHD;
> + List<Item> *list= new (thd->mem_root) List<Item>;
> + list->push_front($5);
> + list->push_front($3);
> + Item_row *item= new (thd->mem_root) Item_row(*list);
> + $$= new (thd->mem_root) Item_func_interval(item);
> + }
> + | INTERVAL_SYM '(' expr ',' expr ',' expr_list ')' %prec INTERVAL_SYM
> + {
> + THD *thd= YYTHD;
> + $7->push_front($5);
> + $7->push_front($3);
> + Item_row *item= new (thd->mem_root) Item_row(*$7);
> + $$= new (thd->mem_root) Item_func_interval(item);
> + }
> | LEFT '(' expr ',' expr ')'
> { $$= new (YYTHD->mem_root) Item_func_left($3,$5); }
> | MINUTE_SYM '(' expr ')'
---
Please consider this fix instead for sql_yacc.yy, use your tests (all
good), and prepare a patch for 5.1 instead of 6.0: now that the rule for
ROW expressions is not changed, the risk is much lower, and this
cleanup will help maintenance a lot.
Not to be fixed in 5.0 (the issue is minor, has a work-around, and the
grammar too different)
Optional:
the rule interval_expr was artificial, and can be removed by in lining
the content (just adjust the $N values accordingly where it's used).
Thanks,
Marc.
