parthchandra commented on code in PR #543:
URL: https://github.com/apache/datafusion-comet/pull/543#discussion_r1633909294
##########
core/src/execution/datafusion/expressions/utils.rs:
##########
@@ -81,12 +81,12 @@ pub fn array_with_timezone(
array: ArrayRef,
timezone: String,
to_type: Option<&DataType>,
-) -> ArrayRef {
+) -> Result<ArrayRef, ArrowError> {
Review Comment:
Should we return an Err instead of panicking in the not supported case?
##########
core/src/execution/datafusion/expressions/utils.rs:
##########
@@ -134,36 +140,40 @@ pub fn array_with_timezone(
/// array - input array of timestamp without timezone
/// tz - timezone of the values in the input array
/// to_timezone - timezone to change the input values to
-fn timestamp_ntz_to_timestamp(array: ArrayRef, tz: &str, to_timezone:
Option<&str>) -> ArrayRef {
+fn timestamp_ntz_to_timestamp(
+ array: ArrayRef,
+ tz: &str,
+ to_timezone: Option<&str>,
+) -> Result<ArrayRef, ArrowError> {
assert!(!tz.is_empty());
match array.data_type() {
DataType::Timestamp(_, None) => {
let array = as_primitive_array::<TimestampMicrosecondType>(&array);
- let tz: Tz = tz.parse().unwrap();
- let values = array.iter().map(|v| {
- v.map(|value| {
- let local_datetime =
as_datetime::<TimestampMicrosecondType>(value).unwrap();
- let datetime: DateTime<Tz> =
tz.from_local_datetime(&local_datetime).unwrap();
- datetime.timestamp_micros()
- })
- });
- let mut array: PrimitiveArray<TimestampMicrosecondType> =
- unsafe { PrimitiveArray::from_trusted_len_iter(values) };
- array = if let Some(to_tz) = to_timezone {
+ let tz: Tz = tz.parse()?;
Review Comment:
We should probably add a case for an invalid timezone in the unit test.
##########
core/src/execution/datafusion/expressions/utils.rs:
##########
@@ -118,14 +118,20 @@ pub fn array_with_timezone(
let dict = as_dictionary_array::<Int32Type>(&array);
let array =
as_primitive_array::<TimestampMicrosecondType>(dict.values());
let array_with_timezone =
- array_with_timezone(Arc::new(array.clone()) as ArrayRef,
timezone, to_type);
+ array_with_timezone(Arc::new(array.clone()) as ArrayRef,
timezone, to_type)?;
let dict = dict.with_values(array_with_timezone);
- Arc::new(dict) as ArrayRef
+ Ok(Arc::new(dict))
}
- _ => array,
+ _ => Ok(array),
}
}
+fn datetime_cast_err(value: i64) -> ArrowError {
+ ArrowError::CastError(format!(
+ "Cannot convert TimestampMicrosecondType {value} to datetime. Comet
only supports dates between of Jan 1, 262145 BCE to Dec 31, 262143 CE",
Review Comment:
```suggestion
"Cannot convert TimestampMicrosecondType {value} to datetime. Comet
only supports dates between Jan 1, 262145 BCE and Dec 31, 262143 CE",
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
