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 testcases while chrono-tz enabled #2932

Merged
merged 5 commits into from Oct 26, 2022

Conversation

waitingkuo
Copy link
Contributor

Which issue does this PR close?

Closes #2931

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Oct 26, 2022
Comment on lines +3856 to +3857
assert_eq!("1997-05-19 00:00:00.005 +00:00", c.value(0));
assert_eq!("2018-12-25 00:00:00.001 +00:00", c.value(1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the result for Timestamp(Unit, Some("UTC")) should contains +00:00

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the space before the timezone offset expected here?

Copy link
Contributor

@tustvold tustvold Oct 26, 2022

Choose a reason for hiding this comment

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

It appears to output https://docs.rs/chrono/latest/chrono/struct.DateTime.html#method.to_rfc2822 which has spaces

Edit: it actually outputs some chrono default format, we should potentially file a ticket to switch to using an actual standard 🤔

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2f37a847f2064109da31fca9845a0d8b

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed - #2934

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'm working on #2917 and found this issue as well. Is 2018-01-26T18:30:09.453829+08:00 preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like rfc3339 format, which is probably the most common or standardized format.

chrono has a separate function for this format which also allows specifying how many fractional digits you want.

Comment on lines +5757 to +5759
Some("1970-01-01 20:30:00 +10:00"),
None,
Some("1970-01-02 09:58:59"),
Some("1970-01-02 09:58:59 +10:00"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the result for Timestamp(Unit, Some("Australia/Sydnet")) should contains +10:00

Comment on lines 530 to 534
consectetur adipiscing elit,,2,false,2019-04-18T10:54:47.378000000,06:51:20,cupcakes
sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555000000,23:46:03,foo
Lorem ipsum dolor sit amet,123.564532,3,true,,00:20:34,cupcakes
consectetur adipiscing elit,,2,false,2019-04-18T10:54:47.378000000+00:00,06:51:20,cupcakes
sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555000000+00:00,23:46:03,foo
consectetur adipiscing elit,,2,false,2019-04-18T10:54:47.378000000,06:51:20,cupcakes
sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555000000,23:46:03,foo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timestamp<Unit, None> contains no timezone

Comment on lines +649 to +650
2019-04-18T20:54:47.378000000+10:00,2019-04-18T10:54:47.378000000
2021-10-30T17:59:07.000000000+11:00,2021-10-30T06:59:07.000000000\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timestamp<Unit, None> contains no timezone

@tustvold
Copy link
Contributor

Could we possibly add checks for this to CI?

@tustvold tustvold added the development-process Related to development process of arrow-rs label Oct 26, 2022
Comment on lines 71 to 72
- name: Test arrow with all features
run: cargo test -p arrow --all-features
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tustvold i just added cargo test with all features

Copy link
Contributor

@tustvold tustvold Oct 26, 2022

Choose a reason for hiding this comment

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

I think this will fail on stable, as it will enable the SIMD feature

Edit: I'm currently digging around in the CI anyway, so I'll push a commit to fix

Edit Edit: I see you got there first 👍

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

LGTM - thank you

Comment on lines +71 to +73
- name: Test --features=force_validate,prettyprint,ipc_compression,ffi,dyn_cmp_dict,dyn_arith_dict,chrono-tz
run: |
cargo test -p arrow --features=force_validate,prettyprint,ipc_compression,ffi,dyn_cmp_dict,dyn_arith_dict
cargo test -p arrow --features=force_validate,prettyprint,ipc_compression,ffi,dyn_cmp_dict,dyn_arith_dict,chrono-tz
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tustvold You're right, it failed while all features enabled. I added chrono-tz here instead

@tustvold tustvold mentioned this pull request Oct 26, 2022
@tustvold
Copy link
Contributor

Clippy seems to be finding more code to complain about now 😢

@waitingkuo
Copy link
Contributor Author

waitingkuo commented Oct 26, 2022

Clippy seems to be finding more code to complain about now 😢

I just remove chrono-tz from clippy. I can submitt another pr to fix that and add it in clippy later

@tustvold tustvold merged commit ed5843e into apache:master Oct 26, 2022
@ursabot
Copy link

ursabot commented Oct 26, 2022

Benchmark runs are scheduled for baseline = f812d2c and contender = ed5843e. ed5843e 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-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-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
arrow Changes to the arrow crate development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failed to pass test cases while enabling feature chrono-tz
4 participants