Databricks: support INSERT BY NAME - #2403
Conversation
| /// Returns true if this dialect supports `INSERT INTO ... BY NAME ...`. | ||
| /// | ||
| /// Databricks: <https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-syntax-dml-insert-into> | ||
| fn supports_insert_by_name(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
unless the syntax conflicts with other dialects (doesn't seem to be the case?) I think we can skip the dialect method and let the parser be permissive?
| by_name = columns.is_empty() | ||
| && self.dialect.supports_insert_by_name() |
There was a problem hiding this comment.
| by_name = columns.is_empty() | |
| && self.dialect.supports_insert_by_name() | |
| by_name = by_name.is_none() | |
| && self.dialect.supports_insert_by_name() |
I think we can drop the columns check? its not clear why it would be needed at this layer, if the implication is something semantic then that can be left up to the consumer to validate. Relatedly, I assume we need to only set the name if we don't already have one (can we cover that scenario in the tests)?
|
Hi @iffyio , I have committed some changes as per your suggestions. Could you plz take a look again? |
| // `BY NAME` is an INSERT clause, not a table alias. | ||
| let table_alias = if self.dialect.supports_insert_table_alias() | ||
| && !self.peek_keywords(&[Keyword::BY, Keyword::NAME]) |
There was a problem hiding this comment.
oh this pattern looks a bit odd, would it make more sense to do the following?
let mut by_name = self.peek_keywords(&[BY, NAME]);
let table_alias = if ... // unchanged
// ...
let partitioned = self.parse_insert_partition()?;
by_name = by_name || self.parse_keywords(&[Keyword::BY, Keyword::NAME]);|
|
||
| #[test] | ||
| fn parse_insert_by_name_in_all_dialects() { | ||
| verified_stmt("INSERT INTO target BY NAME SELECT 1 AS a"); |
There was a problem hiding this comment.
maybe we can also add some scenarios with table options that cover around the new option to ensure that parsing BY NAME isn't conflicting with other features that show up around it
|
|
||
| #[test] | ||
| fn test_databricks_insert_by_name() { | ||
| match databricks_and_generic().verified_stmt("INSERT INTO target BY NAME SELECT 1 AS a") { |
There was a problem hiding this comment.
we can remove the AST assertion and rely on verified_stmt only for these, (AST is already covered in common)
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Summary
This PR adds support for Databricks INSERT ... BY NAME syntax.
Examples
Testing
cargo test --test sqlparser_databricks test_databricks_insert_by_name
Relevant PostgreSQL INSERT regression tests