iffyio commented on code in PR #1980:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1980#discussion_r2239425948
##########
src/ast/mod.rs:
##########
@@ -3381,6 +3381,17 @@ pub enum Statement {
iceberg: bool,
},
/// ```sql
+ /// ALTER SCHEMA
+ /// ```
+ /// See
[BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#alter_schema_collate_statement)
+ AlterSchema {
+ /// Schema name
+ #[cfg_attr(feature = "visitor", visit(with = "visit_relation"))]
+ name: ObjectName,
+ if_exists: bool,
+ operations: Vec<AlterSchemaOperation>,
+ },
Review Comment:
Can we use a [named struct
syntax](https://github.com/apache/datafusion-sqlparser-rs/blob/0aaa561287772341ef1bfbee729d7be75e2e359b/src/ast/mod.rs#L4050-L4053)
here? e.g. `AlterSchema(AlterSchemaStatement)`
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2806,3 +2806,23 @@ fn test_begin_transaction() {
fn test_begin_statement() {
bigquery().verified_stmt("BEGIN");
}
+
+#[test]
+fn test_alter_schema_default_collate() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset SET DEFAULT
COLLATE 'und:ci'");
+}
+
+#[test]
+fn test_alter_schema_add_replica() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset ADD REPLICA
'us'");
+}
+
+#[test]
+fn test_alter_schema_drop_replica() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset DROP REPLICA
'us'");
+}
+
+#[test]
+fn test_alter_schema_set_options() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset SET OPTIONS
(location = 'us')");
+}
Review Comment:
Can we merge these into a single `test_alter_schema()` function?
Also we can move this to the sqlparser_common.rs file since the parser
covers the statement it for all dialects (I imagine other dialects have some
variant of this statement as well).
Also can we add scenarios for e.g.
```sql
ALTER SCHEMA DROP REPLICA;
ALTER SCHEMA SET OPTIONS ();
ALTER SCHEMA SET OPTIONS;
```
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2806,3 +2806,23 @@ fn test_begin_transaction() {
fn test_begin_statement() {
bigquery().verified_stmt("BEGIN");
}
+
+#[test]
+fn test_alter_schema_default_collate() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset SET DEFAULT
COLLATE 'und:ci'");
+}
+
+#[test]
+fn test_alter_schema_add_replica() {
+ bigquery_and_generic().verified_stmt("ALTER SCHEMA mydataset ADD REPLICA
'us'");
Review Comment:
from the implementation, `ADD REPLICA` takes in options, can we add test
coverage for that behavior?
##########
src/parser/mod.rs:
##########
@@ -9241,6 +9243,40 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_alter_schema(&mut self) -> Result<Statement, ParserError> {
+ let if_exists = self.parse_keywords(&[Keyword::IF, Keyword::EXISTS]);
+ let name = self.parse_object_name(false)?;
+ let operation = if self.parse_keywords(&[Keyword::SET,
Keyword::OPTIONS]) {
+ self.prev_token();
+ let options = self.parse_options(Keyword::OPTIONS)?;
+ AlterSchemaOperation::SetOptionsParens { options }
+ } else if self.parse_keywords(&[Keyword::SET, Keyword::DEFAULT,
Keyword::COLLATE]) {
+ let collate = self.parse_expr()?;
+ AlterSchemaOperation::SetDefaultCollate { collate }
+ } else if self.parse_keywords(&[Keyword::ADD, Keyword::REPLICA]) {
+ let replica = self.parse_identifier()?;
+ let options = if self.peek_keyword(Keyword::OPTIONS) {
+ Some(self.parse_options(Keyword::OPTIONS)?)
+ } else {
+ None
+ };
+ AlterSchemaOperation::AddReplica { replica, options }
+ } else if self.parse_keywords(&[Keyword::DROP, Keyword::REPLICA]) {
+ let replica = self.parse_identifier()?;
+ AlterSchemaOperation::DropReplica { replica }
+ } else {
+ return self.expected_ref(
+ "{SET OPTIONS | SET DEFAULT COLLATE | ADD REPLICA | DROP
REPLICA}",
Review Comment:
```suggestion
"ALTER SCHEMA operation",
```
Thinking something generic, in order to avoid an maintaining a growing list
that may potentially go out of sync
##########
src/parser/mod.rs:
##########
@@ -9241,6 +9243,40 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_alter_schema(&mut self) -> Result<Statement, ParserError> {
Review Comment:
We can call prev_token before invoking this method, so that it is self
contained and able to parse a full `ALTER SCHEMA` statement. Also can we add
some documentation for this function, ideally mentioning that it returns a
`Statement::AlterSchema` variant
--
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
