You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/26 22:04:06 UTC
[GitHub] [arrow-datafusion] avantgardnerio opened a new pull request, #2797: Add support for month & year intervals
avantgardnerio opened a new pull request, #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797
# Which issue does this PR close?
Closes #2796.
# Rationale for this change
In order to pass TPC-H benchmarks, datafusion will need to be able to support month & year date intervals.
# What changes are included in this PR?
A refactoring of the datetime module to:
1. Support `IntervalYearMonth` in addition to the currently supported `IntervalDayTime`
2. Perform some non-trivial date modulo math (that I hopefully got correct)
3. Add tests to ensure modulo math edge cases work correctly
4. Switch to an arguably more readable [railroad error handling pattern](https://fsharpforfunandprofit.com/rop/) to take better advantage of the error propagation (`?`) operator.
# Are there any user-facing changes?
More queries should work. Nothing that worked previously should break.
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910187426
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,123 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => add_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
}
}
+
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
Review Comment:
Sadly I was not able to exercise this code path with tests, since the `timestamp` type did not appear to be plumbed. Maybe that makes sense since this is `datetime.rs`?
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178166214
I [PRed](https://github.com/chronotope/chrono/pull/731) the `chrono` repo.
--
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-datafusion] alamb commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r918252534
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,372 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => shift_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
+ }
+}
+
+// TODO: PR to arrow
Review Comment:
```suggestion
// Can remove once https://github.com/apache/arrow-rs/pull/2031 is released
```
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,372 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => shift_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
+ }
+}
+
+// TODO: PR to arrow
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
+ let interval = interval as u128;
+ let nanos = (interval >> 64) as i64 * sign as i64;
+ let days = (interval >> 32) as i32 * sign;
+ let months = interval as i32 * sign;
+ let a = shift_months(prior, months);
+ let b = a.add(Duration::days(days as i64));
+ b.add(Duration::nanoseconds(nanos))
+}
+
+// TODO: PR to arrow
Review Comment:
```suggestion
// Can remove once https://github.com/apache/arrow-rs/pull/2031 is released
```
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,372 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => shift_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
+ }
+}
+
+// TODO: PR to arrow
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
+ let interval = interval as u128;
+ let nanos = (interval >> 64) as i64 * sign as i64;
+ let days = (interval >> 32) as i32 * sign;
+ let months = interval as i32 * sign;
+ let a = shift_months(prior, months);
+ let b = a.add(Duration::days(days as i64));
+ b.add(Duration::nanoseconds(nanos))
+}
+
+// TODO: PR to arrow
+fn add_day_time(prior: NaiveDate, interval: i64, sign: i32) -> NaiveDate {
+ let interval = interval as u64;
+ let days = (interval >> 32) as i32 * sign;
+ let ms = interval as i32 * sign;
+ let intermediate = prior.add(Duration::days(days as i64));
+ intermediate.add(Duration::milliseconds(ms as i64))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::create_physical_expr;
+ use crate::execution_props::ExecutionProps;
+ use arrow::array::{ArrayRef, Date32Builder};
+ use arrow::datatypes::*;
+ use datafusion_common::{Result, ToDFSchema};
+ use datafusion_expr::Expr;
+
+ #[test]
+ fn add_11_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 11);
+ assert_eq!(format!("{:?}", actual).as_str(), "2000-12-01");
+ }
+
+ #[test]
+ fn add_12_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 12);
+ assert_eq!(format!("{:?}", actual).as_str(), "2001-01-01");
+ }
+
+ #[test]
+ fn add_13_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 13);
+ assert_eq!(format!("{:?}", actual).as_str(), "2001-02-01");
+ }
+
+ #[test]
+ fn sub_11_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -11);
+ assert_eq!(format!("{:?}", actual).as_str(), "1999-02-01");
+ }
+
+ #[test]
+ fn sub_12_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -12);
+ assert_eq!(format!("{:?}", actual).as_str(), "1999-01-01");
+ }
+
+ #[test]
+ fn sub_13_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -13);
+ assert_eq!(format!("{:?}", actual).as_str(), "1998-12-01");
+ }
+
+ #[test]
+ fn add_32_day_time() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = create_day_time(1, 0);
+ let interval = Expr::Literal(ScalarValue::IntervalDayTime(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1970-01-02");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
}
+
+ Ok(())
+ }
+
+ #[test]
+ fn sub_32_year_month() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Minus;
+ let interval = Expr::Literal(ScalarValue::IntervalYearMonth(Some(13)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1968-12-01");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_64_day_time() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date64(Some(0)));
+ let op = Operator::Plus;
+ let interval = create_day_time(-15, -24 * 60 * 60 * 1000);
+ let interval = Expr::Literal(ScalarValue::IntervalDayTime(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::milliseconds(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1969-12-16");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_32_year_month() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::IntervalYearMonth(Some(1)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1970-02-01");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_32_month_day_nano() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+
+ let interval = create_month_day_nano(-12, -15, -42);
+
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1968-12-17");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_interval() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::Null);
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add a NULL interval");
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_date() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Null);
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(0)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add to NULL date");
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_op() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Eq;
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(0)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add dates with == operator");
+
+ Ok(())
+ }
+
+ fn exercise(dt: &Expr, op: Operator, interval: &Expr) -> Result<ColumnarValue> {
+ let mut builder = Date32Builder::new(1);
+ builder.append_value(0).unwrap();
+ let a: ArrayRef = Arc::new(builder.finish());
+ let schema = Schema::new(vec![Field::new("a", DataType::Date32, false)]);
+ let batch = RecordBatch::try_new(Arc::new(schema.clone()), vec![a])?;
+
+ let dfs = schema.clone().to_dfschema()?;
+ let props = ExecutionProps::new();
+
+ let lhs = create_physical_expr(dt, &dfs, &schema, &props)?;
+ let rhs = create_physical_expr(interval, &dfs, &schema, &props)?;
+
+ let cut = DateIntervalExpr::try_new(lhs, op, rhs, &schema)?;
+ let res = cut.evaluate(&batch)?;
+ Ok(res)
+ }
+
+ // TODO: PR to arrow
Review Comment:
```suggestion
// Can remove once https://github.com/apache/arrow-rs/pull/2031 is released
```
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
Review Comment:
See https://github.com/apache/arrow-rs/pull/2031
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,372 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => shift_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
+ }
+}
+
+// TODO: PR to arrow
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
+ let interval = interval as u128;
+ let nanos = (interval >> 64) as i64 * sign as i64;
+ let days = (interval >> 32) as i32 * sign;
+ let months = interval as i32 * sign;
+ let a = shift_months(prior, months);
+ let b = a.add(Duration::days(days as i64));
+ b.add(Duration::nanoseconds(nanos))
+}
+
+// TODO: PR to arrow
+fn add_day_time(prior: NaiveDate, interval: i64, sign: i32) -> NaiveDate {
+ let interval = interval as u64;
+ let days = (interval >> 32) as i32 * sign;
+ let ms = interval as i32 * sign;
+ let intermediate = prior.add(Duration::days(days as i64));
+ intermediate.add(Duration::milliseconds(ms as i64))
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::create_physical_expr;
+ use crate::execution_props::ExecutionProps;
+ use arrow::array::{ArrayRef, Date32Builder};
+ use arrow::datatypes::*;
+ use datafusion_common::{Result, ToDFSchema};
+ use datafusion_expr::Expr;
+
+ #[test]
+ fn add_11_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 11);
+ assert_eq!(format!("{:?}", actual).as_str(), "2000-12-01");
+ }
+
+ #[test]
+ fn add_12_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 12);
+ assert_eq!(format!("{:?}", actual).as_str(), "2001-01-01");
+ }
+
+ #[test]
+ fn add_13_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, 13);
+ assert_eq!(format!("{:?}", actual).as_str(), "2001-02-01");
+ }
+
+ #[test]
+ fn sub_11_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -11);
+ assert_eq!(format!("{:?}", actual).as_str(), "1999-02-01");
+ }
+
+ #[test]
+ fn sub_12_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -12);
+ assert_eq!(format!("{:?}", actual).as_str(), "1999-01-01");
+ }
+
+ #[test]
+ fn sub_13_months() {
+ let prior = NaiveDate::from_ymd(2000, 1, 1);
+ let actual = shift_months(prior, -13);
+ assert_eq!(format!("{:?}", actual).as_str(), "1998-12-01");
+ }
+
+ #[test]
+ fn add_32_day_time() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = create_day_time(1, 0);
+ let interval = Expr::Literal(ScalarValue::IntervalDayTime(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1970-01-02");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
}
+
+ Ok(())
+ }
+
+ #[test]
+ fn sub_32_year_month() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Minus;
+ let interval = Expr::Literal(ScalarValue::IntervalYearMonth(Some(13)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1968-12-01");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_64_day_time() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date64(Some(0)));
+ let op = Operator::Plus;
+ let interval = create_day_time(-15, -24 * 60 * 60 * 1000);
+ let interval = Expr::Literal(ScalarValue::IntervalDayTime(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::milliseconds(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1969-12-16");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_32_year_month() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::IntervalYearMonth(Some(1)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1970-02-01");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn add_32_month_day_nano() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+
+ let interval = create_month_day_nano(-12, -15, -42);
+
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(interval)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval)?;
+
+ // assert
+ match res {
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(d))) => {
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let res = epoch.add(Duration::days(d as i64));
+ assert_eq!(format!("{:?}", res).as_str(), "1968-12-17");
+ }
+ _ => Err(DataFusionError::NotImplemented(
+ "Unexpected result!".to_string(),
+ ))?,
+ }
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_interval() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::Null);
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add a NULL interval");
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_date() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Null);
+ let op = Operator::Plus;
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(0)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add to NULL date");
+
+ Ok(())
+ }
+
+ #[test]
+ fn invalid_op() -> Result<()> {
+ // setup
+ let dt = Expr::Literal(ScalarValue::Date32(Some(0)));
+ let op = Operator::Eq;
+ let interval = Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(0)));
+
+ // exercise
+ let res = exercise(&dt, op, &interval);
+ assert!(res.is_err(), "Can't add dates with == operator");
+
+ Ok(())
+ }
+
+ fn exercise(dt: &Expr, op: Operator, interval: &Expr) -> Result<ColumnarValue> {
+ let mut builder = Date32Builder::new(1);
+ builder.append_value(0).unwrap();
+ let a: ArrayRef = Arc::new(builder.finish());
+ let schema = Schema::new(vec![Field::new("a", DataType::Date32, false)]);
+ let batch = RecordBatch::try_new(Arc::new(schema.clone()), vec![a])?;
+
+ let dfs = schema.clone().to_dfschema()?;
+ let props = ExecutionProps::new();
+
+ let lhs = create_physical_expr(dt, &dfs, &schema, &props)?;
+ let rhs = create_physical_expr(interval, &dfs, &schema, &props)?;
+
+ let cut = DateIntervalExpr::try_new(lhs, op, rhs, &schema)?;
+ let res = cut.evaluate(&batch)?;
+ Ok(res)
+ }
+
+ // TODO: PR to arrow
+ /// Creates an IntervalDayTime given its constituent components
+ ///
+ /// https://github.com/apache/arrow-rs/blob/e59b023480437f67e84ba2f827b58f78fd44c3a1/integration-testing/src/lib.rs#L222
+ fn create_day_time(days: i32, millis: i32) -> i64 {
+ let m = millis as u64 & u32::MAX as u64;
+ let d = (days as u64 & u32::MAX as u64) << 32;
+ (m | d) as i64
+ }
+
+ // TODO: PR to arrow
Review Comment:
```suggestion
// Can remove once https://github.com/apache/arrow-rs/pull/2031 is released
```
--
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-datafusion] alamb commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r912350722
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,123 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => add_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
}
}
+
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
Review Comment:
Perhaps we can add some unit tests in this module:
```rust
[#cfg(test)]
mod test {
[#test]
fn test_calculations() { ... }
}
```
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,123 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => add_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
}
}
+
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
+ let interval = interval as u128;
+ let months = (interval >> 96) as i32 * sign;
+ let days = (interval >> 64) as i32 * sign;
+ let nanos = interval as i64 * sign as i64;
+ let a = add_months(prior, months);
+ let b = a.add(Duration::days(days as i64));
+ let c = b.add(Duration::nanoseconds(nanos));
+ c
+}
+
+fn add_day_time(prior: NaiveDate, interval: i64, sign: i32) -> NaiveDate {
+ let interval = interval as u64;
+ let days = (interval >> 32) as i32 * sign;
+ let ms = interval as i32 * sign;
+ let intermediate = prior.add(Duration::days(days as i64));
+ let posterior = intermediate.add(Duration::milliseconds(ms as i64));
+ posterior
+}
+
+fn add_months(prior: NaiveDate, interval: i32) -> NaiveDate {
+ let target = chrono_add_months(prior, interval);
+ let target_plus = chrono_add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
+ NaiveDate::from_ymd(target.year(), target.month(), day)
+}
+
+fn chrono_add_months(dt: NaiveDate, delta: i32) -> NaiveDate {
Review Comment:
👍 - I like filing a ticket / PR in the target repo and then leaving a link in the comments of DataFusion
Btw I poked around and found this code in arrow2 (from @jorgecarleitao ) that is similar: https://docs.rs/arrow2/latest/src/arrow2/temporal_conversions.rs.html#342-368
--
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-datafusion] alamb commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1179298566
> @alamb , do you know where (in arrow-rs), I can PR these functions to:
I recommend https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/temporal.rs
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178259954
> or perhaps https://docs.rs/arrow/16.0.0/arrow/compute/kernels/arithmetic/fn.add.html
Okay, I think I understand this. Ideally `math_op` would look something like this: ```
pub fn math_op<LT, RT, F>(
left: &PrimitiveArray<LT>,
right: &PrimitiveArray<RT>,
op: F,
) -> Result<PrimitiveArray<LT>>
where
LT: ArrowNumericType,
RT: ArrowNumericType,
F: Fn(LT::Native, RT::Native) -> LT::Native,
{
```
So that we could add an array of `IntervalXXX` to and array of `DateXXX`
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r917139040
##########
datafusion/physical-expr/Cargo.toml:
##########
@@ -44,6 +44,7 @@ arrow = { version = "17.0.0", features = ["prettyprint"] }
blake2 = { version = "^0.10.2", optional = true }
blake3 = { version = "1.0", optional = true }
chrono = { version = "0.4", default-features = false }
+chronoutil = "0.2.3"
Review Comment:
I inlined it in my latest push. If we want to go back to including it in cargo.toml, it's an easy revert.
My $0.02 would be to leave it in cargo.toml, so that we can get updates if the author fixes something, and so that RAT checkers can see declaratively that we depend on that code, rather than trying to use heuristics of sematic code or binary diff.
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178178840
> Once that is done I can write up a "move this to arrow" type ticket
@alamb , do you know where (in arrow), I can PR these functions to:
- `fn create_day_time(days: i32, millis: i32) -> i64`
- `fn create_month_day_nano(months: i32, days: i32, nanos: i64) -> i128`
?
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r907767853
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
Oh my, I think this is incorrect and needs to be fixed: https://github.com/apache/arrow-datafusion/blob/7617d78809d4ff5bde31142e0744c70024e40635/datafusion/physical-expr/src/expressions/datetime.rs#L91
It's just grabbing the (low order?) 32 bits of the full 64 bit word, so it must only be operating on the time part and ignoring the date? I'll write some tests and fix it while I'm in 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-datafusion] codecov-commenter commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1166676580
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2797](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7cc9eaa) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/fed20457306f5327eb3d772651d070054ee786fb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fed2045) will **increase** coverage by `0.02%`.
> The diff coverage is `78.48%`.
```diff
@@ Coverage Diff @@
## master #2797 +/- ##
==========================================
+ Coverage 85.14% 85.16% +0.02%
==========================================
Files 273 273
Lines 48248 48298 +50
==========================================
+ Hits 41079 41133 +54
+ Misses 7169 7165 -4
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/common/src/scalar.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL3NjYWxhci5ycw==) | `74.94% <ø> (ø)` | |
| [...tafusion/physical-expr/src/expressions/datetime.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9kYXRldGltZS5ycw==) | `59.15% <66.66%> (+26.50%)` | :arrow_up: |
| [datafusion/core/tests/sql/timestamp.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9jb3JlL3Rlc3RzL3NxbC90aW1lc3RhbXAucnM=) | `100.00% <100.00%> (ø)` | |
| [datafusion/expr/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `73.71% <0.00%> (-0.20%)` | :arrow_down: |
| [datafusion/core/src/physical\_plan/metrics/value.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9wbGFuL21ldHJpY3MvdmFsdWUucnM=) | `87.43% <0.00%> (+0.50%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [fed2045...7cc9eaa](https://codecov.io/gh/apache/arrow-datafusion/pull/2797?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910184783
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -1963,10 +1963,10 @@ mod tests {
// Note that constant folder runs and folds the entire
// expression down to a single constant (true)
- let expected = "Projection: Date32(\"18636\") AS CAST(totimestamp(Utf8(\"2020-09-08T12:05:00+00:00\")) AS Date32) + IntervalDayTime(\"123\")\
- \n TableScan: test";
+ let expected = r#"Projection: Date32("18636") AS CAST(totimestamp(Utf8("2020-09-08T12:05:00+00:00")) AS Date32) + IntervalDayTime("528280977408")
+ TableScan: test"#;
let actual = get_optimized_plan_formatted(&plan, &time);
- assert_eq!(expected, actual);
+ assert_eq!(actual, expected);
Review Comment:
JetBrains products assume `actual` is on the left when visualizing diffs. The `assert_eq!` macro seems to not specify, so I updated it to look correct in my IDE so as to be less confusing.
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910183035
##########
datafusion/common/src/scalar.rs:
##########
@@ -87,11 +89,14 @@ pub enum ScalarValue {
TimestampMicrosecond(Option<i64>, Option<String>),
/// Timestamp Nanoseconds
TimestampNanosecond(Option<i64>, Option<String>),
- /// Interval with YearMonth unit
+ /// Number of elapsed whole months
IntervalYearMonth(Option<i32>),
- /// Interval with DayTime unit
+ /// Number of elapsed days and milliseconds (no leap seconds)
+ /// stored as 2 contiguous 32-bit signed integers
IntervalDayTime(Option<i64>),
- /// Interval with MonthDayNano unit
+ /// A triple of the number of elapsed months, days, and nanoseconds.
+ /// Months and days are encoded as 32-bit signed integers.
+ /// Nanoseconds is encoded as a 64-bit signed integer (no leap seconds).
Review Comment:
The docs are unfortunately ambiguous as to what order these words are stored in.
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178023067
Added unit tests. Will file PRs to respective dependency repos as well.
- chrono - I can figure out where to send the PR
- arrow - any idea what file / impl the functions listed as `TODO` should go into?
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910187839
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -74,88 +77,123 @@ impl PhysicalExpr for DateIntervalExpr {
self
}
- fn data_type(&self, input_schema: &Schema) -> datafusion_common::Result<DataType> {
+ fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
self.lhs.data_type(input_schema)
}
- fn nullable(&self, input_schema: &Schema) -> datafusion_common::Result<bool> {
+ fn nullable(&self, input_schema: &Schema) -> Result<bool> {
self.lhs.nullable(input_schema)
}
- fn evaluate(&self, batch: &RecordBatch) -> datafusion_common::Result<ColumnarValue> {
+ fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
- }
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(d)) => epoch.add(Duration::days(d as i64)),
+ ScalarValue::Date64(Some(ms)) => epoch.add(Duration::milliseconds(ms)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
+ };
+
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Invert sign for subtraction
+ let sign = match &self.op {
+ Operator::Plus => 1,
+ Operator::Minus => -1,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Do math
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(i)) => add_day_time(prior, *i, sign),
+ ScalarValue::IntervalYearMonth(Some(i)) => add_months(prior, *i * sign),
+ ScalarValue::IntervalMonthDayNano(Some(i)) => add_m_d_nano(prior, *i, sign),
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+
+ // convert back
+ let res = match operand {
+ ScalarValue::Date32(Some(_)) => {
+ let days = posterior.sub(epoch).num_days() as i32;
+ ColumnarValue::Scalar(ScalarValue::Date32(Some(days)))
+ }
+ ScalarValue::Date64(Some(_)) => {
+ let ms = posterior.sub(epoch).num_milliseconds();
+ ColumnarValue::Scalar(ScalarValue::Date64(Some(ms)))
+ }
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {}",
+ scalar
+ )))?,
+ };
+ Ok(res)
}
}
+
+fn add_m_d_nano(prior: NaiveDate, interval: i128, sign: i32) -> NaiveDate {
+ let interval = interval as u128;
+ let months = (interval >> 96) as i32 * sign;
+ let days = (interval >> 64) as i32 * sign;
+ let nanos = interval as i64 * sign as i64;
+ let a = add_months(prior, months);
+ let b = a.add(Duration::days(days as i64));
+ let c = b.add(Duration::nanoseconds(nanos));
+ c
+}
+
+fn add_day_time(prior: NaiveDate, interval: i64, sign: i32) -> NaiveDate {
+ let interval = interval as u64;
+ let days = (interval >> 32) as i32 * sign;
+ let ms = interval as i32 * sign;
+ let intermediate = prior.add(Duration::days(days as i64));
+ let posterior = intermediate.add(Duration::milliseconds(ms as i64));
+ posterior
+}
+
+fn add_months(prior: NaiveDate, interval: i32) -> NaiveDate {
+ let target = chrono_add_months(prior, interval);
+ let target_plus = chrono_add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
+ NaiveDate::from_ymd(target.year(), target.month(), day)
+}
+
+fn chrono_add_months(dt: NaiveDate, delta: i32) -> NaiveDate {
Review Comment:
I still think I would like to PR this to chrono when I get a chance, and we can remove it from datafusion.
--
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-datafusion] alamb commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1179299347
For what it is worth https://github.com/apache/arrow-rs/compare/master...spaceandtimelabs:arrow-rs:bg_date_math looks pretty good to me 👍
--
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-datafusion] thinkharderdev commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
thinkharderdev commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r917098113
##########
datafusion/physical-expr/Cargo.toml:
##########
@@ -44,6 +44,7 @@ arrow = { version = "17.0.0", features = ["prettyprint"] }
blake2 = { version = "^0.10.2", optional = true }
blake3 = { version = "1.0", optional = true }
chrono = { version = "0.4", default-features = false }
+chronoutil = "0.2.3"
Review Comment:
I haven't reviewed the PR so am not sure how much of that create is used but I agree if it is a small amount of code it is better to just copy the code rather than add another dependency.
--
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-datafusion] alamb commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r907668027
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
Review Comment:
It would b great to eventually put some/all of this logic into the arrow-rs kernels -- for example
https://docs.rs/arrow/16.0.0/arrow/compute/kernels/temporal/index.html
or perhaps https://docs.rs/arrow/16.0.0/arrow/compute/kernels/arithmetic/fn.add.html
That would also likely result in support for columnar execution support (aka adding a column of integers)
Maybe we can (I will file a ticket) start with kernels in datafusion and then port them to arrow.rs
##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -814,3 +814,83 @@ async fn group_by_timestamp_millis() -> Result<()> {
assert_batches_eq!(expected, &actual);
Ok(())
}
+
+#[tokio::test]
+async fn interval_year() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-01' + interval '1' year as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1995-01-01 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn add_interval_month() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-31' + interval '1' month as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1994-02-28 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn sub_interval_month() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-03-31' - interval '1' month as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1994-02-28 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn sub_month_wrap() -> Result<()> {
Review Comment:
nice
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
I don't really understand this logic -- I would have expected that code would split the interval into the 3 three fields (month, day, nano) and then use the various Chrono operations to do the timestamp arithmetic
https://github.com/apache/arrow/blob/master/format/Schema.fbs#L354-L375
##########
datafusion/common/src/scalar.rs:
##########
@@ -75,9 +76,9 @@ pub enum ScalarValue {
LargeBinary(Option<Vec<u8>>),
/// list of nested ScalarValue
List(Option<Vec<ScalarValue>>, Box<DataType>),
- /// Date stored as a signed 32bit int
+ /// Date stored as a signed 32bit int days since UNIX epoch 1970-01-01
Date32(Option<i32>),
- /// Date stored as a signed 64bit int
+ /// Date stored as a signed 64bit int days since UNIX epoch 1970-01-01
Review Comment:
```suggestion
/// Date stored as a signed 64bit int milliseconds since UNIX epoch 1970-01-01
```
I think 64bits are milliseconds rather than days -- format reference: https://github.com/apache/arrow/blob/master/format/Schema.fbs#L202-L210
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910186338
##########
datafusion/optimizer/src/simplify_expressions.rs:
##########
@@ -1951,7 +1951,7 @@ mod tests {
let date_plus_interval_expr = to_timestamp_expr(ts_string)
.cast_to(&DataType::Date32, schema)
.unwrap()
- + Expr::Literal(ScalarValue::IntervalDayTime(Some(123)));
Review Comment:
Based upon tests that used the parser to load "real" dates seemed to confirm the lower-order bytes are the `ms` part, so this was adding `123ms` which wasn't enough to bump the 32 bit date forward. Since the parser based tests are more likely to be correct than this hand-rolled one, I updated this to match that.
--
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-datafusion] alamb commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r907787029
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
Thank you @avantgardnerio
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178313163
I think I see how to PR this into the arrow kernels. I'll try that shortly.
--
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-datafusion] alamb commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r917090447
##########
datafusion/physical-expr/Cargo.toml:
##########
@@ -44,6 +44,7 @@ arrow = { version = "17.0.0", features = ["prettyprint"] }
blake2 = { version = "^0.10.2", optional = true }
blake3 = { version = "1.0", optional = true }
chrono = { version = "0.4", default-features = false }
+chronoutil = "0.2.3"
Review Comment:
I am somewhat concerned about adding dependencies (and this particular crate seems to be maintained by a single person) -- however, since it is so small (and MIT licensced), we could also just inline the functions we care about.
Any thoughts @andygrove or @thinkharderdev ?
https://github.com/olliemath/chronoutil/blob/main/src/delta.rs
--
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-datafusion] andygrove merged pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r907718292
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
When the type is `IntervalDayTime`, I think I copied the behavior of the existing code, which was to grab a 32-bit number, which appears to represent the number of days based on this snippet https://github.com/apache/arrow-datafusion/blob/7617d78809d4ff5bde31142e0744c70024e40635/datafusion/physical-expr/src/expressions/datetime.rs#L122 (I assume there were tests around this? and if so they still pass).
For `IntervalYearMonth` I guessed and was correct based on the documentation you linked (ty!):
```
YEAR_MONTH - Indicates the number of elapsed whole months
```
I wasn't clear on what `IntervalMonthDayNano` meant, but now I do thanks to your link! This PR leaves it unimplemented (no change in behavior). It isn't critical to passing the TPC-H benchmarks, so it wasn't a priority for me at this moment.
As for the actual business logic of `IntervalYearMonth`, it is always stored as a number of months (year=12), and it turns out that adding (and subtracting) months is not very straight forward, and not implemented in chrono https://stackoverflow.com/questions/64081289/how-do-i-add-a-month-to-a-chrono-naivedate . When I looked into other implementations, they appeared to have issues (i.e. not supporting negative arguments), so I wrote this.
Unfortunately, one can't just add a fixed time unit (months have different numbers of days), and one has to know what is being added to in order to add properly (some Februaries have 29 days, others only 28). Observing postgres behavior, days of the month get clamped `2022-01-31` + 1 month = `2022-02-28`, so this logic is attempting to reproduce that behavior.
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r907722613
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
Reflecting, there could be a valid argument to PR chrono :thinking:
--
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-datafusion] alamb commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1182217556
🐱 🚀
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910182307
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -86,76 +89,114 @@ impl PhysicalExpr for DateIntervalExpr {
let dates = self.lhs.evaluate(batch)?;
let intervals = self.rhs.evaluate(batch)?;
- let interval = match intervals {
- ColumnarValue::Scalar(interval) => match interval {
- ScalarValue::IntervalDayTime(Some(interval)) => interval as i32,
- ScalarValue::IntervalYearMonth(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalYearMonth".to_string(),
- ))
- }
- ScalarValue::IntervalMonthDayNano(Some(_)) => {
- return Err(DataFusionError::Execution(
- "DateIntervalExpr does not support IntervalMonthDayNano"
- .to_string(),
- ))
- }
- other => {
- return Err(DataFusionError::Execution(format!(
- "DateIntervalExpr does not support non-interval type {:?}",
- other
- )))
- }
- },
- _ => {
- return Err(DataFusionError::Execution(
- "Columnar execution is not yet supported for DateIntervalExpr"
- .to_string(),
- ))
+ // Unwrap days since epoch
+ let operand = match dates {
+ ColumnarValue::Scalar(scalar) => scalar,
+ _ => Err(DataFusionError::Execution(
+ "Columnar execution is not yet supported for DateIntervalExpr"
+ .to_string(),
+ ))?,
+ };
+
+ // Convert to NaiveDate
+ let epoch = NaiveDate::from_ymd(1970, 1, 1);
+ let prior = match operand {
+ ScalarValue::Date32(Some(date)) => {
+ epoch.add(chrono::Duration::days(date as i64))
}
+ ScalarValue::Date64(Some(date)) => epoch.add(chrono::Duration::days(date)),
+ _ => Err(DataFusionError::Execution(format!(
+ "Invalid lhs type for DateIntervalExpr: {:?}",
+ operand
+ )))?,
};
- match dates {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Date32(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date + interval),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date32(
- Some(date - interval),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- ScalarValue::Date64(Some(date)) => match &self.op {
- Operator::Plus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date + interval as i64),
- ))),
- Operator::Minus => Ok(ColumnarValue::Scalar(ScalarValue::Date64(
- Some(date - interval as i64),
- ))),
- _ => {
- // this should be unreachable because we check the operators in `try_new`
- Err(DataFusionError::Execution(
- "Invalid operator for DateIntervalExpr".to_string(),
- ))
- }
- },
- _ => {
- // this should be unreachable because we check the types in `try_new`
- Err(DataFusionError::Execution(
- "Invalid lhs type for DateIntervalExpr".to_string(),
- ))
- }
- },
+ // Unwrap interval to add
+ let scalar = match &intervals {
+ ColumnarValue::Scalar(interval) => interval,
_ => Err(DataFusionError::Execution(
"Columnar execution is not yet supported for DateIntervalExpr"
.to_string(),
- )),
- }
+ ))?,
+ };
+
+ // Negate for subtraction
+ let interval = match &scalar {
+ ScalarValue::IntervalDayTime(Some(interval)) => *interval,
+ ScalarValue::IntervalYearMonth(Some(interval)) => *interval as i64,
+ ScalarValue::IntervalMonthDayNano(Some(_interval)) => {
+ Err(DataFusionError::Execution(
+ "DateIntervalExpr does not support IntervalMonthDayNano".to_string(),
+ ))?
+ }
+ other => Err(DataFusionError::Execution(format!(
+ "DateIntervalExpr does not support non-interval type {:?}",
+ other
+ )))?,
+ };
+ let interval = match &self.op {
+ Operator::Plus => interval,
+ Operator::Minus => -interval,
+ _ => {
+ // this should be unreachable because we check the operators in `try_new`
+ Err(DataFusionError::Execution(
+ "Invalid operator for DateIntervalExpr".to_string(),
+ ))?
+ }
+ };
+
+ // Add interval
+ let posterior = match scalar {
+ ScalarValue::IntervalDayTime(Some(_)) => {
+ prior.add(chrono::Duration::days(interval))
+ }
+ ScalarValue::IntervalYearMonth(Some(_)) => {
+ let target = add_months(prior, interval);
+ let target_plus = add_months(target, 1);
+ let last_day = target_plus.sub(chrono::Duration::days(1));
+ let day = min(prior.day(), last_day.day());
Review Comment:
I implemented `IntervalDayTime` and `IntervalMonthDayNano`, but could only figure out how to test `IntervalDayTime`, since I need a `timestamp` to test < 1 day resolutions, and it didn't seem like the timestamp path was plumbed or was plumbing it in scope for this PR.
--
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-datafusion] avantgardnerio commented on a diff in pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r910183727
##########
datafusion/core/tests/sql/timestamp.rs:
##########
@@ -814,3 +814,123 @@ async fn group_by_timestamp_millis() -> Result<()> {
assert_batches_eq!(expected, &actual);
Ok(())
}
+
+#[tokio::test]
+async fn interval_year() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-01' + interval '1' year as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1995-01-01 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn add_interval_month() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-31' + interval '1' month as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1994-02-28 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn sub_interval_month() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-03-31' - interval '1' month as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1994-02-28 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn sub_month_wrap() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-15' - interval '1' month as date;";
+ let results = execute_to_batches(&ctx, sql).await;
+
+ let expected = vec![
+ "+------------+",
+ "| date |",
+ "+------------+",
+ "| 1993-12-15 |",
+ "+------------+",
+ ];
+
+ assert_batches_eq!(expected, &results);
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn add_interval_day() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "select date '1994-01-15' + interval '1' day as date;";
Review Comment:
`interval 1 day` seemed to cause it to use the `IntervalDayTime` type.
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1176425555
Sorry this stalled out. I was really close on the subqueries, and I think I have them all working now, so I'll jump back on this.
--
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-datafusion] avantgardnerio commented on pull request #2797: Add support for month & year intervals
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2797:
URL: https://github.com/apache/arrow-datafusion/pull/2797#issuecomment-1178437642
What this is looking like when done inside `arrow-rs`: https://github.com/apache/arrow-rs/compare/master...spaceandtimelabs:arrow-rs:bg_date_math
--
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