Skip to content

Fix parsing cast operator after parenthesized DEFAULT expression #2168

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
iffyio merged 5 commits into from
Feb 2, 2026
Merged

Fix parsing cast operator after parenthesized DEFAULT expression #2168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2598da
Fix parsing of :: cast after parenthesized DEFAULT expression
isaacparker0 Jan 21, 2026
7aaa839
More comprehensive tests
isaacparker0 Jan 30, 2026
2f20af6
Add suggested test case
isaacparker0 Jan 30, 2026
7f746c5
New approach to handle at parenthesis boundary
isaacparker0 Feb 1, 2026
d6bb7a5
Pr feedback
isaacparker0 Feb 2, 2026
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
10 changes: 9 additions & 1 deletion src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9002,7 +9002,15 @@ impl<'a> Parser<'a> {
/// [ColumnOption::NotNull].
fn parse_column_option_expr(&mut self) -> Result<Expr, ParserError> {
if self.peek_token_ref().token == Token::LParen {
let expr: Expr = self.with_state(ParserState::Normal, |p| p.parse_prefix())?;
let mut expr = self.with_state(ParserState::Normal, |p| p.parse_prefix())?;
Copy link
Copy Markdown
Contributor

@iffyio iffyio Jan 29, 2026

Choose a reason for hiding this comment

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

Suggested change
let mut expr = self.with_state(ParserState::Normal, |p| p.parse_prefix())?;
let mut expr = self.with_state(ParserState::Normal, |p| p.parse_subexpr())?;

not sure I followed the intent of the added code, but I wonder would the issue be fixed by changing this line instead (thinking since (foo())::INT should be parsed as an expression), or are there other considerations?

Copy link
Copy Markdown
Contributor Author

@isaacparker0 isaacparker0 Jan 29, 2026

Choose a reason for hiding this comment

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

That change would fix the bug here, but break the behavior added in #1927. In Normal state, NOT NULL is treated as an expression (i.e., IS NOT NULL), but in ColumnDefinition state it's not, allowing the trailing NOT NULL to be parsed as a column constraint instead.

My proposed change is basically just duplicating part of the existing logic used in parse_subexpr1. If we run parse_prefix in normal state but the infix loop in ColumnDefinition state, we maintain the NOT NULL parsing behavior added in the previous PR but still properly handle infix operators like ::TEXT.

I'm still familiarizing myself with this repo, so there may be a better way to do this, but this is what I've come up with so far and why it seems like the right approach to me.

Also happy to do a bit of refactoring to cleanly extract a single shared logic for the part of parse_subexpr that we are duplicating if that seems preferable.

Footnotes

  1. https://github.com/apache/datafusion-sqlparser-rs/blob/62cf16f3ece6f3d5985e35893407c8db359ffd3f/src/parser/mod.rs#L1337-L1363 ↩

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jan 30, 2026

Choose a reason for hiding this comment

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

could we do something like this instead?

let expr = self.with_state(ParserState::Normal, |p| p.parse_subexpr(self.dialect.prec_value(Precedence::DoubleColon)))?;
if self.consume_token(&Token::DoubleColon) {
	Ok(Expr::Cast {
    	kind: CastKind::DoubleColon,
        expr: Box::new(expr),
        data_type: self.parse_data_type()?,
        format: None,
    })
} else {
	Ok(expr)
}

Copy link
Copy Markdown
Contributor Author

@isaacparker0 isaacparker0 Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This approach would only handle the :: case. There could be other infix operators or chained casts, which my current approach handles properly by leveraging the full existing infix loop.

I added additional test cases to document this behavior.

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jan 30, 2026

Choose a reason for hiding this comment

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

to support the chained cast, we would consume the double colon in a loop? I think the current approach duplicates parts of the expr parsing code most of which aren't exactly related to the problem being solved for, which makes it not ideal

Copy link
Copy Markdown
Contributor Author

@isaacparker0 isaacparker0 Jan 30, 2026

Choose a reason for hiding this comment

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

What about a non :: operator, such as CREATE TABLE t (c INT DEFAULT (foo()) + 1)? (I just added a test case for this)

Admittedly, we are getting into the territory of obscure but technically legal postgres syntax, but it still seems like we should solve this in a more general way than hardcoding one specific operator.

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jan 31, 2026

Choose a reason for hiding this comment

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

What about a non :: operator, such as CREATE TABLE t (c INT DEFAULT (foo()) + 1)?

the precedence operator should handle this case?

Copy link
Copy Markdown
Contributor

@iffyio iffyio Jan 31, 2026

Choose a reason for hiding this comment

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

Im thinking a generic version would need to use sub_expr somehow, ideally we find a way to reuse the core expr parsing loop

Copy link
Copy Markdown
Contributor Author

@isaacparker0 isaacparker0 Feb 1, 2026

Choose a reason for hiding this comment

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

Thanks for pushing back on this. I thought about this some more, and I think I came up with a more principled approach that also simplifies the solution from #1927: if we just change parse_prefix() for LParen to switch to ParserState::Normal, it allows us to remove the special-cased parse_column_option_expr added in #1927, while also fixing the regression with :: cast.

Let me know what you think of this approach.

Copy link
Copy Markdown
Contributor

@iffyio iffyio Feb 2, 2026

Choose a reason for hiding this comment

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

That looks good to me thanks!

expr = self.parse_compound_expr(expr, vec![])?;
loop {
let next_precedence = self.get_next_precedence()?;
if next_precedence == 0 || self.peek_token_ref().token == Token::Period {
break;
}
expr = self.parse_infix(expr, next_precedence)?;
}
Ok(expr)
} else {
Ok(self.parse_expr()?)
Expand Down
6 changes: 6 additions & 0 deletions tests/sqlparser_postgres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,12 @@ fn parse_create_table_with_defaults() {
}
}

#[test]
fn parse_default_expr_parenthesized_then_cast() {
Comment thread
isaacparker0 marked this conversation as resolved.
Outdated
// Infix operators like :: after parenthesized expression in DEFAULT
pg().verified_stmt("CREATE TABLE t (c TEXT DEFAULT (foo())::TEXT)");
Comment thread
isaacparker0 marked this conversation as resolved.
}

#[test]
fn parse_create_table_from_pg_dump() {
let sql = "CREATE TABLE public.customer (
Expand Down