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.

Sign up for GitHub

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

Jump to bottom

feat: interval add timestamp #5491

Merged
merged 3 commits into from
Mar 8, 2023
Merged

feat: interval add timestamp #5491

merged 3 commits into from
Mar 8, 2023

Conversation

Weijun-H
Copy link
Member

@Weijun-H Weijun-H commented Mar 6, 2023

Which issue does this PR close?

Closes #5413

Rationale for this change

As the timestamp + feature has already been implemented, you can further enhance the imp_op function by adding additional matching conditions to support interval operations.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@Weijun-H Weijun-H force-pushed the interval_add_timestamp branch from c2ac111 to 8e65992 Compare March 6, 2023 21:51
@korowa
Copy link
Contributor

korowa commented Mar 7, 2023

Thank you for the contribution @Weijun-H!

Could you please add some "end-to-end" sqllogictests (I guess adding to timestamps.slt or maybe scalar.slt will be fine)? It'll also allow to check if additional changes in type coercion rules / transforming SQL to physical expressions are needed.

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 7, 2023
korowa
korowa reviewed Mar 7, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/timestamps.slt
@@ -191,3 +191,24 @@ query P
SELECT DATE_TRUNC('second', '2022-08-03 14:38:50Z');
----
2022-08-03T14:38:50

# Test that interval can add a timestamp
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!

I also suppose it's worth adding tests for interval + timestamp case, as it was added in scalar.rs

Copy link
Contributor

Choose a reason for hiding this comment

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

I see tests here for timestamp + interval and interval + timestamp (e.g. SELECT INTERVAL '8' DAY + timestamp '2013-07-01 12:00:00';)

I don't see tests for date + timestamp and timestamp + date -- maybe that is what you meant?

Copy link
Contributor

@korowa korowa Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I meant last four tests, added in 2nd commit. Now when we have them this conversation can be "marked as resolved" 🙃

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Mar 7, 2023
@Weijun-H Weijun-H force-pushed the interval_add_timestamp branch from 4b3b94a to fbe6c7c Compare March 7, 2023 19:10
alamb
alamb approved these changes Mar 7, 2023
View reviewed changes
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.

Thank you @Weijun-H -- this is awesome. I'll wait for your response to @korowa 's comment.

datafusion/physical-expr/src/planner.rs
@@ -197,6 +197,16 @@ pub fn create_physical_expr(
rhs,
input_schema,
)?)),
(
DataType::Interval(_),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

datafusion/core/tests/sqllogictests/test_files/timestamps.slt
@@ -191,3 +191,24 @@ query P
SELECT DATE_TRUNC('second', '2022-08-03 14:38:50Z');
----
2022-08-03T14:38:50

# Test that interval can add a timestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

I see tests here for timestamp + interval and interval + timestamp (e.g. SELECT INTERVAL '8' DAY + timestamp '2013-07-01 12:00:00';)

I don't see tests for date + timestamp and timestamp + date -- maybe that is what you meant?

@alamb alamb mentioned this pull request Mar 7, 2023
Closed
@Weijun-H
Copy link
Member Author

Weijun-H commented Mar 7, 2023

Hi @alamb, I have not tested date + timestamp and timestamp + date because they are invalid computations. Do you mean that I should test date + interval and interval + date instead?

@korowa
Copy link
Contributor

korowa commented Mar 8, 2023

LGTM

Thanks again @Weijun-H!

@alamb
Copy link
Contributor

alamb commented Mar 8, 2023

Do you mean that I should test date + interval and interval + date instead?

No sorry for not being clear. I was confused about @korowa 's suggestion. No changes needed. Thank you!

@alamb alamb merged commit 89ca583 into apache:main Mar 8, 2023
@ursabot
Copy link

ursabot commented Mar 8, 2023

Benchmark runs are scheduled for baseline = 9e32de3 and contender = 89ca583. 89ca583 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@andygrove andygrove added the enhancement New feature or request label Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@korowa korowa korowa approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate enhancement New feature or request logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Interval plus(+) Timestamp operation support
5 participants
@Weijun-H @korowa @alamb @ursabot @andygrove