Skip to content

Conversation

@waitingkuo
Copy link
Contributor

Which issue does this PR close?

Closes #3046

Rationale for this change

What changes are included in this PR?

change the oreder for to_timestamp signagure. make int64 be in front of utf8

Are there any user-facing changes?

@waitingkuo waitingkuo marked this pull request as ready for review August 5, 2022 20:03
@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions labels Aug 5, 2022
Copy link
Contributor Author

@waitingkuo waitingkuo left a comment

Choose a reason for hiding this comment

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

.

@waitingkuo
Copy link
Contributor Author

could anyone help me to figure this error? (for this first check Dev / Lint C++, .......)

Run archery lint --rat
  archery lint --rat
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/[3](https://github.com/apache/arrow-datafusion/runs/7698281962?check_suite_focus=true#step:5:3).10.5/x6[4](https://github.com/apache/arrow-datafusion/runs/7698281962?check_suite_focus=true#step:5:4)
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.[5](https://github.com/apache/arrow-datafusion/runs/7698281962?check_suite_focus=true#step:5:5)/x[6](https://github.com/apache/arrow-datafusion/runs/7698281962?check_suite_focus=true#step:5:6)4/lib
INFO:archery:Running apache-rat linter
apache-rat license violation: js/test.ts
Error: Process completed with exit code 1.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #3047 (30c9993) into master (a8ed874) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3047   +/-   ##
=======================================
  Coverage   85.84%   85.85%           
=======================================
  Files         289      289           
  Lines       51862    51890   +28     
=======================================
+ Hits        44520    44549   +29     
+ Misses       7342     7341    -1     
Impacted Files Coverage Δ
datafusion/core/tests/sql/timestamp.rs 100.00% <100.00%> (ø)
datafusion/expr/src/function.rs 97.68% <100.00%> (ø)
datafusion/expr/src/logical_plan/plan.rs 77.43% <0.00%> (-0.18%) ⬇️
datafusion/core/src/physical_plan/metrics/value.rs 87.43% <0.00%> (+0.50%) ⬆️
datafusion/expr/src/window_frame.rs 93.27% <0.00%> (+0.84%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@waitingkuo
Copy link
Contributor Author

@alamb would you mind helping me figure out my first check? I've committed dummy commits to trigger this several times.

@alamb
Copy link
Contributor

alamb commented Aug 6, 2022

https://github.com/apache/arrow-datafusion/runs/7699097645?check_suite_focus=true

Run archery lint --rat
INFO:archery:Running apache-rat linter
apache-rat license violation: js/test.ts

This means (very unobviously) that the file js/test.ts has no license header

Interestingly, this test is also failing on master changes too -- I will get a PR up to fix it
https://github.com/apache/arrow-datafusion/runs/7698061920?check_suite_focus=true

@alamb
Copy link
Contributor

alamb commented Aug 6, 2022

#3052 tracks the CI failure

@alamb
Copy link
Contributor

alamb commented Aug 6, 2022

#3053 should "fix" the problem -- TLDR is that the the CI check failure is not related to your changes in this PR @waitingkuo

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 @waitingkuo -- other than the test coverage looks good to me!

async fn to_timestamp_i32() -> Result<()> {
let ctx = SessionContext::new();

let sql = "select to_timestamp(cast (1 as int));";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add to_timestamp_seconds(..), to_timestamp_milliseconds(..) etc here?

@waitingkuo waitingkuo force-pushed the fix-to-timestamp-i32 branch from 38d1ff7 to 883c85a Compare August 6, 2022 12:37
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.

LGTM -- thanks @waitingkuo

@alamb alamb merged commit 815f1bc into apache:master Aug 6, 2022
@ursabot
Copy link

ursabot commented Aug 6, 2022

Benchmark runs are scheduled for baseline = a8ed874 and contender = 815f1bc. 815f1bc 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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

incorrect i32 coercion for to_timestamp

4 participants