fix: parser for negative intervals#6698
fix: parser for negative intervals#6698alamb merged 4 commits intoapache:mainfrom izveigor:parser_intervals
Conversation
github-actions
bot
added
core
datafusion/sql/src/expr/value.rs
Outdated
| ) => s, | ||
| ) => { | ||
| if negative { | ||
| "-".to_owned() + &s |
There was a problem hiding this comment.
| "-".to_owned() + &s | |
| format!("-{s}") |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @izveigor -- @waitingkuo I wonder if you also have a moment to review (the tests especially) to see if this looks right to you?
|
I apologize -- I accidentally pushed a commit to your branch 🤦 -- I force pushed to remove it |
| select -interval '5' - '1' - '2' year; | ||
| ---- | ||
| 0 years -24 mons 0 days 0 hours 0 mins 0.000000000 secs | ||
| 0 years -96 mons 0 days 0 hours 0 mins 0.000000000 secs |
There was a problem hiding this comment.
To me this doesn't look right. The previous test seems correct. When putting a negation in front of interval I believe it should negate the entire interval statement.
interval '5' - '1' - '2' year is one interval statement.
I think the statement
select -interval '5' - '1' - '2' year;
Should be interpreted as:
select - (interval '5' - '1' - '2' year);
Rather than
select (- interval '5') - '1' - '2' year;
If the user wants to negate only 5, they should write it as follows (below are all equivalents):
select interval '-5' - '1' - '2' year;
select interval - '5' - '1' - '2' year;
select - interval '5' year - interval '1' year - interval '2' year;
There was a problem hiding this comment.
Agree with you, I think we should notice this point. cc @alamb .
If we will merge this PR, I think we need to add a ticket to follow this problem
There was a problem hiding this comment.
I will wait for @waitingkuo 's comments -- I agree about filing the ticket if we merge
@alamb sure, I'll finish the review by tomorrow |
waitingkuo
left a comment
waitingkuo
left a comment
There was a problem hiding this comment.
looks good to me, thanks.
in postgrseql
select -interval '5' - '1' - '2';is intepreted as
select (-interval '5') - '1' - '2';willy=# select -interval '5' - '1' - '2';
?column?
-----------
-00:00:08
(1 row)note that select -interval '5' - '1' - '2' hour doesn't work in postgresql
willy=# select -interval '5' - '1' - '2' hour;
ERROR: syntax error at or near "hour"
LINE 1: select -interval '5' - '1' - '2' hour;
|
SQL in general and Postgres in particular never ceases to amaze me |
|
Thanks again @izveigor @waitingkuo @aprimadi @comphead and @jackwener -- team effort ✋ |
6 participants
Which issue does this PR close?
Part of #6649
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Yes