Skip to content
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.

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

Restrain JSON_TABLE table function parsing to MySqlDialect and AnsiDialect #1123

Closed
wants to merge 19 commits into from
Closed
2 changes: 1 addition & 1 deletion src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7503,10 +7503,10 @@
Ok(TableFactor::UNNEST {
alias,
array_exprs,
with_offset,

Check failure on line 7506 in src/parser/mod.rs

View workflow job for this annotation

GitHub Actions / codestyle

Diff in /home/runner/work/sqlparser-rs/sqlparser-rs/src/parser/mod.rs
with_offset_alias,
})
} else if self.parse_keyword(Keyword::JSON_TABLE) {
} else if dialect_of!(self is MySqlDialect | AnsiDialect) && self.parse_keyword(Keyword::JSON_TABLE) {
Copy link
Member Author

@viirya viirya Feb 8, 2024

Choose a reason for hiding this comment

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

Although AnsiDialect is for SQL:2011 standard based on its comment, seems it makes sense to add it in supported dialect.

Copy link
Member Author

@viirya viirya Feb 8, 2024

Choose a reason for hiding this comment

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

I think it's ok to accept json_table as a table name for specific dialects where it has been tested to be accepted unquoted, but the default behavior should probably be to parse it as the json_table function, not as a table name.

It could be the default behavior for dialects which are likely to follow the standard to support the table function.

Copy link
Member Author

Choose a reason for hiding this comment

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

As there are only very few dialects supporting this, I think it makes more sense to not add it into GenericDialect currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think the dialect identity should be hardcoded. This should probably be a dialect.supports_json_table. And it should be true in the 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.

I agree it would be nicer to do if self.dialect.supports_json_table() instead. However, I think we could also do this as a follow on PR as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why it should be true in generic dialect. GenericDialect is not strictly SQL standard dialect (AnsiDialect is for the purpose). It is for syntax that most dialects use. Currently the table function is only supported by very few dialects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it would be nicer to do if self.dialect.supports_json_table() instead. However, I think we could also do this as a follow on PR as well

Agreed. As I mentioned early, this can be discussed separately. Although I wonder if it will result in many similar functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I wonder if it will result in many similar functions.

Yes I think it would. The codebase has a mix of styles currently -- both dialect_of! and trait methods

self.expect_token(&Token::LParen)?;
let json_expr = self.parse_expr()?;
self.expect_token(&Token::Comma)?;
Expand Down
19 changes: 19 additions & 0 deletions tests/sqlparser_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8407,3 +8407,22 @@
p.parse_statements().unwrap();
let _ = p.into_tokens();
}

Check failure on line 8410 in tests/sqlparser_common.rs

View workflow job for this annotation

GitHub Actions / codestyle

Diff in /home/runner/work/sqlparser-rs/sqlparser-rs/tests/sqlparser_common.rs
#[test]
fn parse_json_table_err() {
let unsupported_dialects = all_dialects_except(|d| d.is::<AnsiDialect>() || d.is::<MySqlDialect>());

// JSON_TABLE table function is not supported in the above dialects.
assert!(unsupported_dialects
.parse_sql_statements("SELECT * FROM JSON_TABLE('[[1, 2], [3, 4]]', '$[*]' COLUMNS(a INT PATH '$[0]', b INT PATH '$[1]')) AS t")
.is_err());
}

Check failure on line 8420 in tests/sqlparser_common.rs

View workflow job for this annotation

GitHub Actions / codestyle

Diff in /home/runner/work/sqlparser-rs/sqlparser-rs/tests/sqlparser_common.rs
#[test]
fn parse_json_table_as_identifier() {
let supported_dialects = all_dialects_except(|d| d.is::<AnsiDialect>() || d.is::<MySqlDialect>());

assert!(supported_dialects
.parse_sql_statements("SELECT * FROM json_table")
.is_ok());
}
Loading