Skip to content

Conversation

git-hulk
Copy link
Member

@git-hulk git-hulk commented Jun 20, 2025

Before applying this patch, the following SQL cannot be parsed:

CREATE TABLE my_table (
  f0 STRING,
  f1 STRUCT<a STRING, b STRUCT<c INT64, d STRING>>,
  f2 STRING,
)

But it's a valid SQL in BigQuery. The root cause is that having a comma after the trailing bracket will be recognized as a mismatched closing bracket.

@git-hulk
Copy link
Member Author

git-hulk commented Jun 20, 2025

@iffyio Could you please take a look at this fix? The root cause is that it might have more field definitions after the trailing bracket, but now always regarded as an unmatched bracket condition.

The code block is somewhat complex to fix due to its recursive behavior, but I have added more cases for it, and it works well.

Before applying this patch, the following SQL cannot be parsed:

```SQL
CREATE TABLE my_table (
  f0 STRING,
  f1 STRUCT<a STRING, b STRUCT<c INT64, d STRING>>,
  f2 STRING,
)
```

But it's a valid SQL in BigQuery. The root cause is having comma after
the trailing bracket will be reconigized as a mismatched closing.
@git-hulk git-hulk force-pushed the fix/error-in-trailling-bracket branch from 758c307 to b66a377 Compare June 20, 2025 11:16
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Thanks for fixing @git-hulk! The fix looks good to me, left a minor comment to merge the test

}

#[test]
fn test_struct_trailing_bracket() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we merge this test with this one? since they're covering the same behavior. I imagine we could just move the test cases in the latter function into this new function and should be good?

Copy link
Member Author

Choose a reason for hiding this comment

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

@iffyio Thanks for your suggestion. I didn't notice that test case, and have moved it into a dedicated one.

@git-hulk git-hulk requested a review from iffyio June 20, 2025 13:19
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @git-hulk!
cc @alamb

@iffyio iffyio merged commit 185a490 into apache:main Jun 20, 2025
10 checks passed
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