Skip to content

Databricks: support INSERT BY NAME - #2403

Open
finchxxia wants to merge 5 commits into
apache:mainfrom
finchxxia:dbx-insert-by-name
Open

Databricks: support INSERT BY NAME#2403
finchxxia wants to merge 5 commits into
apache:mainfrom
finchxxia:dbx-insert-by-name

Conversation

@finchxxia

Copy link
Copy Markdown
Contributor

Summary

This PR adds support for Databricks INSERT ... BY NAME syntax.

  • Adds a by_name field to the Insert AST node.
  • Parses BY NAME for the Databricks and Generic dialects.
  • Preserves BY NAME when formatting the AST back to SQL.
  • Adds test coverage for INSERT INTO ... BY NAME with both SELECT and WITH queries.
  • Updates existing Insert AST construction sites with the default by_name: false.

Examples

INSERT INTO target BY NAME
SELECT 1 AS a;
INSERT INTO TABLE target BY NAME
WITH source AS (
    SELECT 1 AS event_data_id
)
SELECT event_data_id FROM source;

Testing

cargo test --test sqlparser_databricks test_databricks_insert_by_name
Relevant PostgreSQL INSERT regression tests

Comment thread src/dialect/mod.rs Outdated
Comment on lines +1356 to +1361
/// 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
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread src/parser/mod.rs Outdated
Comment on lines +18352 to +18353
by_name = columns.is_empty()
&& self.dialect.supports_insert_by_name()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)?

@finchxxia

Copy link
Copy Markdown
Contributor Author

Hi @iffyio , I have committed some changes as per your suggestions. Could you plz take a look again?

Comment thread src/parser/mod.rs
Comment on lines +18407 to +18409
// `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])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

Comment thread tests/sqlparser_common.rs Outdated
Comment thread tests/sqlparser_common.rs

#[test]
fn parse_insert_by_name_in_all_dialects() {
verified_stmt("INSERT INTO target BY NAME SELECT 1 AS a");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants