-
Notifications
You must be signed in to change notification settings - Fork 713
BigQuery: Parse WITH CONNECTION on CREATE EXTERNAL TABLE #2326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
4df2961
b9b1edc
85c9353
1ae133b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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> | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
| pub with_connection: Option<ObjectName>, | ||||||||||
| /// Optional catalog name. | ||||||||||
| pub catalog: Option<String>, | ||||||||||
| /// Optional catalog synchronization option. | ||||||||||
|
|
@@ -235,6 +238,7 @@ impl CreateTableBuilder { | |||||||||
| with_tags: None, | ||||||||||
| base_location: None, | ||||||||||
| external_volume: None, | ||||||||||
| with_connection: None, | ||||||||||
| catalog: None, | ||||||||||
| catalog_sync: None, | ||||||||||
| storage_serialization_policy: None, | ||||||||||
|
|
@@ -488,6 +492,11 @@ impl CreateTableBuilder { | |||||||||
| self.external_volume = external_volume; | ||||||||||
| self | ||||||||||
| } | ||||||||||
| /// Set the BigQuery `WITH CONNECTION` clause for external tables. | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
| pub fn with_connection(mut self, with_connection: Option<ObjectName>) -> Self { | ||||||||||
| self.with_connection = with_connection; | ||||||||||
| self | ||||||||||
| } | ||||||||||
| /// Set the catalog name for the table. | ||||||||||
| pub fn catalog(mut self, catalog: Option<String>) -> Self { | ||||||||||
| self.catalog = catalog; | ||||||||||
|
|
@@ -605,6 +614,7 @@ impl CreateTableBuilder { | |||||||||
| with_tags: self.with_tags, | ||||||||||
| base_location: self.base_location, | ||||||||||
| external_volume: self.external_volume, | ||||||||||
| with_connection: self.with_connection, | ||||||||||
| catalog: self.catalog, | ||||||||||
| catalog_sync: self.catalog_sync, | ||||||||||
| storage_serialization_policy: self.storage_serialization_policy, | ||||||||||
|
|
@@ -686,6 +696,7 @@ impl From<CreateTable> for CreateTableBuilder { | |||||||||
| with_tags: table.with_tags, | ||||||||||
| base_location: table.base_location, | ||||||||||
| external_volume: table.external_volume, | ||||||||||
| with_connection: table.with_connection, | ||||||||||
| catalog: table.catalog, | ||||||||||
| catalog_sync: table.catalog_sync, | ||||||||||
| storage_serialization_policy: table.storage_serialization_policy, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||
| 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)?; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I follow the goal here, is
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, reverted. The parser change is now just the new |
||||||
| let table_properties = if options.is_empty() { | ||||||
| self.parse_options(Keyword::TBLPROPERTIES)? | ||||||
| } else { | ||||||
| vec![] | ||||||
| }; | ||||||
| let table_options = if !options.is_empty() { | ||||||
| CreateTableOptions::Options(options) | ||||||
| } else if !table_properties.is_empty() { | ||||||
| CreateTableOptions::TableProperties(table_properties) | ||||||
| } else if let Some(options) = self.maybe_parse_options(Keyword::OPTIONS)? { | ||||||
| CreateTableOptions::Options(options) | ||||||
|
|
@@ -6430,6 +6450,7 @@ impl<'a> Parser<'a> { | |||||
| .hive_distribution(hive_distribution) | ||||||
| .hive_formats(hive_formats) | ||||||
| .table_options(table_options) | ||||||
| .with_connection(with_connection) | ||||||
| .or_replace(or_replace) | ||||||
| .if_not_exists(if_not_exists) | ||||||
| .external(true) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the tests is there a reason we don't use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove these superfluous comments from the tests
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| // a spurious with_connection when the keyword pair is absent. | ||
| let stmt = | ||
| bigquery().verified_stmt("CREATE EXTERNAL TABLE t (a INT64) OPTIONS(uris = [\"gs://x\"])"); | ||
| let Statement::CreateTable(ct) = stmt else { | ||
| panic!("expected CreateTable"); | ||
| }; | ||
| assert!(ct.with_connection.is_none()); | ||
| } | ||
|
|
||
| fn bigquery() -> TestedDialects { | ||
| TestedDialects::new(vec![Box::new(BigQueryDialect {})]) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.