datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

feat: Add support for TIME literal values

Open stuartcarnie opened this issue 3 years ago • 2 comments

This is a draft PR, as it depends on an upgraded arrow-rs crate which contains breaking changes. This PR includes commits to resolve the changes and all tests pass, however, more test coverage for the TIME literal is expected, which is the primary intent of this PR.

Which issue does this PR close?

Closes #2883.

Rationale for this change

What changes are included in this PR?

TODO(sgc): Write details of all the changes

Are there any user-facing changes?

This PR teaches DataFusion about TIME literals and therefore documentation would need to be revised.

stuartcarnie avatar Aug 02 '22 04:08 stuartcarnie

Codecov Report

Merging #3010 (cc8caad) into master (48f9b7a) will decrease coverage by 0.00%. The diff coverage is 78.04%.

@@            Coverage Diff             @@
##           master    #3010      +/-   ##
==========================================
- Coverage   85.95%   85.94%   -0.01%     
==========================================
  Files         291      291              
  Lines       52382    52420      +38     
==========================================
+ Hits        45025    45054      +29     
- Misses       7357     7366       +9     
Impacted Files Coverage Δ
datafusion/common/src/scalar.rs 84.61% <63.15%> (-0.28%) :arrow_down:
datafusion/sql/src/planner.rs 81.99% <87.50%> (+0.04%) :arrow_up:
datafusion/core/tests/sql/timestamp.rs 99.78% <92.85%> (-0.22%) :arrow_down:
datafusion/expr/src/logical_plan/plan.rs 77.60% <0.00%> (-0.18%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Aug 02 '22 04:08 codecov-commenter

Thank you SO much for this! I struggled for a few days on this issue. Your help is very appreciated!

avantgardnerio avatar Aug 02 '22 15:08 avantgardnerio

thank you @stuartcarnie !

note that time64 cannot be compared yet as there's no implementation in the kernel.

waitingkuo avatar Aug 15 '22 17:08 waitingkuo

Benchmark runs are scheduled for baseline = 49a3b005ca3e0415e920b2459b1121dad729b743 and contender = 15a9a4becc4bb41262b10f1d5395fc1d026de753. 15a9a4becc4bb41262b10f1d5395fc1d026de753 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. Conbench compare runs links: [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2 [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x [Skipped :warning: 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

ursabot avatar Aug 15 '22 19:08 ursabot