iffyio commented on code in PR #2079:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/2079#discussion_r2477052483


##########
src/dialect/mod.rs:
##########
@@ -1207,6 +1207,17 @@ pub trait Dialect: Debug + Any {
     fn supports_semantic_view_table_factor(&self) -> bool {
         false
     }
+
+    /// Returns true if the dialect supports the `RESET` statement
+    /// for resetting session variables.
+    ///
+    /// ```sql
+    /// RESET configuration_parameter;
+    /// RESET ALL;
+    /// ```
+    fn supports_reset(&self) -> bool {
+        false
+    }

Review Comment:
   ```suggestion
   ```
   I think we can let the parser always accept the statement if it shows up, 
without considering the dialect



##########
src/parser/mod.rs:
##########
@@ -18529,4 +18542,29 @@ mod tests {
             assert!(Parser::parse_sql(&GenericDialect, &sql).is_err());
         }
     }
+
+    #[test]
+    fn test_reset_all() {
+        let sql = "RESET ALL";
+        let ast = Parser::parse_sql(&PostgreSqlDialect {}, sql).unwrap();

Review Comment:
   ```suggestion
           let ast = verified_stmt(sql);
   ```
   we should use the built in functions for the tests - those additionally 
asserts roundtrip behavior



##########
src/parser/mod.rs:
##########
@@ -18529,4 +18542,29 @@ mod tests {
             assert!(Parser::parse_sql(&GenericDialect, &sql).is_err());
         }
     }
+
+    #[test]
+    fn test_reset_all() {
+        let sql = "RESET ALL";
+        let ast = Parser::parse_sql(&PostgreSqlDialect {}, sql).unwrap();
+        assert_eq!(
+            ast,
+            vec![Statement::Reset(ResetStatement { reset: Reset::ALL })]
+        );
+    }
+
+    #[test]
+    fn test_reset_parameter() {

Review Comment:
   Can we merge the two tests into the same `parse_reset()` function? since 
they belong to the same feature



##########
src/parser/mod.rs:
##########
@@ -17723,6 +17724,18 @@ impl<'a> Parser<'a> {
             _ => self.expected("expected option value", self.peek_token()),
         }
     }
+
+    fn parse_reset(&mut self) -> Result<Statement, ParserError> {

Review Comment:
   ```suggestion
       /// Parse a [Statement::Reset]
       fn parse_reset(&mut self) -> Result<Statement, ParserError> {
   ```



##########
src/parser/mod.rs:
##########
@@ -17723,6 +17724,18 @@ impl<'a> Parser<'a> {
             _ => self.expected("expected option value", self.peek_token()),
         }
     }
+
+    fn parse_reset(&mut self) -> Result<Statement, ParserError> {
+        // RESET { ALL | <configuration_parameter> }

Review Comment:
   ```suggestion
   ```



##########
tests/sqlparser_postgres.rs:
##########
@@ -6652,3 +6652,30 @@ fn parse_foreign_key_match_with_actions() {
 
     pg_and_generic().verified_stmt(sql);
 }
+
+#[test]

Review Comment:
   I think we can move and merge this into the test function in sqlparser_common



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to