Skip to content
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.

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

feat: negation of Intervals #6312

Merged
merged 2 commits into from
May 10, 2023
Merged

feat: negation of Intervals #6312

merged 2 commits into from
May 10, 2023

Conversation

izveigor
Copy link
Contributor

@izveigor izveigor commented May 9, 2023

Which issue does this PR close?

Closes #6311

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels May 9, 2023
query ?
select -interval '5' - '1' - '2' year;
----
0 years -24 mons 0 days 0 hours 0 mins 0.000000000 secs
Copy link
Contributor

Choose a reason for hiding this comment

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

why it is -24 mons, not -2 years? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't clearly know why years always translate to months (See: Example) (maybe because the output function use IntervalYearMonth::to_months(). But, I think it is not the actual problem of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't clearly know why years always translate to months (See: Example) (maybe because the output function use IntervalYearMonth::to_months(). But, I think it is not the actual problem of this PR.

right, I'll investigate if we need a follow up on this

Copy link
Contributor

Choose a reason for hiding this comment

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

select extract( year from interval '5' year);

This works in PG and returns 5 years

But fails in DF, I'll create another ticket on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is related to the fact that interval gets parsed to the same IntervalMonthDayNano all the time -- which was changed in #5806

I think to handle it properly we need to have cast support between the different interval types, maybe 🤔

use datafusion_common::{cast::as_primitive_array, Result};
use paste::paste;

macro_rules! test_array_negative_op {
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for adding this coverage of regular negate

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks like an improvement to me -- thank you @izveigor and @comphead

@alamb
Copy link
Contributor

alamb commented May 10, 2023

Looks like @comphead has filed #6327 👍

@alamb alamb merged commit 1921ee6 into apache:main May 10, 2023
Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request May 11, 2023
* feat: negation of Intervals

* fix: clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negation of Intervals
3 participants