You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cht42 (via GitHub)" <gi...@apache.org> on 2026/01/09 12:12:20 UTC
[PR] feat(spark): implement add_months function [datafusion]
cht42 opened a new pull request, #19711:
URL: https://github.com/apache/datafusion/pull/19711
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Part of #15914
- Closes #19710
## Rationale for this change
Implementation of spark `add_months` function.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
yes
## Are there any user-facing changes?
yes
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2675971508
##########
datafusion/sqllogictest/test_files/spark/datetime/date_add.slt:
##########
@@ -41,7 +41,7 @@ SELECT date_add('2016-07-30'::date, arrow_cast(1, 'Int8'));
2016-07-31
query D
-SELECT date_sub('2016-07-30'::date, 0::int);
+SELECT date_add('2016-07-30'::date, 0::int);
Review Comment:
caught this issue
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2678429548
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
Oh this is interesting; panicking is definitely not good, and I can reproduce that on main too:
```sql
DataFusion CLI v51.0.0
> select '2016-07-30'::date + arrow_cast(2147483647::int, 'Interval(YearMonth)');
thread 'main' (15797910) panicked at /Users/jeffrey/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/chrono-0.4.42/src/naive/date/mod.rs:2005:41:
`NaiveDate + Months` out of range
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
We should raise this as a separate issue as this should be erroring not panicking
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2703686463
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
@Jefffrey should we merge this and add the test back after next arrow upgrade ? or wait for the upgrade first ?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2678418948
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
saprk will error with `ARITHMETIC_OVERFLOW`. I tried adding the following test case `SELECT add_months('2016-07-30'::date, 2147483647::int);` and im getting `External error: task 35 panicked with message "`NaiveDate + Months` out of range"`. So in a way we have the same behavior as spark but its not testable ?
##########
datafusion/sqllogictest/test_files/spark/datetime/date_add.slt:
##########
@@ -51,20 +51,15 @@ SELECT date_add('2016-07-30'::date, 2147483647::int)::int;
-2147466637
query I
-SELECT date_sub('1969-01-01'::date, 2147483647::int)::int;
+SELECT date_add('1969-01-01'::date, 2147483647::int)::int;
----
-2147483284
+2147483282
query D
SELECT date_add('2016-07-30'::date, 100000::int);
----
2290-05-15
-query D
-SELECT date_sub('2016-07-30'::date, 100000::int);
-----
-1742-10-15
Review Comment:
updated
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2676319850
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
Does spark have a behaviour for overflow/result date too large that we need to consider?
##########
datafusion/sqllogictest/test_files/spark/datetime/date_add.slt:
##########
@@ -51,20 +51,15 @@ SELECT date_add('2016-07-30'::date, 2147483647::int)::int;
-2147466637
query I
-SELECT date_sub('1969-01-01'::date, 2147483647::int)::int;
+SELECT date_add('1969-01-01'::date, 2147483647::int)::int;
----
-2147483284
+2147483282
query D
SELECT date_add('2016-07-30'::date, 100000::int);
----
2290-05-15
-query D
-SELECT date_sub('2016-07-30'::date, 100000::int);
-----
-1742-10-15
Review Comment:
Maybe we can copy the date_sub ones into date_sub.slt?
##########
datafusion/spark/src/function/datetime/add_months.rs:
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::ops::Add;
+use std::sync::Arc;
+
+use arrow::datatypes::{DataType, Field, FieldRef, IntervalUnit};
+use datafusion_common::utils::take_function_args;
+use datafusion_common::{Result, internal_err};
+use datafusion_expr::simplify::{ExprSimplifyResult, SimplifyContext};
+use datafusion_expr::{
+ Cast, ColumnarValue, Expr, ReturnFieldArgs, ScalarFunctionArgs, ScalarUDFImpl,
+ Signature, Volatility,
+};
+
+/// <https://spark.apache.org/docs/latest/api/sql/index.html#add_months>
+#[derive(Debug, PartialEq, Eq, Hash)]
+pub struct SparkAddMonths {
+ signature: Signature,
+}
+
+impl Default for SparkAddMonths {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl SparkAddMonths {
+ pub fn new() -> Self {
+ Self {
+ signature: Signature::exact(
+ vec![DataType::Date32, DataType::Int32],
+ Volatility::Immutable,
+ ),
+ }
+ }
+}
+
+impl ScalarUDFImpl for SparkAddMonths {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn name(&self) -> &str {
+ "add_months"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ internal_err!("return_field_from_args should be used instead")
+ }
+
+ fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> {
+ let nullable = args.arg_fields.iter().any(|f| f.is_nullable())
+ || args
+ .scalar_arguments
+ .iter()
+ .any(|arg| matches!(arg, Some(sv) if sv.is_null()));
Review Comment:
```suggestion
let nullable = args.arg_fields.iter().any(|f| f.is_nullable());
```
If an argument is a scalar null then its corresponding field should already be nullable
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#issuecomment-3775972028
Thanks @cht42
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2703895609
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
done
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2703694158
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
Let's comment out the test so we can merge this PR; can re-enable the test later when the upstream fix comes 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #19711:
URL: https://github.com/apache/datafusion/pull/19711
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2678462241
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
traced it back to https://github.com/chronotope/chrono/blob/1c0b8f011ab2f2e53c195df1866a1fb4c7fd193a/src/naive/date/mod.rs#L2034
```
0: __rustc::rust_begin_unwind
at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/std/src/panicking.rs:698:5
1: core::panicking::panic_fmt
at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/core/src/panicking.rs:80:14
2: core::panicking::panic_display
at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/core/src/panicking.rs:264:5
3: core::option::expect_failed
at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/core/src/option.rs:2183:5
4: core::option::Option<T>::expect
at /Users/chuet/.rustup/toolchains/1.92.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:970:21
5: <chrono::naive::date::NaiveDate as core::ops::arith::Add<chrono::month::Months>>::add
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/chrono-0.4.42/src/naive/date/mod.rs:2005:41
6: arrow_array::delta::shift_months
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-array-57.1.0/src/delta.rs:36:30
7: arrow_array::types::Date32Type::add_year_months
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-array-57.1.0/src/types.rs:934:25
8: <arrow_array::types::Date32Type as arrow_arith::numeric::DateOp>::add_year_month
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/numeric.rs:554:12
9: arrow_arith::numeric::date_op::{{closure}}
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/numeric.rs:775:47
10: arrow_arith::arity::try_binary_no_nulls
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/arity.rs:385:35
11: arrow_arith::arity::try_binary
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/arity.rs:274:9
12: arrow_arith::numeric::date_op
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/numeric.rs:295:46
13: arrow_arith::numeric::arithmetic_op
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/numeric.rs:249:24
14: arrow_arith::numeric::add_wrapping
at /Users/chuet/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-arith-57.1.0/src/numeric.rs:40:5
15: core::ops::function::Fn::call
at /Users/chuet/.rustup/toolchains/1.92.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:79:5
16: datafusion_physical_expr_common::datum::apply
at /Users/chuet/Projects/foundry/datafusion/datafusion/physical-expr-common/src/datum.rs:51:25
17: <datafusion_physical_expr::expressions::binary::BinaryExpr as datafusion_physical_expr_common::physical_expr::PhysicalExpr>::evaluate
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2678462241
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
traced it back to https://github.com/chronotope/chrono/blob/1c0b8f011ab2f2e53c195df1866a1fb4c7fd193a/src/naive/date/mod.rs#L2034
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(spark): implement add_months function [datafusion]
Posted by "cht42 (via GitHub)" <gi...@apache.org>.
cht42 commented on code in PR #19711:
URL: https://github.com/apache/datafusion/pull/19711#discussion_r2681469422
##########
datafusion/sqllogictest/test_files/spark/datetime/add_months.slt:
##########
@@ -15,13 +15,38 @@
# specific language governing permissions and limitations
# under the License.
Review Comment:
opened https://github.com/apache/arrow-rs/pull/9144 to fix the panic in arrow
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org