Fix parsing cast operator after parenthesized DEFAULT expression

[isaacparker0]

Fix a regression introduced in v0.59.0 by #1927:

CREATE TABLE t (c text DEFAULT (foo())::text);

fails to parse with

ParserError("Expected: ',' or ')' after column definition, found: ::")

To fix, continue parsing infix operators after the parenthesized prefix in parse_column_option_expr.

[isaacparker0]

@iffyio looks like you reviewed #1927, perhaps you have the most context to review this PR?

[iffyio]

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?

[isaacparker0]

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_subexpr¹. 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 ↩

[iffyio]

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)
}

[isaacparker0]

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.

[iffyio]

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

[isaacparker0]

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.

[iffyio]

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

the precedence operator should handle this case?

[iffyio]

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

[isaacparker0]

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.

[iffyio]

That looks good to me thanks!

[iffyio]

Thanks @isaacparker0, left some minor comments otherwise I think this looks good overall

[iffyio]

LGTM! Thanks @isaacparker0!