martin-g commented on code in PR #18761:
URL: https://github.com/apache/datafusion/pull/18761#discussion_r2533635468
##########
datafusion/expr-common/src/columnar_value.rs:
##########
@@ -275,16 +282,69 @@ impl ColumnarValue {
) -> Result<ColumnarValue> {
let cast_options =
cast_options.cloned().unwrap_or(DEFAULT_CAST_OPTIONS);
match self {
- ColumnarValue::Array(array) => Ok(ColumnarValue::Array(
- kernels::cast::cast_with_options(array, cast_type,
&cast_options)?,
- )),
+ ColumnarValue::Array(array) => {
+ ensure_date_array_timestamp_bounds(array, cast_type)?;
+ Ok(ColumnarValue::Array(kernels::cast::cast_with_options(
+ array,
+ cast_type,
+ &cast_options,
+ )?))
+ }
ColumnarValue::Scalar(scalar) => Ok(ColumnarValue::Scalar(
scalar.cast_to_with_options(cast_type, &cast_options)?,
)),
}
}
}
+fn ensure_date_array_timestamp_bounds(
+ array: &ArrayRef,
+ cast_type: &DataType,
+) -> Result<()> {
+ let source_type = array.data_type().clone();
+ let Some(multiplier) = date_to_timestamp_multiplier(&source_type,
cast_type) else {
+ return Ok(());
+ };
+
+ if multiplier <= 1 {
+ return Ok(());
+ }
+
+ let iter: Box<dyn Iterator<Item = i64> + '_> = match &source_type {
+ DataType::Date32 => {
+ let arr = array
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .ok_or_else(|| {
+ internal_datafusion_err!(
+ "Expected Date32Array but found {}",
+ array.data_type()
+ )
+ })?;
+ Box::new(arr.iter().flatten().map(|v| v as i64))
+ }
+ DataType::Date64 => {
+ let arr = array
+ .as_any()
+ .downcast_ref::<Date64Array>()
+ .ok_or_else(|| {
+ internal_datafusion_err!(
+ "Expected Date64Array but found {}",
+ array.data_type()
+ )
+ })?;
+ Box::new(arr.iter().flatten())
+ }
+ _ => return Ok(()), // Not a date type, nothing to do
+ };
+
+ for value in iter {
+ ensure_timestamp_in_bounds(value, multiplier, &source_type,
cast_type)?;
Review Comment:
Could this be done more effectively with the `compute::max()/min()` kernels
instead of iterating over all items ?!
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -91,6 +91,99 @@ use chrono::{Duration, NaiveDate};
use half::f16;
pub use struct_builder::ScalarStructBuilder;
+const SECONDS_PER_DAY: i64 = 86_400;
+const MILLIS_PER_DAY: i64 = SECONDS_PER_DAY * 1_000;
+const MICROS_PER_DAY: i64 = MILLIS_PER_DAY * 1_000;
+const NANOS_PER_DAY: i64 = MICROS_PER_DAY * 1_000;
+const MICROS_PER_MILLISECOND: i64 = 1_000;
+const NANOS_PER_MILLISECOND: i64 = 1_000_000;
+
+/// Returns the multiplier that converts the input date representation into the
+/// desired timestamp unit, if the conversion requires a multiplication that
can
+/// overflow an `i64`.
+pub fn date_to_timestamp_multiplier(
+ source_type: &DataType,
+ target_type: &DataType,
+) -> Option<i64> {
+ let DataType::Timestamp(target_unit, _) = target_type else {
+ return None;
+ };
+
+ // Only `Timestamp` target types have a time unit; otherwise no
+ // multiplier applies (handled above). The function returns `Some(m)`
+ // when converting the `source_type` to `target_type` requires a
+ // multiplication that could overflow `i64`. It returns `None` when
+ // the conversion is a division or otherwise doesn't require a
+ // multiplication (e.g. Date64 -> Second).
+ match source_type {
+ // Date32 stores days since epoch. Converting to any timestamp
+ // unit requires multiplying by the per-day factor (seconds,
+ // milliseconds, microseconds, nanoseconds).
+ DataType::Date32 => Some(match target_unit {
+ TimeUnit::Second => SECONDS_PER_DAY,
+ TimeUnit::Millisecond => MILLIS_PER_DAY,
+ TimeUnit::Microsecond => MICROS_PER_DAY,
+ TimeUnit::Nanosecond => NANOS_PER_DAY,
+ }),
+
+ // Date64 stores milliseconds since epoch. Converting to
+ // seconds is a division (no multiplication), so return `None`.
+ // Converting to milliseconds is 1:1 (multiplier 1). Converting
+ // to micro/nano requires multiplying by 1_000 / 1_000_000.
+ DataType::Date64 => match target_unit {
+ TimeUnit::Second => None,
+ TimeUnit::Millisecond => Some(1),
Review Comment:
Is it necessary to multiply/divide by 1 at all ? I.e. maybe this should
return a None ?!
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -91,6 +91,99 @@ use chrono::{Duration, NaiveDate};
use half::f16;
pub use struct_builder::ScalarStructBuilder;
+const SECONDS_PER_DAY: i64 = 86_400;
+const MILLIS_PER_DAY: i64 = SECONDS_PER_DAY * 1_000;
+const MICROS_PER_DAY: i64 = MILLIS_PER_DAY * 1_000;
+const NANOS_PER_DAY: i64 = MICROS_PER_DAY * 1_000;
+const MICROS_PER_MILLISECOND: i64 = 1_000;
+const NANOS_PER_MILLISECOND: i64 = 1_000_000;
+
+/// Returns the multiplier that converts the input date representation into the
+/// desired timestamp unit, if the conversion requires a multiplication that
can
+/// overflow an `i64`.
+pub fn date_to_timestamp_multiplier(
+ source_type: &DataType,
+ target_type: &DataType,
+) -> Option<i64> {
+ let DataType::Timestamp(target_unit, _) = target_type else {
+ return None;
+ };
+
+ // Only `Timestamp` target types have a time unit; otherwise no
+ // multiplier applies (handled above). The function returns `Some(m)`
+ // when converting the `source_type` to `target_type` requires a
+ // multiplication that could overflow `i64`. It returns `None` when
+ // the conversion is a division or otherwise doesn't require a
+ // multiplication (e.g. Date64 -> Second).
+ match source_type {
+ // Date32 stores days since epoch. Converting to any timestamp
+ // unit requires multiplying by the per-day factor (seconds,
+ // milliseconds, microseconds, nanoseconds).
+ DataType::Date32 => Some(match target_unit {
+ TimeUnit::Second => SECONDS_PER_DAY,
+ TimeUnit::Millisecond => MILLIS_PER_DAY,
+ TimeUnit::Microsecond => MICROS_PER_DAY,
+ TimeUnit::Nanosecond => NANOS_PER_DAY,
+ }),
+
+ // Date64 stores milliseconds since epoch. Converting to
+ // seconds is a division (no multiplication), so return `None`.
+ // Converting to milliseconds is 1:1 (multiplier 1). Converting
+ // to micro/nano requires multiplying by 1_000 / 1_000_000.
+ DataType::Date64 => match target_unit {
+ TimeUnit::Second => None,
+ TimeUnit::Millisecond => Some(1),
+ TimeUnit::Microsecond => Some(MICROS_PER_MILLISECOND),
+ TimeUnit::Nanosecond => Some(NANOS_PER_MILLISECOND),
+ },
+
+ _ => None,
+ }
+}
+
+/// Ensures the provided value can be represented as a timestamp with the given
+/// multiplier. Returns an [`DataFusionError::Execution`] when the converted
+/// value would overflow the timestamp range.
+pub fn ensure_timestamp_in_bounds(
+ value: i64,
+ multiplier: i64,
+ source_type: &DataType,
+ target_type: &DataType,
+) -> Result<()> {
+ if multiplier <= 1 {
+ return Ok(());
+ }
+
+ if value.checked_mul(multiplier).is_none() {
+ let target = format_timestamp_type_for_error(target_type);
+ _exec_err!(
+ "Cannot cast {} value {} to {}: timestamp values are limited to
+/-2262 years",
Review Comment:
+/-2262 is true if the target type is nanoseconds, no ? For
seconds/milli/micro the limit years would be a bigger number.
Maybe:
```suggestion
"Cannot cast {} value {} to {}: converted value exceeds the
representable i64 range",
```
--
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]
