iffyio commented on code in PR #2037:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2037#discussion_r2362172946
##########
src/ast/ddl.rs:
##########
@@ -3199,6 +3199,22 @@ pub struct CreateTrigger {
///
///
[MsSql](https://learn.microsoft.com/en-us/sql/t-sql/statements/create-trigger-transact-sql?view=sql-server-ver16#arguments)
pub or_alter: bool,
+ /// True if this is a temporary trigger, which is supported in SQLite.
+ ///
+ /// The possible syntaxes are two:
+ ///
+ /// ```sql
+ /// CREATE TEMP TRIGGER trigger_name
+ /// ```
+ ///
+ /// or
+ ///
+ /// ```sql
+ /// CREATE TEMPORARY TRIGGER trigger_name
+ /// ```
+ ///
+ /// [Temporary Triggers in
SQLite](https://sqlite.org/lang_createtrigger.html#temp_triggers_on_non_temp_tables)
Review Comment:
```suggestion
/// Examples:
///
/// ```sql
/// CREATE TEMP TRIGGER trigger_name
/// ```
///
/// or
///
/// ```sql
/// CREATE TEMPORARY TRIGGER trigger_name;
/// CREATE TEMP TRIGGER trigger_name;
/// ```
///
///
[SQLite](https://sqlite.org/lang_createtrigger.html#temp_triggers_on_non_temp_tables)
```
##########
src/ast/ddl.rs:
##########
@@ -3199,6 +3199,22 @@ pub struct CreateTrigger {
///
///
[MsSql](https://learn.microsoft.com/en-us/sql/t-sql/statements/create-trigger-transact-sql?view=sql-server-ver16#arguments)
pub or_alter: bool,
+ /// True if this is a temporary trigger, which is supported in SQLite.
Review Comment:
```suggestion
/// True if this is a temporary trigger.
```
##########
src/ast/ddl.rs:
##########
@@ -3243,14 +3259,16 @@ pub struct CreateTrigger {
/// ```
pub period: TriggerPeriod,
/// Whether the trigger period was specified before the target table name.
+ /// This does not refer to whether the period is BEFORE, AFTER, or INSTEAD
OF,
+ /// but rather the position of the period clause in relation to the table
name.
///
/// ```sql
- /// -- period_before_table == true: Postgres, MySQL, and standard SQL
+ /// -- period_specified_before_table == true: Postgres, MySQL, and
standard SQL
/// CREATE TRIGGER t BEFORE INSERT ON table_name ...;
- /// -- period_before_table == false: MSSQL
+ /// -- period_specified_before_table == false: MSSQL
/// CREATE TRIGGER t ON table_name BEFORE INSERT ...;
/// ```
- pub period_before_table: bool,
+ pub period_specified_before_table: bool,
Review Comment:
was there a requirement to rename this variable?
##########
src/ast/ddl.rs:
##########
@@ -3262,7 +3280,9 @@ pub struct CreateTrigger {
pub referencing: Vec<TriggerReferencing>,
/// This specifies whether the trigger function should be fired once for
/// every row affected by the trigger event, or just once per SQL
statement.
- pub trigger_object: TriggerObject,
+ /// This is optional in some SQL dialects, such as SQLite, and if not
specified, in
+ /// those cases, the implied default is `FOR EACH ROW`.
Review Comment:
```suggestion
```
Since the optionality is clear from the type and we can avoid documenting
semantic implications
##########
src/parser/mod.rs:
##########
@@ -5574,11 +5575,14 @@ impl<'a> Parser<'a> {
pub fn parse_create_trigger(
&mut self,
+ temporary: bool,
or_alter: bool,
or_replace: bool,
is_constraint: bool,
) -> Result<Statement, ParserError> {
- if !dialect_of!(self is PostgreSqlDialect | GenericDialect |
MySqlDialect | MsSqlDialect) {
+ if !dialect_of!(self is PostgreSqlDialect | SQLiteDialect |
GenericDialect | MySqlDialect | MsSqlDialect)
+ || dialect_of!(self is SQLiteDialect) && (or_alter || or_replace
|| is_constraint)
Review Comment:
What is the intent of the `&& (or_alter ..)` part? not sure I fully followed
##########
src/parser/mod.rs:
##########
@@ -5605,14 +5609,24 @@ impl<'a> Parser<'a> {
}
}
- self.expect_keyword_is(Keyword::FOR)?;
- let include_each = self.parse_keyword(Keyword::EACH);
- let trigger_object =
- match self.expect_one_of_keywords(&[Keyword::ROW,
Keyword::STATEMENT])? {
- Keyword::ROW => TriggerObject::Row,
- Keyword::STATEMENT => TriggerObject::Statement,
- _ => unreachable!(),
- };
+ let (include_each, trigger_object) = if
self.parse_keyword(Keyword::FOR) {
+ (
+ self.parse_keyword(Keyword::EACH),
+ Some(
+ match self.expect_one_of_keywords(&[Keyword::ROW,
Keyword::STATEMENT])? {
+ Keyword::ROW => TriggerObject::Row,
+ Keyword::STATEMENT => TriggerObject::Statement,
+ _ => unreachable!(),
+ },
+ ),
+ )
+ } else {
+ if !dialect_of!(self is SQLiteDialect ) {
+ self.expect_keyword_is(Keyword::FOR)?;
+ }
Review Comment:
```suggestion
self.expect_keyword_is(Keyword::FOR)?;
```
Can we skip the dialect guard here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
