Skip to content

Require space after -- to start single line comment in MySQL #1705

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

Merged
merged 3 commits into from
Feb 5, 2025
Merged

Require space after -- to start single line comment in MySQL #1705

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eba94c
Require space after -- to start single line comment in MySQL
hansott Feb 4, 2025
2b4b6ce
Refactor and improve tests
hansott Feb 4, 2025
4a90098
Feedback
hansott Feb 5, 2025
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
9 changes: 9 additions & 0 deletions src/dialect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,15 @@ pub trait Dialect: Debug + Any {
fn supports_table_hints(&self) -> bool {
false
}

/// Returns whether it's the start of a single line comment
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns whether it's the start of a single line comment
/// Returns true if this dialect requires a whitespace after `--` for single line comments.

/// e.g. MySQL requires a space after `--` to be a single line comment
/// Otherwise it's interpreted as a double minus operator
///
/// MySQL: <https://dev.mysql.com/doc/refman/8.4/en/ansi-diff-comments.html>
fn requires_whitespace_to_start_comment(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn requires_whitespace_to_start_comment(&self) -> bool {
fn requires_single_line_comment_whitespace(&self) -> bool {

thinking something like this to flag that this is only for the -- comment style

false
}
}

/// This represents the operators for which precedence must be defined
Expand Down
9 changes: 9 additions & 0 deletions src/dialect/mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ impl Dialect for MySqlDialect {
fn supports_table_hints(&self) -> bool {
true
}

/// Returns whether it's the start of a single line comment
/// e.g. MySQL requires a space after `--` to be a single line comment
/// Otherwise it's interpreted as a double minus operator
///
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is already duplicated at the definition of the dialect method, we could consider not including it (The link to the doc on the other hand is useful in any case)

/// MySQL: <https://dev.mysql.com/doc/refman/8.4/en/ansi-diff-comments.html>
fn requires_whitespace_to_start_comment(&self) -> bool {
true
}
}

/// `LOCK TABLES`
Expand Down
101 changes: 96 additions & 5 deletions src/tokenizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,15 +1229,31 @@ impl<'a> Tokenizer<'a> {
// operators
'-' => {
chars.next(); // consume the '-'
match chars.peek() {
Some('-') => {
chars.next(); // consume the second '-', starting a single-line comment

if let Some('-') = chars.peek() {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to have this remain as a match statement, calling chars.peek() only once? i.e.

match chars.peek() {
    Some('-') => { ... }
    Some('>') => { ... }
 }

let mut is_comment = true;
if self.dialect.requires_whitespace_to_start_comment() {
// MySQL requires a space after the -- for a single-line comment
// Otherwise it's interpreted as two minus signs
// e.g. UPDATE account SET balance=balance--1
// WHERE account_id=5752;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the example to the doc string in the dialect method? That would help clarify the behavior to folks without having to dig deeper in the docs or the parser code.
Then for the rest of the comment here we can probably skip since we've already documented as much in the dialect method

match chars.peekable.clone().nth(1) {
Some(' ') => (),
_ => is_comment = false,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
match chars.peekable.clone().nth(1) {
Some(' ') => (),
_ => is_comment = false,
}
is_comment = Some(' ') == chars.peekable.clone().nth(1);

looks like this can be simplified?

}

if is_comment {
chars.next(); // consume second '-'
let comment = self.tokenize_single_line_comment(chars);
Ok(Some(Token::Whitespace(Whitespace::SingleLineComment {
return Ok(Some(Token::Whitespace(Whitespace::SingleLineComment {
prefix: "--".to_owned(),
comment,
})))
})));
}
}

match chars.peek() {
Some('>') => {
chars.next();
match chars.peek() {
Expand Down Expand Up @@ -3685,4 +3701,79 @@ mod tests {
],
);
}

#[test]
fn test_whitespace_required_after_single_line_comment() {
all_dialects_where(|dialect| dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT --'abc'",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Minus,
Token::Minus,
Token::SingleQuotedString("abc".to_string()),
],
);

all_dialects_where(|dialect| dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT -- 'abc'",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Whitespace(Whitespace::SingleLineComment {
prefix: "--".to_string(),
comment: " 'abc'".to_string(),
}),
],
);

all_dialects_where(|dialect| dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT --",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Minus,
Token::Minus,
],
);
}

#[test]
fn test_whitespace_not_required_after_single_line_comment() {
all_dialects_where(|dialect| !dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT --'abc'",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Whitespace(Whitespace::SingleLineComment {
prefix: "--".to_string(),
comment: "'abc'".to_string(),
}),
],
);

all_dialects_where(|dialect| !dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT -- 'abc'",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Whitespace(Whitespace::SingleLineComment {
prefix: "--".to_string(),
comment: " 'abc'".to_string(),
}),
],
);

all_dialects_where(|dialect| !dialect.requires_whitespace_to_start_comment()).tokenizes_to(
"SELECT --",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Whitespace(Whitespace::SingleLineComment {
prefix: "--".to_string(),
comment: "".to_string(),
}),
],
);
}
}
59 changes: 59 additions & 0 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3244,3 +3244,62 @@ fn parse_double_precision() {
"CREATE TABLE foo (bar DOUBLE(11,0))",
);
}

#[test]
fn parse_looks_like_single_line_comment() {
// See https://dev.mysql.com/doc/refman/8.4/en/ansi-diff-comments.html
match mysql().parse_sql_statements(
r#"
UPDATE account SET balance=balance--1
WHERE account_id=5752
"#,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use mysql().verified_stmt() for the tests?

) {
Ok(statement) => match statement.first() {
Some(Statement::Update { assignments, .. }) => {
assert_eq!(assignments.len(), 1);
assert_eq!(
assignments[0],
Assignment {
value: Expr::BinaryOp {
left: Box::new(Expr::Identifier(Ident::new("balance"))),
op: BinaryOperator::Minus,
right: Box::new(Expr::UnaryOp {
op: UnaryOperator::Minus,
expr: Box::new(Expr::Value(number("1")))
}),
},
target: AssignmentTarget::ColumnName(ObjectName::from(vec![Ident::new(
"balance".to_string()
)])),
}
);
}
_ => panic!("expected update statement"),
},
_ => panic!("expected error"),
}

match mysql().parse_sql_statements(
r#"
UPDATE account SET balance=balance-- 1
WHERE account_id=5752
"#,
) {
Ok(statement) => match statement.first() {
Some(Statement::Update { assignments, .. }) => {
assert_eq!(assignments.len(), 1);
assert_eq!(
assignments[0],
Assignment {
value: Expr::Identifier(Ident::new("balance")),
target: AssignmentTarget::ColumnName(ObjectName::from(vec![Ident::new(
"balance".to_string()
)])),
}
);
}
_ => panic!("expected update statement"),
},
_ => panic!("expected error"),
}
}