Re: custom error messages

[Akim Demaille]

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)