Skip to content

Conversation

@sploiselle
Copy link
Contributor

Moving MaterializeInc/sqlparser#33 over

Added test case to assuage concern of parsing something like ..07 as a valid number. ..07 doesn't pass parsing because there are no expressions that begin with with a period; this is the same as the current behavior when trying to parse .07, which results in a SQL parsing error:

ERROR: sql parser error: Expected an expression, found: .

@benesch lmk if you'd still like this refactored!

@sploiselle sploiselle requested a review from benesch December 30, 2019 15:51
@sploiselle sploiselle added A-sql Area: SQL planning C-feature Category: new feature or request labels Dec 30, 2019
@benesch
Copy link
Contributor

benesch commented Dec 30, 2019

Oh, sorry, I gave you a bad counterexample but I think the issue does exist. Specifically it seems like something like .0.7.7 gets parsed as 0.0 rather than rejected as a syntax error.

Expr::Value(Value::Number(bigdecimal::BigDecimal::from(.1)))
);

#[cfg(not(feature = "bigdecimal"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, also, I deleted the bigdecimal feature, so if you remember you can drop the bigdecimal conditional here, and just delete the hunk above entirely.

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

🎉🎉 lgtm!

@benesch
Copy link
Contributor

benesch commented Dec 30, 2019

Although not to clippy, apologies.

@sploiselle sploiselle merged commit 436240d into MaterializeInc:master Dec 30, 2019
@sploiselle sploiselle deleted the parse-numbers-begin-decimal branch December 30, 2019 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-sql Area: SQL planning C-feature Category: new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants