You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2023/05/31 15:13:41 UTC
[GitHub] [arrow-rs] comphead opened a new pull request, #4323: fix date conversion if timestamp below unixtimestamp
comphead opened a new pull request, #4323:
URL: https://github.com/apache/arrow-rs/pull/4323
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #4211 .
# Rationale for this change
Fix date conversion if timestamp below unixtimestamp
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] comphead commented on pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#issuecomment-1570430335
@parthchandra @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] comphead commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1212077856
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(num::integer::div_floor::<i64>(time_array.value(i), from_size) as i32);
Review Comment:
Can this convo be resolved then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1212028263
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(num::integer::div_floor::<i64>(time_array.value(i), from_size) as i32);
Review Comment:
If you look at the implementation of div floor, that is what it is implemented as
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1211899450
##########
arrow-cast/src/cast.rs:
##########
@@ -9172,4 +9172,46 @@ mod tests {
);
assert!(casted_array.is_err());
}
+
+ #[test]
+ fn test_cast_below_unixtimestamp() {
+ let valid = StringArray::from(vec![
+ "1900-01-03 23:59:59",
+ "1969-12-31 00:00:01",
+ "1989-12-31 00:00:01",
+ ]);
+
+ let array = Arc::new(valid) as ArrayRef;
+ let casted_array = cast_with_options(
+ &array,
+ &DataType::Timestamp(TimeUnit::Nanosecond, Some("+00:00".into())),
+ &CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ },
+ )
+ .unwrap();
+
+ let ts_array = casted_array
+ .as_any()
+ .downcast_ref::<TimestampNanosecondArray>()
+ .unwrap();
+
+ let ts_array = ts_array
+ .values()
+ .iter()
+ .map(|ts| Some(ts / 1_000_000))
+ .collect::<Vec<_>>();
+ let array =
+ TimestampMillisecondArray::from(ts_array).with_timezone("UTC".to_string());
+
+ let casted_array = cast(&array, &DataType::Date32).unwrap();
+ let date_array = casted_array.as_any().downcast_ref::<Date32Array>().unwrap();
Review Comment:
```suggestion
let date_array = casted_array.as_primitive::<Date32Type>();
```
##########
arrow-cast/src/cast.rs:
##########
@@ -9172,4 +9172,46 @@ mod tests {
);
assert!(casted_array.is_err());
}
+
+ #[test]
+ fn test_cast_below_unixtimestamp() {
+ let valid = StringArray::from(vec![
+ "1900-01-03 23:59:59",
+ "1969-12-31 00:00:01",
+ "1989-12-31 00:00:01",
+ ]);
+
+ let array = Arc::new(valid) as ArrayRef;
+ let casted_array = cast_with_options(
+ &array,
+ &DataType::Timestamp(TimeUnit::Nanosecond, Some("+00:00".into())),
+ &CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ },
+ )
+ .unwrap();
+
+ let ts_array = casted_array
+ .as_any()
+ .downcast_ref::<TimestampNanosecondArray>()
+ .unwrap();
+
+ let ts_array = ts_array
+ .values()
+ .iter()
+ .map(|ts| Some(ts / 1_000_000))
+ .collect::<Vec<_>>();
+ let array =
+ TimestampMillisecondArray::from(ts_array).with_timezone("UTC".to_string());
+
+ let casted_array = cast(&array, &DataType::Date32).unwrap();
+ let date_array = casted_array.as_any().downcast_ref::<Date32Array>().unwrap();
+
+ let casted_array = cast(&date_array, &DataType::Utf8).unwrap();
+ let string_array = casted_array.as_any().downcast_ref::<StringArray>().unwrap();
Review Comment:
```suggestion
let string_array = casted_array.as_string::<i32>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1211898332
##########
arrow-cast/src/cast.rs:
##########
@@ -9172,4 +9172,46 @@ mod tests {
);
assert!(casted_array.is_err());
}
+
+ #[test]
+ fn test_cast_below_unixtimestamp() {
+ let valid = StringArray::from(vec![
+ "1900-01-03 23:59:59",
+ "1969-12-31 00:00:01",
+ "1989-12-31 00:00:01",
+ ]);
+
+ let array = Arc::new(valid) as ArrayRef;
+ let casted_array = cast_with_options(
+ &array,
+ &DataType::Timestamp(TimeUnit::Nanosecond, Some("+00:00".into())),
+ &CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ },
+ )
+ .unwrap();
+
+ let ts_array = casted_array
+ .as_any()
+ .downcast_ref::<TimestampNanosecondArray>()
+ .unwrap();
Review Comment:
```suggestion
let ts_array = casted_array.as_primitive::<TimestampNanosecondType>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] parthchandra commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1212025802
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(num::integer::div_floor::<i64>(time_array.value(i), from_size) as i32);
Review Comment:
Something like
```
let pre_epoch_adjustment = if value(i) < 0 { -1 } else { 0 };
b.append_value(pre_epoch_adjustment + (time_array.value(i) / from_size) as i32);
```
might be faster (because it skips all the check made by `div_floor`) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] parthchandra commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1212340953
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(num::integer::div_floor::<i64>(time_array.value(i), from_size) as i32);
Review Comment:
@tustvold you'er right. My ide led me to the BigInt implementation of div_floor which is necessarily more complex, but not relevant here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1211899043
##########
arrow-cast/src/cast.rs:
##########
@@ -9172,4 +9172,46 @@ mod tests {
);
assert!(casted_array.is_err());
}
+
+ #[test]
+ fn test_cast_below_unixtimestamp() {
+ let valid = StringArray::from(vec![
+ "1900-01-03 23:59:59",
+ "1969-12-31 00:00:01",
+ "1989-12-31 00:00:01",
+ ]);
+
+ let array = Arc::new(valid) as ArrayRef;
+ let casted_array = cast_with_options(
+ &array,
+ &DataType::Timestamp(TimeUnit::Nanosecond, Some("+00:00".into())),
+ &CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ },
+ )
+ .unwrap();
+
+ let ts_array = casted_array
+ .as_any()
+ .downcast_ref::<TimestampNanosecondArray>()
+ .unwrap();
+
+ let ts_array = ts_array
+ .values()
+ .iter()
+ .map(|ts| Some(ts / 1_000_000))
Review Comment:
```suggestion
.map(|ts| ts / 1_000_000)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1211893203
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(f64::floor(time_array.value(i) as f64 / from_size as f64) as i32);
Review Comment:
Perhaps we could use https://docs.rs/num/latest/num/integer/fn.div_floor.html or something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] comphead commented on pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#issuecomment-1570453103
Thanks @tustvold I will address the comments. Probably we need to review all tests for cast and use new syntax instead of downcast?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1212096858
##########
arrow-cast/src/cast.rs:
##########
@@ -9172,4 +9172,41 @@ mod tests {
);
assert!(casted_array.is_err());
}
+
+ #[test]
+ fn test_cast_below_unixtimestamp() {
Review Comment:
I confirmed this test fails on master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4323: fix date conversion if timestamp below unixtimestamp
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4323:
URL: https://github.com/apache/arrow-rs/pull/4323#discussion_r1211893203
##########
arrow-cast/src/cast.rs:
##########
@@ -1853,7 +1853,7 @@ pub fn cast_with_options(
if time_array.is_null(i) {
b.append_null();
} else {
- b.append_value((time_array.value(i) / from_size) as i32);
+ b.append_value(f64::floor(time_array.value(i) as f64 / from_size as f64) as i32);
Review Comment:
Perhaps we could use https://docs.rs/num/latest/num/trait.Integer.html#tymethod.div_floor or something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org