Re: custom error messages

[Kevin Villela]

Uh-oh haha. I forgot to qualify this with that I'm not actually on the
ZetaSQL team and had little to nothing to do with their parser - I'm on a
team that owns an internal FE to various query engines which implement the
ZetaSQL standard and use their parser. We wanted to have AutoComplete so I
added the feature to ZetaSQL internally, but this required a small fraction
of changes to the grammar file.

So, for most of your questions, I have no idea :) I do appreciate you
taking a close look at the grammar, though. I'll pass your comments along
to the team, I'm sure they will be very happy to receive the expert
suggestions!

On Fri, Jan 3, 2020 at 9:42 AM Akim Demaille <address@hidden> wrote:

> Hi Kevin,
>
> > Le 3 janv. 2020 à 14:27, Kevin Villela <address@hidden> a écrit :
> >
> > I should mention that our use-case for ZetaSQL is basically identical to
> that of Tableau's as Adrian described it.
>
> I have a few questions about your parser.
>
>
> 1. Do you know about gperf (https://www.gnu.org/software/gperf/)?  It
> looks like it would be a nice implementation of
> https://github.com/google/zetasql/blob/master/zetasql/parser/keywords.cc.
>
>
>
> 2. Why do you use "%union" rather than "%define api.value.type union" (
> https://www.gnu.org/software/bison/manual/html_node/_0025define-Summary.html#index-_0025define-api_002evalue_002etype).
> BTW, you may use one "%type <foo>" for many symbols, you don't have to have
> one per symbol.
>
>
>
> 3. And while at it, did you consider "%define api.value.type variant"?  (
> https://www.gnu.org/software/bison/manual/html_node/C_002b_002b-Variants.html
> ).
>
>
>
> 4. You have a bazillion parse-params! (
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L94)
> Many of them are pointers where I would have expected references.
>
>
>
> 5. It is written (
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L87
> )
>
> > // Bison doesn't support nested namespaces for this, so we can't use
> > // zetasql::parser.
> > %name-prefix "zetasql_bison_parser"
>
> but you shouldn't be using name-prefix here.  Have a look at api.namespace
> instead (
> https://www.gnu.org/software/bison/manual/html_node/_0025define-Summary.html#index-_0025define-api_002enamespace
> ).
>
>
>
> 6. You don't use %empty (
> https://www.gnu.org/software/bison/manual/html_node/Empty-Rules.html#index-_0025empty).
> Why?
>
>
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L2622
>
>
>
> 7. You don't use named references (
> https://www.gnu.org/software/bison/manual/html_node/Named-References.html),
> although it would certainly be clearer in some actions.  For instance
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L2061
>
> > create_external_table_statement:
> >     "CREATE" opt_or_replace opt_create_scope "EXTERNAL"
> >     "TABLE" opt_if_not_exists path_expression
> >     opt_table_element_list opt_partition_by_clause_no_hint
> >     opt_cluster_by_clause_no_hint opt_options_list
> >       {
> >         if ($11 == nullptr) {
> >           YYERROR_AND_ABORT_AT(
> >               @11,
> >               "Syntax error: Expected keyword OPTIONS");
> >         }
> >         auto* create =
> >             MAKE_NODE(ASTCreateExternalTableStatement, @$,
> >             {$7, $8, $9, $10, $11});
> >         create->set_is_or_replace($2);
> >         create->set_scope($3);
> >         create->set_is_if_not_exists($6);
> >         $$ = create;
> >       }
> >     ;
>
>
> would become something like
>
> > create_external_table_statement:
> >     "CREATE" opt_or_replace opt_create_scope "EXTERNAL"
> >     "TABLE" opt_if_not_exists path_expression[path]
> >     opt_table_element_list[elements]
> opt_partition_by_clause_no_hint[partition]
> >     opt_cluster_by_clause_no_hint[cluster] opt_options_list[options]
> >       {
> >         if ($options == nullptr) {
> >           YYERROR_AND_ABORT_AT(
> >               @options,
> >               "Syntax error: Expected keyword OPTIONS");
> >         }
> >         auto* create =
> >             MAKE_NODE(ASTCreateExternalTableStatement, @$,
> >                       {$path, $elements, $partition, $cluster,
> $options});
> >         create->set_is_or_replace($opt_or_replace);
> >         create->set_scope($opt_create_scope);
> >         create->set_is_if_not_exists($opt_if_not_exists);
> >         $$ = create;
> >       }
> >     ;
>
>
>
> 8. It is written (
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L421
> )
>
> > // Precedence for operator tokens. We use operator precedence parsing
> because it
> > // is *much* faster than recursive productions (~2x speedup). The
> operator
> > // precedence is defined by the order of the declarations here, with
> tokens
> > // specified in the same declaration having the same precedence.
>
> Independently of the speedup, would you have preferred to use recursive
> productions rather than precedence directives?
>
>
>
> 9. The role of "placeholder" is quite mysterious :)  (
> https://github.com/google/zetasql/blob/aac17f8352ab620afbf6124d7193ccedc79ec786/zetasql/parser/bison_parser.y#L7258
> )