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

fix testcases while chrono-tz enabled #2932

Merged
merged 5 commits into from
Oct 26, 2022
Merged

fix testcases while chrono-tz enabled #2932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55f1480
fix testcases
waitingkuo Oct 26, 2022
755368f
add arrow test with all features
waitingkuo Oct 26, 2022
f7bbe79
add chrono-tz in features
waitingkuo Oct 26, 2022
052d266
remove chrono-tz
waitingkuo Oct 26, 2022
e0803a0
remove duplicated blocks
waitingkuo Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/arrow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ jobs:
run: cargo test -p arrow-integration-test --all-features
- name: Test arrow
run: cargo test -p arrow
- name: Test --features=force_validate,prettyprint,ipc_compression,ffi,dyn_cmp_dict,dyn_arith_dict
- 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
Comment on lines +71 to +73
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

- name: Run examples
run: |
# Test arrow examples
Expand Down
8 changes: 4 additions & 4 deletions arrow/src/compute/kernels/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3853,8 +3853,8 @@ mod tests {
let b = cast(&array, &DataType::Utf8).unwrap();
let c = b.as_any().downcast_ref::<StringArray>().unwrap();
assert_eq!(&DataType::Utf8, c.data_type());
assert_eq!("1997-05-19 00:00:00.005", c.value(0));
assert_eq!("2018-12-25 00:00:00.001", c.value(1));
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));
Comment on lines +3856 to +3857
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 •
edited

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.

assert!(c.is_null(2));
}

Expand Down Expand Up @@ -5754,9 +5754,9 @@ mod tests {
let out = cast(&(Arc::new(array) as ArrayRef), &DataType::Utf8).unwrap();

let expected = StringArray::from(vec![
Some("1970-01-01 20:30:00"),
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"),
Comment on lines +5757 to +5759
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

]);

assert_eq!(
Expand Down
19 changes: 4 additions & 15 deletions arrow/src/csv/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,25 +524,14 @@ mod tests {
let mut buffer: Vec<u8> = vec![];
file.read_to_end(&mut buffer).unwrap();

let expected = if cfg!(feature = "chrono-tz") {
r#"c1,c2,c3,c4,c5,c6,c7
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
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
"#
} else {
r#"c1,c2,c3,c4,c5,c6,c7
let expected = r#"c1,c2,c3,c4,c5,c6,c7
Lorem ipsum dolor sit amet,123.564532,3,true,,00:20:34,cupcakes
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,06:51:20,cupcakes
sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555000000,23:46:03,foo
"#
};
"#;
assert_eq!(expected.to_string(), String::from_utf8(buffer).unwrap());
}

Expand Down Expand Up @@ -646,8 +635,8 @@ sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555000000,23:46:03,foo
}

let left = "c1,c2
2019-04-18T20:54:47.378000000+10:00,2019-04-18T10:54:47.378000000+00:00
2021-10-30T17:59:07.000000000+11:00,2021-10-30T06:59:07.000000000+00:00\n";
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";
Comment on lines +638 to +639
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

let right = writer.writer.into_inner().map(|s| s.to_string());
assert_eq!(Some(left.to_string()), right.ok());
}
Expand Down