Support DuckDB struct syntax and support list of struct syntax #1372

jayzhan211 · 2024-08-09T06:06:14Z

Signed-off-by: jayzhan211 <[email protected]>

coveralls · 2024-08-09T06:10:23Z

Pull Request Test Coverage Report for Build 10404149988

Details

114 of 118 (96.61%) changed or added relevant lines in 4 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.143%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/sqlparser_bigquery.rs	33	34	97.06%
tests/sqlparser_duckdb.rs	59	60	98.33%
src/ast/data_type.rs	4	6	66.67%

Files with Coverage Reduction	New Missed Lines	%
src/ast/mod.rs	1	81.47%
tests/sqlparser_bigquery.rs	4	90.27%

Totals
Change from base Build 10390800057:	0.02%
Covered Lines:	28260
Relevant Lines:	31702

💛 - Coveralls

Signed-off-by: jayzhan211 <[email protected]>

git-hulk · 2024-08-09T06:27:01Z

src/parser/mod.rs

@@ -1074,10 +1074,14 @@ impl<'a> Parser<'a> {
                Keyword::MATCH if dialect_of!(self is MySqlDialect | GenericDialect) => {
                    self.parse_match_against()
                }
-                Keyword::STRUCT if dialect_of!(self is BigQueryDialect | GenericDialect) => {
+                Keyword::STRUCT if dialect_of!(self is BigQueryDialect | GenericDialect ) => {


Can you revert this line?

git-hulk · 2024-08-09T06:27:12Z

src/parser/mod.rs

                    self.prev_token();
                    self.parse_bigquery_struct_literal()
                }
+                Keyword::STRUCT if dialect_of!(self is DuckDbDialect ) => {


Suggested change

Keyword::STRUCT if dialect_of!(self is DuckDbDialect ) => {

Keyword::STRUCT if dialect_of!(self is DuckDbDialect) => {

git-hulk · 2024-08-09T06:42:11Z

src/parser/mod.rs

+            match self.peek_token().token {
+                Token::RParen => {
+                    self.next_token();
+                    false.into()
+                }
+                _ => return self.expected(")", self.peek_token()),
+            }


Suggested change

match self.peek_token().token {

Token::RParen => {

self.next_token();

false.into()

}

_ => return self.expected(")", self.peek_token()),

}

self.expect_token(&Token::RParen)?;

false.into()

git-hulk · 2024-08-09T06:42:37Z

src/parser/mod.rs

@@ -7229,10 +7274,18 @@ impl<'a> Parser<'a> {
                        ))))
                    }
                }
