LucaCappelletti94 commented on code in PR #2317:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2317#discussion_r4111346856
##########
src/parser/mod.rs:
##########
@@ -5400,6 +5400,8 @@ impl<'a> Parser<'a> {
);
}
self.parse_create_foreign_table().map(Into::into)
+ } else if self.parse_keywords(&[Keyword::FOREIGN, Keyword::DATA,
Keyword::WRAPPER]) {
+ self.parse_create_foreign_data_wrapper().map(Into::into)
Review Comment:
You should reject `CREATE` modifiers here as the `FOREIGN TABLE` arm does.
`CREATE TEMPORARY FOREIGN DATA WRAPPER w` and `CREATE OR ALTER FOREIGN DATA
WRAPPER w` currently parse and render as `CREATE FOREIGN DATA WRAPPER w`,
silently dropping the modifier.
I will propose a red test for these.
```suggestion
} else if self.parse_keywords(&[Keyword::FOREIGN, Keyword::DATA,
Keyword::WRAPPER]) {
if has_modifier {
return parser_err!(
"CREATE FOREIGN DATA WRAPPER does not accept this
modifier",
modifier_loc
);
}
self.parse_create_foreign_data_wrapper().map(Into::into)
```
##########
src/parser/mod.rs:
##########
Review Comment:
You could move the check so both `CREATE FOREIGN` arms share one list.
```suggestion
let create_view_params = self.parse_create_view_params()?;
// `or_replace` is caught by an earlier arm. It stays so that
reordering cannot bypass it.
let has_modifier = or_replace
|| or_alter
|| temporary
|| global.is_some()
|| transient
|| volatile
|| multiset.is_some()
|| persistent
|| create_view_params.is_some();
```
##########
tests/sqlparser_postgres.rs:
##########
@@ -10138,3 +10138,99 @@ fn parse_bitstring_literal_escaping() {
pg_and_generic().verified_stmt("SELECT B''''");
pg_and_generic().verified_stmt("SELECT B'it''s'");
}
+
+#[test]
+fn parse_create_foreign_data_wrapper() {
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(stmt.name.to_string(), "myfdw");
+ assert!(stmt.handler.is_none());
+ assert!(stmt.validator.is_none());
+ assert!(stmt.options.is_none());
+
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw HANDLER myhandler";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(
+ stmt.handler,
+ Some(ForeignDataWrapperRoutineClause::Function(ObjectName::from(
+ vec!["myhandler".into()]
+ )))
+ );
+
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw NO HANDLER";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(stmt.handler, Some(ForeignDataWrapperRoutineClause::Absent));
+
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw NO VALIDATOR";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(
+ stmt.validator,
+ Some(ForeignDataWrapperRoutineClause::Absent)
+ );
+
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw HANDLER myhandler VALIDATOR
myvalidator OPTIONS (debug 'true')";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(
+ stmt.options,
+ Some(vec![CreateServerOption {
+ key: "debug".into(),
+ value: Ident {
+ value: "true".to_string(),
+ quote_style: Some('\''),
+ span: Span::empty(),
+ },
+ }])
+ );
+
+ let sql = "CREATE FOREIGN DATA WRAPPER myfdw NO HANDLER NO VALIDATOR";
+ let Statement::CreateForeignDataWrapper(stmt) =
pg_and_generic().verified_stmt(sql) else {
+ unreachable!()
+ };
+ assert_eq!(stmt.handler, Some(ForeignDataWrapperRoutineClause::Absent));
+ assert_eq!(
+ stmt.validator,
+ Some(ForeignDataWrapperRoutineClause::Absent)
+ );
+
+ // A schema-qualified name is not valid: FDW names are bare identifiers.
+ assert!(matches!(
+ pg_and_generic().parse_sql_statements("CREATE FOREIGN DATA WRAPPER
myschema.myfdw"),
+ Err(ParserError::ParserError(_))
+ ));
Review Comment:
Here are some red tests:
```suggestion
));
for sql in [
"CREATE TEMPORARY FOREIGN DATA WRAPPER myfdw",
"CREATE OR ALTER FOREIGN DATA WRAPPER myfdw",
"CREATE GLOBAL TEMPORARY FOREIGN DATA WRAPPER myfdw",
] {
let err = pg_and_generic().parse_sql_statements(sql).unwrap_err();
assert!(
err.to_string()
.contains("CREATE FOREIGN DATA WRAPPER does not accept this
modifier"),
"unexpected error for {sql}: {err}"
);
}
```
##########
src/parser/mod.rs:
##########
@@ -5400,6 +5400,8 @@ impl<'a> Parser<'a> {
);
}
Review Comment:
With the moved `has_modifier`, this arm reduces to the shared check.
```suggestion
if has_modifier {
return parser_err!(
"CREATE FOREIGN TABLE does not accept this modifier",
modifier_loc
);
}
```
--
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]
