iffyio commented on code in PR #2037:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2037#discussion_r2371990786
##########
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:
> the implied default is `FOR EACH ROW`
If I'm not mistaken, this is rather describing the behavior of the sqlserver
rather than the implied default of the parser? If so we try to avoid semantic
descriptions in general since those vary vastly, if a reader is interested in
such implications we could include a reference to the sqlite docs.
Also I think a clearer representation in that case actually would be
something like the following?
```rust
enum TriggerObjectKind {
For(TriggerObject),
ForEach(TriggerObject),
}
pub trigger_object: Option<TriggerObjectKind>
```
That would let us skip the extra boolean `include_each` it looks like?
##########
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:
The Sqlite bit is documented via the documentation reference, in the
description header I think its easier to skip enumerating dialects so that the
description doesn't change or go out of sync when new dialects are added
##########
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:
I think we can skip the flags and let the parser be permissive, downstream
crates maybe validate the AST further when necessary (it would be too much of a
large effort for the parser to treat each dialect individually in such a
granular manner otherwise).
##########
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:
Ah I see, I think we can revert the diff since it might not be worth
breaking the API.
--
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
