Skip to content

Support Struct datatype parsing for GenericDialect #1241

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 30, 2024
Merged

Support Struct datatype parsing for GenericDialect #1241

merged 4 commits into from
Apr 30, 2024

Conversation

duongcongtoai
Copy link
Contributor

Fix #1240

duongcongtoai
duongcongtoai commented Apr 28, 2024
View reviewed changes
tests/sqlparser_bigquery.rs
@@ -350,7 +350,7 @@ fn parse_create_table_with_options() {
#[test]
fn parse_nested_data_types() {
let sql = "CREATE TABLE table (x STRUCT<a ARRAY<INT64>, b BYTES(42)>, y ARRAY<STRUCT<INT64>>)";
match bigquery().one_statement_parses_to(sql, sql) {
match bigquery_and_generic().one_statement_parses_to(sql, sql) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also test for generic dialect

duongcongtoai
duongcongtoai commented Apr 28, 2024
View reviewed changes
tests/sqlparser_bigquery.rs
@@ -632,12 +638,12 @@ fn parse_typed_struct_syntax() {
expr_from_projection(&select.projection[1])
);

let sql = r#"SELECT STRUCT<DATE>("2011-05-05"), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
let select = bigquery().verified_only_select(sql);
let sql = r#"SELECT STRUCT<DATE>('2011-05-05'), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
Copy link
Contributor Author

@duongcongtoai duongcongtoai Apr 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

change " to ', so it is also understandable by generic dialect

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than updating the existing test, can you please keep the existing test (that uses ") and add new coverage for bigquery_and_generic dialect (with the ')?

This is to avoid losing existing test coverage

Copy link
Contributor Author

@duongcongtoai duongcongtoai Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i added extra test, was thinking of using a scenario struct to reuse the query logic and just replacing " with ' in the middle before invoking the parser, but i guess a bit boilerplating is easier to read

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, thank you -- I know it is a lot of repetition but it is easier to understand I think. Thank you 🙏

alamb
alamb reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you for this contirbution @duongcongtoai -- the code in this PR looks great to me. I think we need to avoid changing the existing tests, but then this PR wll be good to go

tests/sqlparser_bigquery.rs
@@ -632,12 +638,12 @@ fn parse_typed_struct_syntax() {
expr_from_projection(&select.projection[1])
);

let sql = r#"SELECT STRUCT<DATE>("2011-05-05"), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
let select = bigquery().verified_only_select(sql);
let sql = r#"SELECT STRUCT<DATE>('2011-05-05'), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than updating the existing test, can you please keep the existing test (that uses ") and add new coverage for bigquery_and_generic dialect (with the ')?

This is to avoid losing existing test coverage

@coveralls
Copy link

coveralls commented Apr 30, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8900428855

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 235 of 265 (88.68%) changed or added relevant lines in 2 files are covered.
  • 1236 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.197%
Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_bigquery.rs 234 264 88.64%
Files with Coverage Reduction New Missed Lines %
src/dialect/bigquery.rs 1 80.0%
src/dialect/generic.rs 1 73.91%
src/ast/operator.rs 2 96.67%
src/ast/value.rs 9 86.73%
tests/sqlparser_snowflake.rs 14 98.06%
src/dialect/mod.rs 18 80.0%
tests/sqlparser_postgres.rs 20 98.06%
tests/sqlparser_mssql.rs 29 92.35%
src/ast/query.rs 80 83.9%
src/ast/mod.rs 334 80.62%
Totals Coverage Status
Change from base Build 8859297496: -0.02%
Covered Lines: 24069
Relevant Lines: 26984
💛 - Coveralls

alamb
alamb approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @duongcongtoai -- looks great to me

tests/sqlparser_bigquery.rs
@@ -632,12 +638,12 @@ fn parse_typed_struct_syntax() {
expr_from_projection(&select.projection[1])
);

let sql = r#"SELECT STRUCT<DATE>("2011-05-05"), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
let select = bigquery().verified_only_select(sql);
let sql = r#"SELECT STRUCT<DATE>('2011-05-05'), STRUCT<DATETIME>(DATETIME '1999-01-01 01:23:34.45'), STRUCT<FLOAT64>(5.0), STRUCT<INT64>(1)"#;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, thank you -- I know it is a lot of repetition but it is easier to understand I think. Thank you 🙏

@alamb alamb merged commit bafaa91 into apache:main Apr 30, 2024
10 checks passed
JichaoS pushed a commit to luabase/sqlparser-rs that referenced this pull request May 7, 2024
@duongcongtoai @JichaoS
Support Struct datatype parsing for GenericDialect (apache#1241)
edf8f0b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@universalmind303 universalmind303 universalmind303 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Struct datatype parsing for GenericDialect
4 participants
@duongcongtoai @coveralls @alamb @universalmind303