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

fix: support date - timestamp and timestamp - date #5960

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

jackwener
Copy link
Member

Which issue does this PR close?

Closes #5931 .

Rationale for this change

support data/timestamp minus in type coercion.

What changes are included in this PR?

Add test in slt.

Are these changes tested?

Are there any user-facing changes?

@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 Apr 11, 2023
@jackwener jackwener force-pushed the date_minus branch 2 times, most recently from 24fbb70 to 908055e Compare April 11, 2023 10:45
@jackwener jackwener requested a review from alamb April 11, 2023 13:46
@jackwener
Copy link
Member Author

cc @alamb @berkaysynnada

@@ -88,3 +88,19 @@ h
statement error DataFusion error: Error during planning: Timestamp\(Nanosecond, Some\("\+00:00"\)\) \+ Utf8 can't be evaluated because there isn't a common type to coerce the types to
select i_item_desc from test
where d3_date > now() + '5 days';

# DATE minus DATE
Copy link
Member Author

Choose a reason for hiding this comment

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

I have a idea, maybe we can add a type_coercion.slt.
I prepare to add it in next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, btw we already have array_typeof.slt which also covers some casts

Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

Thank you @jackwener, LGTM. @berkaysynnada can chime in if there is anything I'm missing

@alamb alamb changed the title fix: support data/timestamp minus fix: support date - timestamp and timestamp - date Apr 11, 2023
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.

Thanks @jackwener and @comphead

0 years 0 mons 0 days 0 hours 0 mins 0.000000000 secs

# large timestamp - small timestamp
# Todo: Does it need to be fixed for negative values?.
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 negative values is correct and a valid interval. The intervals are signed according to the arrow spec:

https://github.com/apache/arrow/blob/776ea3f4f3d665f38fc370d0f284760eafa7de45/format/Schema.fbs#L363-L384

@@ -754,51 +751,6 @@ fn temporal_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataTyp
}
}

/// Coercion rule for numerical types: The type that both lhs and rhs
Copy link
Contributor

Choose a reason for hiding this comment

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

it is always nice to see a bug fixed by deleting code. Thank you @jackwener 👏

@jackwener jackwener merged commit da373e5 into apache:main Apr 12, 2023
@jackwener jackwener deleted the date_minus branch April 12, 2023 02:39
@jackwener
Copy link
Member Author

Thanks everyone❤️

korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request Apr 13, 2023
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.

Data Timestamp don't support minus
4 participants