+                Keyword::STRUCT if dialect_of!(self is DuckDbDialect) => {
+                    self.prev_token();
+                    // let field_defs=


Suggested change

// let field_defs=

tests/sqlparser_duckdb.rs

git-hulk · 2024-08-09T06:43:43Z

cc @iffyio @alamb

Signed-off-by: jayzhan211 <[email protected]>

git-hulk

LGTM, great.

jayzhan211 · 2024-08-09T09:28:55Z

Thanks for your review @git-hulk

iffyio · 2024-08-11T04:27:29Z

src/parser/mod.rs

@@ -2227,7 +2228,15 @@ impl<'a> Parser<'a> {

        Ok((
            field_defs,
-            self.expect_closing_angle_bracket(trailing_bracket)?,
+            if token == Token::Lt {


There's a slight mismatch I'm thinking might not be worth having - the open delimeter token is configurable now but here the function still needs to figure out for itself what the value is for the closing token.
The function is a bit complicated on its own, specifically due to the < delimeter. then since duckdb syntax doesn't have that I'm thinking it makes sense to use a dedicated function that is simpler.

Something roughly like this I think should be all duckdb need, would it make sense to add this instead and have duckdb (and any future dialect sharing the same syntax) use it?

fn parse_duck_db_struct_type_def<F>( &mut self, ) -> Result<Vec<StructField>, ParserError> { let start_token = self.peek_token(); self.expect_keyword(Keyword::STRUCT)?; self.expect_token(Token::LParen)?; let field_defs = self.parse_comma_separated(Self::parse_struct_field_def)?; self.expect_token(Token::RParen)?; Ok(field_defs) }

Signed-off-by: jayzhan211 <[email protected]>

iffyio · 2024-08-14T04:27:59Z

tests/sqlparser_bigquery.rs

+#[test]
+fn test_struct() {
+    // nested struct
+    let canonical = r#"CREATE TABLE t1 (s STRUCT<v VARCHAR, s STRUCT<a1 INTEGER, a2 VARCHAR>>[])"#;
+    let sql = r#"CREATE TABLE t1 (s STRUCT<v VARCHAR, s STRUCT<a1 INTEGER, a2 VARCHAR>>[])"#;
+    let select = bigquery().parse_sql_statements(sql).unwrap().pop().unwrap();
+    // TODO: '>>' is incorrect parsed in bigquery syntax
+    assert_ne!(select.to_string(), canonical);
+}


not sure I follow if/why this test is needed, since bigquery doesn't support [] syntax for arrays?

iffyio · 2024-08-14T04:34:11Z

src/parser/mod.rs

+        let field_defs = self
+            .parse_comma_separated(Self::parse_struct_field_def)?
+            .into_iter()
+            .map(|(f, _)| f)


Oh this looks like a bug for us to silently drop the returned flag since there might be an extraneous >, seems like we would instead do something like this

let field_defs = self.parse_comma_separated(|self| { Ok(StructField { field_name: Some(self.parse_identifier(false)?), // I assume field name isn't optional for duckdb? field_type: self.parse_data_type(), })

src/parser/mod.rs

iffyio · 2024-08-14T04:37:34Z

tests/sqlparser_duckdb.rs

+    let canonical = r#"CREATE TABLE t1 (s STRUCT<v VARCHAR, i INTEGER>)"#;
+    let sql = r#"CREATE TABLE t1 (s STRUCT(v VARCHAR, i INTEGER))"#;


Oh I think this is incorrect, no need for the input and output to not match, in this case the output is significantly different syntax from what went into the parser. We can probably add a parens_delimeter: bool or similar flag to the StructField struct so that it knows how to display accordingly?

It is not clear to me. Do you mean adding flag so we have different canonical name for struct<> and struct()?

Ah yeah that's what I meant - my bad realised I wrote comma_delimeter but meant to write parens_delimeter instead. Yeah so like if that flag is true display uses struct (...) otherwise struct<...>

alamb · 2024-08-14T15:44:03Z

I am hoping to make a sqlparser release soon (like tomorrow) -- do we think this PR is close to ready (aka should I hold the release for this PR?)

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-08-15T00:43:47Z

I am hoping to make a sqlparser release soon (like tomorrow) -- do we think this PR is close to ready (aka should I hold the release for this PR?)

It is close. We can release together with this PR

Signed-off-by: jayzhan211 <[email protected]>

src/parser/mod.rs

tests/sqlparser_duckdb.rs

Signed-off-by: jayzhan211 <[email protected]>

iffyio

One minor cleanup comment, otherwise LGTM! cc @alamb

tests/sqlparser_duckdb.rs

Co-authored-by: Ifeanyi Ubah <[email protected]>

…to duckdb-struct

alamb · 2024-08-15T12:49:33Z

I am working on updating the tests and adding some small comments. I'll be done shortly

alamb · 2024-08-15T12:58:21Z

tests/sqlparser_duckdb.rs

@@ -32,6 +32,118 @@ fn duckdb_and_generic() -> TestedDialects {
    }
 }

+#[test]
+fn test_struct() {
+    // s STRUCT(v VARCHAR, i INTEGER)


I updated the tests to also have coverage of the actual parsed data types

alamb · 2024-08-15T12:58:42Z

src/ast/data_type.rs

@@ -618,6 +625,17 @@ fn format_clickhouse_datetime_precision_and_timezone(
    Ok(())
 }

+/// Type of brackets used for `STRUCT` literals.


I also added some comments on this struct

alamb · 2024-08-15T13:02:19Z

Thanks again @jayzhan211 and @iffyio and @git-hulk

jayzhan211 · 2024-08-15T13:13:06Z

Great! Thanks @iffyio @alamb and @git-hulk

enable duckdb struct

6b7ac54

Signed-off-by: jayzhan211 <[email protected]>

fmt

ef3da33

Signed-off-by: jayzhan211 <[email protected]>

git-hulk reviewed Aug 9, 2024

View reviewed changes

add test

d26444d

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 changed the title ~~Support DuckDB struct symtax~~ Support DuckDB struct syntax and support list of struct syntax Aug 9, 2024

add failing test and simplify code

93d9245

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 mentioned this pull request Aug 9, 2024

Add the ability to create tables with deeply nested schemas in SQL apache/datafusion#11746

Open

git-hulk approved these changes Aug 9, 2024

View reviewed changes

iffyio reviewed Aug 11, 2024

View reviewed changes

jayzhan211 added 2 commits August 12, 2024 22:21

simplifed duckdb struct parser

ea04755

Signed-off-by: jayzhan211 <[email protected]>

simplified old func

3dca179

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 requested review from iffyio and git-hulk August 12, 2024 14:24

git-hulk approved these changes Aug 12, 2024

View reviewed changes

iffyio reviewed Aug 14, 2024

View reviewed changes

bracket kind

fe715d7

Signed-off-by: jayzhan211 <[email protected]>

rename

4638137

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 requested a review from iffyio August 15, 2024 00:46

iffyio reviewed Aug 15, 2024

View reviewed changes

src/parser/mod.rs Outdated Show resolved Hide resolved

iffyio reviewed Aug 15, 2024

View reviewed changes

tests/sqlparser_duckdb.rs Show resolved Hide resolved

add more test and return error

96e3164

Signed-off-by: jayzhan211 <[email protected]>

iffyio approved these changes Aug 15, 2024

View reviewed changes

tests/sqlparser_duckdb.rs Outdated Show resolved Hide resolved

alamb and others added 3 commits August 15, 2024 08:39

Update tests/sqlparser_duckdb.rs

f3a6cd8

Co-authored-by: Ifeanyi Ubah <[email protected]>

Add docs for StructBrackedKind

6e25767

Merge branch 'duckdb-struct' of github.com:jayzhan211/sqlparser-rs in…

6947c15

…to duckdb-struct

Merge remote-tracking branch 'origin/main' into duckdb-struct

7bf5b68

Improve tests

d898230

alamb reviewed Aug 15, 2024

View reviewed changes

alamb approved these changes Aug 15, 2024

View reviewed changes

alamb merged commit 8c4d30b into apache:main Aug 15, 2024
10 checks passed

jayzhan211 deleted the duckdb-struct branch August 15, 2024 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DuckDB struct syntax and support list of struct syntax #1372

Support DuckDB struct syntax and support list of struct syntax #1372

jayzhan211 commented Aug 9, 2024

coveralls commented Aug 9, 2024 •

edited

Loading

git-hulk Aug 9, 2024

git-hulk Aug 9, 2024

git-hulk Aug 9, 2024

git-hulk Aug 9, 2024

git-hulk commented Aug 9, 2024 •

edited

Loading

git-hulk left a comment

jayzhan211 commented Aug 9, 2024

iffyio Aug 11, 2024

This comment was marked as outdated.

iffyio Aug 14, 2024

iffyio Aug 14, 2024

iffyio Aug 14, 2024 •

edited

Loading

jayzhan211 Aug 14, 2024 •

edited

Loading

iffyio Aug 14, 2024

alamb commented Aug 14, 2024

jayzhan211 commented Aug 15, 2024

iffyio left a comment

alamb commented Aug 15, 2024

alamb Aug 15, 2024

alamb Aug 15, 2024

alamb commented Aug 15, 2024

jayzhan211 commented Aug 15, 2024

	Keyword::STRUCT if dialect_of!(self is DuckDbDialect ) => {
	Keyword::STRUCT if dialect_of!(self is DuckDbDialect) => {

		let canonical = r#"CREATE TABLE t1 (s STRUCT<v VARCHAR, i INTEGER>)"#;
		let sql = r#"CREATE TABLE t1 (s STRUCT(v VARCHAR, i INTEGER))"#;

Support DuckDB struct syntax and support list of struct syntax #1372

Support DuckDB struct syntax and support list of struct syntax #1372

Conversation

jayzhan211 commented Aug 9, 2024

coveralls commented Aug 9, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10404149988

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

git-hulk commented Aug 9, 2024 • edited Loading

git-hulk left a comment

Choose a reason for hiding this comment

jayzhan211 commented Aug 9, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

jayzhan211 Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 14, 2024

jayzhan211 commented Aug 15, 2024

iffyio left a comment

Choose a reason for hiding this comment

alamb commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 15, 2024

jayzhan211 commented Aug 15, 2024

coveralls commented Aug 9, 2024 •

edited

Loading

git-hulk commented Aug 9, 2024 •

edited

Loading

iffyio Aug 14, 2024 •

edited

Loading

jayzhan211 Aug 14, 2024 •

edited

Loading