Skip to content

19927: fix: change token consumption to pick to test on EOF in parser #202

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

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

19927: fix: change token consumption to pick to test on EOF in parser #202

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
46 changes: 44 additions & 2 deletions datafusion/sql/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ impl<'a> DFParser<'a> {
}
}
} else {
let token = self.parser.next_token();
let token = self.parser.peek_token();
Copy link

@gemini-code-assist gemini-code-assist bot Jan 22, 2026

Choose a reason for hiding this comment

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

high

Changing next_token() to peek_token() here is a crucial correctness fix. The original implementation would consume the token regardless of whether it was EOF or SemiColon, potentially leading to incorrect parsing or error messages if the token was not one of those. peek_token() allows for inspection without consumption, which is the correct approach in this control flow.

Suggested change
let token = self.parser.peek_token();
let token = self.parser.peek_token();

Copy link
Owner Author

@martin-augment martin-augment Jan 22, 2026

Choose a reason for hiding this comment

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

value:annoying; category:bug; feedback:The Gemini AI reviewer is not correct to suggest this code change because it will lose the indentation and the code will be harder to read. And most probably cargo fmt --check will complain about it.

if token == Token::EOF || token == Token::SemiColon {
break;
} else {
Expand Down Expand Up @@ -1079,7 +1079,7 @@ impl<'a> DFParser<'a> {
}
}
} else {
let token = self.parser.next_token();
let token = self.parser.peek_token();
Copy link

@gemini-code-assist gemini-code-assist bot Jan 22, 2026

Choose a reason for hiding this comment

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

high

This change from next_token() to peek_token() mirrors the fix on line 661 and addresses the same correctness issue within the parse_create_external_table function. It prevents premature token consumption, ensuring that the parser's state remains consistent when checking for statement delimiters.

Suggested change
let token = self.parser.peek_token();
let token = self.parser.peek_token();

Copy link
Owner Author

@martin-augment martin-augment Jan 22, 2026

Choose a reason for hiding this comment

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

value:annoying; category:bug; feedback:The Gemini AI reviewer is not correct to suggest this code change because it will lose the indentation and the code will be harder to read. And most probably cargo fmt --check will complain about it.

if token == Token::EOF || token == Token::SemiColon {
break;
} else {
Expand Down Expand Up @@ -2026,6 +2026,48 @@ mod tests {
);
}

#[test]
fn test_multistatement() {
let sql = "COPY foo TO bar STORED AS CSV; \
CREATE EXTERNAL TABLE t(c1 int) STORED AS CSV LOCATION 'foo.csv'; \
RESET var;";
let statements = DFParser::parse_sql(sql).unwrap();
assert_eq!(
statements,
vec![
Statement::CopyTo(CopyToStatement {
source: object_name("foo"),
target: "bar".to_string(),
partitioned_by: vec![],
stored_as: Some("CSV".to_owned()),
options: vec![],
}),
{
let name = ObjectName::from(vec![Ident::from("t")]);
let display = None;
Statement::CreateExternalTable(CreateExternalTable {
name: name.clone(),
columns: vec![make_column_def("c1", DataType::Int(display))],
file_type: "CSV".to_string(),
location: "foo.csv".into(),
table_partition_cols: vec![],
order_exprs: vec![],
if_not_exists: false,
or_replace: false,
temporary: false,
unbounded: false,
options: vec![],
constraints: vec![],
})
},
{
let name = ObjectName::from(vec![Ident::from("var")]);
Statement::Reset(ResetStatement::Variable(name))
}
]
);
}
Comment on lines +2030 to +2069
Copy link

@gemini-code-assist gemini-code-assist bot Jan 22, 2026

Choose a reason for hiding this comment

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

medium

The addition of test_multistatement is excellent. It provides good coverage for the parser's ability to handle multiple statements, which is directly impacted by the peek_token changes. This test ensures the parser correctly identifies and processes a sequence of COPY, CREATE EXTERNAL TABLE, and RESET statements.


fn expect_parse_expr_ok(sql: &str, expected: ExprWithAlias) {
let expr = DFParser::parse_sql_into_expr(sql).unwrap();
assert_eq!(expr, expected, "actual:\n{expr:#?}");
Expand Down