iffyio commented on code in PR #2326:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2326#discussion_r3159441464
##########
src/ast/helpers/stmt_create_table.rs:
##########
@@ -157,6 +157,9 @@ pub struct CreateTableBuilder {
pub base_location: Option<String>,
/// Optional external volume identifier.
pub external_volume: Option<String>,
+ /// BigQuery `WITH CONNECTION` clause for external tables.
+ ///
<https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#create_external_table_statement>
Review Comment:
```suggestion
/// `WITH CONNECTION` clause.
///
[BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#create_external_table_statement)
```
##########
src/ast/helpers/stmt_create_table.rs:
##########
@@ -488,6 +492,11 @@ impl CreateTableBuilder {
self.external_volume = external_volume;
self
}
+ /// Set the BigQuery `WITH CONNECTION` clause for external tables.
Review Comment:
```suggestion
/// Set the `WITH CONNECTION` clause.
```
##########
src/ast/ddl.rs:
##########
@@ -3021,6 +3021,9 @@ pub struct CreateTable {
/// Snowflake "EXTERNAL_VOLUME" clause for Iceberg tables
/// <https://docs.snowflake.com/en/sql-reference/sql/create-iceberg-table>
pub external_volume: Option<String>,
+ /// BigQuery `WITH CONNECTION` clause for external tables
+ ///
<https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#create_external_table_statement>
Review Comment:
```suggestion
/// `WITH CONNECTION` clause.
///
[BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-definition-language#create_external_table_statement)
```
##########
src/parser/mod.rs:
##########
@@ -6416,8 +6416,28 @@ impl<'a> Parser<'a> {
None
};
let location = hive_formats.as_ref().and_then(|hf|
hf.location.clone());
- let table_properties = self.parse_options(Keyword::TBLPROPERTIES)?;
- let table_options = if !table_properties.is_empty() {
+
+ // BigQuery external tables support `WITH CONNECTION <name>` and
`OPTIONS(...)`
+ // clauses after the (optional) column list.
+ let with_connection = if self.parse_keywords(&[Keyword::WITH,
Keyword::CONNECTION]) {
+ Some(self.parse_object_name(false)?)
+ } else {
+ None
+ };
+ // BigQuery uses OPTIONS(...); Hive uses TBLPROPERTIES(...). They are
+ // mutually exclusive in practice, and `parse_options` returns an empty
+ // vec when the keyword isn't present, so trying OPTIONS first and
+ // falling back to TBLPROPERTIES preserves the existing Hive path
+ // without accepting both in the same statement.
+ let options = self.parse_options(Keyword::OPTIONS)?;
Review Comment:
not sure I follow the goal here, is `OPTIONS` part of the `WITH CONNECTION`
clause (if not isn't it already parsed elsewhere for bigquery today)?
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2203,6 +2203,90 @@ fn parse_big_query_declare() {
);
}
+#[test]
+fn parse_bigquery_create_external_table_with_connection_and_options() {
+ let sql = concat!(
+ "CREATE OR REPLACE EXTERNAL TABLE `proj.ds.tbl` ",
+ "WITH CONNECTION `projects/proj/locations/us/connections/c` ",
+ r#"OPTIONS(format = "ICEBERG", uris = ["gs://b/m.json"])"#,
+ );
+ let stmts = bigquery().parse_sql_statements(sql).unwrap();
+ assert_eq!(stmts.len(), 1);
+ let Statement::CreateTable(ct) = &stmts[0] else {
+ panic!("expected CreateTable, got {:?}", stmts[0]);
+ };
+ assert!(ct.external);
+ assert!(ct.or_replace);
+ assert!(ct.columns.is_empty());
+ let with_connection = ct.with_connection.as_ref().expect("with_connection
set");
+ assert_eq!(
+ with_connection.to_string(),
+ "`projects/proj/locations/us/connections/c`"
+ );
+ let CreateTableOptions::Options(opts) = &ct.table_options else {
+ panic!("expected OPTIONS, got {:?}", ct.table_options);
+ };
+ assert_eq!(opts.len(), 2);
+}
+
+#[test]
+fn parse_bigquery_create_external_table_with_connection_variants() {
+ // WITH CONNECTION alone — no OPTIONS, no explicit column list.
+ // Display normalizes the bare form by adding an empty column list, so
+ // use `one_statement_parses_to` to assert the normalized output.
+ let stmt = bigquery().one_statement_parses_to(
+ "CREATE EXTERNAL TABLE t WITH CONNECTION c",
+ "CREATE EXTERNAL TABLE t () WITH CONNECTION c",
+ );
+ let Statement::CreateTable(ct) = stmt else {
+ panic!("expected CreateTable");
+ };
+ assert!(ct.external);
+ assert!(!ct.or_replace);
+ assert!(ct.columns.is_empty());
+ assert_eq!(
+ ct.with_connection
+ .as_ref()
+ .expect("with_connection set")
+ .to_string(),
+ "c"
+ );
+ assert!(matches!(ct.table_options, CreateTableOptions::None));
+
+ // With explicit columns + OPTIONS. Exercises the Display round-trip of
+ // the `(columns) WITH CONNECTION <name> OPTIONS(...)` ordering so a
+ // future refactor that reshuffles clause order will fail this test.
+ let sql = concat!(
+ "CREATE EXTERNAL TABLE t (a INT64, b STRING) ",
+ r#"WITH CONNECTION c OPTIONS(uris = ["gs://x"])"#,
+ );
+ let stmt = bigquery().verified_stmt(sql);
+ let Statement::CreateTable(ct) = stmt else {
+ panic!("expected CreateTable");
+ };
+ assert_eq!(ct.columns.len(), 2);
+ assert_eq!(
+ ct.with_connection
+ .as_ref()
+ .expect("with_connection set")
+ .to_string(),
+ "c"
+ );
+ let CreateTableOptions::Options(opts) = &ct.table_options else {
+ panic!("expected OPTIONS, got {:?}", ct.table_options);
+ };
+ assert_eq!(opts.len(), 1);
+
+ // Baseline: no WITH CONNECTION. The new parser branch must not produce
Review Comment:
I think we can remove these superfluous comments from the tests
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2203,6 +2203,90 @@ fn parse_big_query_declare() {
);
}
+#[test]
+fn parse_bigquery_create_external_table_with_connection_and_options() {
+ let sql = concat!(
+ "CREATE OR REPLACE EXTERNAL TABLE `proj.ds.tbl` ",
+ "WITH CONNECTION `projects/proj/locations/us/connections/c` ",
+ r#"OPTIONS(format = "ICEBERG", uris = ["gs://b/m.json"])"#,
+ );
+ let stmts = bigquery().parse_sql_statements(sql).unwrap();
Review Comment:
for the tests is there a reason we don't use `verified_stmt`? Also I think
we can skip the assertions on the ast and have it solely as roundtrip tests (I
don't think the ast assertions are adding much given the changes made and
preexisting tests)
##########
src/parser/mod.rs:
##########
@@ -6416,8 +6416,28 @@ impl<'a> Parser<'a> {
None
};
let location = hive_formats.as_ref().and_then(|hf|
hf.location.clone());
- let table_properties = self.parse_options(Keyword::TBLPROPERTIES)?;
- let table_options = if !table_properties.is_empty() {
+
+ // BigQuery external tables support `WITH CONNECTION <name>` and
`OPTIONS(...)`
+ // clauses after the (optional) column list.
Review Comment:
```suggestion
```
--
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]
