On 26-11-2014 10:40, Dmitry Yemanov wrote:
> Mark,
>
>>> I'm looking at the patch, please be patient ;-)
>>
>> Any news on this?
>
> I don't have any major problems with the patch. Just a few questions:
>
> 1) Why have you decided to extend select_expr via duplication rather
> than using something like this:
>
> %type <selectExprNode> select_expr
> select_expr
> : with_clause select_expr_body order_clause limit_clause2
> {
> SelectExprNode* node = $$ = newNode<SelectExprNode>();
> node->querySpec = $2;
> node->orderClause = $3;
> node->rowsClause = $4;
> node->withClause = $1;
> }
> ;
>
> %type <rowsClause> limit_clause2
> limit_clause2
> : rows_clause
> { $$ = $1; }
> | result_offset_clause fetch_first_clause
> {
> if ($1 || $2) {
> RowsClause* rowsNode = newNode<RowsClause>();
> rowsNode->skip = $1;
> rowsNode->length = $2;
> $$ = rowsNode;
> } else {
> $$ = NULL;
> }
> }
> ;
I initially had a similar implementation (slightly more complex), but I
decided to extend select_expr so the new production reads more like the
production in the SQL standard:
Compare parse.y:
select_expr
: ...
| with_clause select_expr_body order_clause result_offset_clause
fetch_first_clause
with SQL:2011:
<query expression> ::=
[<with clause>] <query expression body>
[<order by clause>] [<result offset clause>] [<fetch first clause>]
Both will work, but I like the elegance of my current solution: it makes
it easier to compare syntax with the SQL spec*. However your suggestion
removes some repetition, and would make it easier to address your second
point.
What do you want me to do?
> 2) You have left non-standard ROWS clause for searched UPDATE/DELETE
> statements because the standard does not define OFFSET/FETCH for them,
> right? In this case, generic recommendation to use OFFSET/FETCH instead
> of the [deperecated] ROWS looks too hard, as ROWS remains pretty valid
> (and the only option) for UPDATE/DELETE.
I indeed left it out because it isn't in the standard, and I think it is
slightly weird functionality to delete or update a range like this; and
as you remark below, the wording is a bit weird.
I will change the doc to remove the word deprecated and describe this
better.
> Would it make sense to adapt OFFSET/FETCH for UPDATE/DELETE as well? Or
> does it sound too weird for end-users (we're updating/deleting, not
> fetching)?
I am not sure. I think it sounds awkward to use FETCH here, and to add
more syntax to make non-standard functionality more conforming to the
standard might be overkill. I think we'd better leave it as-is.
> 3) New reserved words (OFFSET and ROW) should be added to the
> keyword_or_column rule to simplify migration. That said, I'm wondering
> why ROWS wasn't added there too in v2.0. Probably my oversight.
What does this do? Does this allow a keyword to be used as a column name
without quoting it?
I was wondering: Do you agree with my introduction and use of
simple_value_spec with result_offset_clause and fetch_first_clause? This
restricts the values to literals (constants), parameters or variables as
in the standard, but it takes away some flexibility (like allowing
expressions, column references etc) as is currently possible with ROWS.
Mark
* Looking at the syntax I think I have found some areas where small
tweaks to parse.y might bring improved conformance to the SQL spec
without big changes to the internals of Firebird (and not breaking
existing syntax). I will look into that further.
--
Mark Rotteveel
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
