Repository navigation
Interval multiplication parses differently depending on operand order #1345
Description
Depending on operands order, intervals without leading fields (with literal alone) parses differently.
I've checked on sqlparser = "0.48.0"
SELECT 1 * interval '1 month'; parses as BinOp(Number, Interval(String)), which, I think, is correct:
projection: [
UnnamedExpr(
BinaryOp {
left: Value(
Number(
"1",
false,
),
),
op: Multiply,
right: Interval(
Interval {
value: Value(
SingleQuotedString(
"1 month",
),
),
// ...
},
),
},
),
],
But SELECT interval '1 month' * 1; parses as Interval(BinOp(String, Number)), which, to me, is incorrect:
projection: [
UnnamedExpr(
Interval(
Interval {
value: BinaryOp {
left: Value(
SingleQuotedString(
"1 month",
),
),
op: Multiply,
right: Value(
Number(
"1",
false,
),
),
},
// ...
},
),
),
],
I think that culprit is these calls
https://github.com/sqlparser-rs/sqlparser-rs/blob/20f7ac59e38d52e293476b7ad844e7f744a16c43/src/parser/mod.rs#L2027
=>
https://github.com/sqlparser-rs/sqlparser-rs/blob/20f7ac59e38d52e293476b7ad844e7f744a16c43/src/parser/mod.rs#L906
It would consume not only literals, but any expression as well, and store result as Intervals value
I propose to change parse_interval_expr so it would allow only literals, or maybe even replace it with parse_value.
I didn't actually check if that would break any tests, but at first glance it should not.
Also representation of interval value in AST could be changed from Expr to Value, but that would break public API, so could be postponed.
Activity
Abdullahsab3 commentedon Sep 5, 2024
I looked into this, since I think that's the root cause of apache/datafusion#12190 (Thanks @tshauck for the pointer). I think changing
parse_interval_exprtoparse_valuewould fix the issue of a wrongly parsed AST for the binops between intervals. However it would break the following syntax (and test):SELECT INTERVAL 1 + 1 DAYThese PR might be relevant:
Value::IntervaltoExpr::Interval#609exprinstead ofstringfor argument tointerval#517I think this syntax is used for the MySQL/BigQuery dialects.
mcheshkov commentedon Sep 5, 2024
Wow MySQL is
strangepowerful! It can parseINTERVAL CONCAT('1', '1') DAY, and interpret that an 11 day interval.I guess it would need a separate flag in dialect then, something like "suports expressions in interval values", as PostgreSQL does not seem to support that.
Or even "parse literal value", because, I think, they both differ from standard, which requires both single-quoted strings and qualifier outside of string, while in PostgreSQL stuff like
SELECT INTERVAL '''10:12 hours''';worksAbdullahsab3 commentedon Sep 6, 2024
I think that makes sense. I think a somewhat similar alternative is to define how binary operators between intervals are parsed. As an example:
Following is how you binop intervals in postgresql/Datafusion:
Following is how you binop intervals in mysql
There might be other approaches/syntax for binary operations between intervals. Not sure
Abdullahsab3 commentedon Sep 6, 2024
I take it back. I think postgres does kinda support that, just not in the same way
However, it shouldn't matter actually for sqlparser, since it will just be
Interval { value: Value(SingleQuotedString("1 month + 2 day")), leading_field: Some(Day)Abdullahsab3 commentedon Sep 6, 2024
Following PR that has been recently merged might be relevant:
#1398
findepi commentedon Sep 12, 2024
cc @alamb
alamb commentedon Sep 13, 2024
@samuelcolvin has also made some good progress on cleaning up interval parsing recently. Perhaps you can retry the example with the recently release sqlparser 0.51.0
mcheshkov commentedon Sep 13, 2024
Yes, I've rechecked, and my examples parses fine on
sqlparser = "0.51.0", so I'll just close this.