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.

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

Support "with" identifiers surrounded by backticks in GenericDialect #1010

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Support "with" identifiers surrounded by backticks in GenericDialect #1010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dialect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub trait Dialect: Debug + Any {
/// MySQL, MS SQL, and sqlite). You can accept one of characters listed
/// in `Word::matching_end_quote` here
fn is_delimited_identifier_start(&self, ch: char) -> bool {
ch == '"'
ch == '"' || ch == '`'
}
/// Determine if quoted characters are proper for identifier
fn is_proper_identifier_inside_quotes(&self, mut _chars: Peekable<Chars<'_>>) -> bool {
Expand Down
23 changes: 16 additions & 7 deletions tests/sqlparser_hive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use sqlparser::ast::{
SelectItem, Statement, TableFactor, UnaryOperator, Value,
};
use sqlparser::dialect::{GenericDialect, HiveDialect};
use sqlparser::parser::ParserError;
use sqlparser::parser::{ParserError, ParserOptions};
use sqlparser::test_utils::*;

#[test]
Expand All @@ -32,6 +32,20 @@ fn parse_table_create() {
hive().verified_stmt(iof);
}

fn generic(options: Option<ParserOptions>) -> TestedDialects {
TestedDialects {
dialects: vec![Box::new(GenericDialect {})],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also please test HiveDialect given this is in the sqlparser_hive.rs module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, good point. Thank you.

Copy link
Contributor Author

@bitemyapp bitemyapp Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't find any test-cases that tested generic and not hive. Everything was either hive alone or hive and generic, e.g. the parse_create_function test. I'm not sure it makes sense to blend them given how they get used in the tests. I think it makes sense to test hive-isms that generic should be able to handle in the module that tests hive-isms rather than duplicating everything into the generic test module.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like this in sqlparser_common.rs?

https://github.com/sqlparser-rs/sqlparser-rs/blob/ce62fe6d274d354fef34fad919b58f6ba16c61a3/tests/sqlparser_common.rs#L7303-L7316

(maybe we can do this as a follow on PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind doing a follow-up PR.

Perhaps I could do something like parse_deeply_nested_expr_hits_recursion_limits in the common module but parameterized over the dialect, then invoke the helper in the generic & hive test modules? If that sounds acceptable let me know and I will pursue that.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was suggesting was to remove generic() and test parse_describe with both hive and generic

options,
}
}

#[test]
fn parse_describe() {
let describe = r#"DESCRIBE namespace.`table`"#;
hive().verified_stmt(describe);
generic(None).verified_stmt(describe);
}

#[test]
fn parse_insert_overwrite() {
let insert_partitions = r#"INSERT OVERWRITE TABLE db.new_table PARTITION (a = '1', b) SELECT a, b, c FROM db.table"#;
Expand Down Expand Up @@ -265,13 +279,8 @@ fn parse_create_function() {
_ => unreachable!(),
}

let generic = TestedDialects {
dialects: vec![Box::new(GenericDialect {})],
options: None,
};

assert_eq!(
generic.parse_sql_statements(sql).unwrap_err(),
generic(None).parse_sql_statements(sql).unwrap_err(),
ParserError::ParserError(
"Expected an object type after CREATE, found: FUNCTION".to_string()
)
Expand Down
Loading