Skip to content

feat(spark): implement add_months function #19711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
cht42 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(spark): implement add_months function #19711

cht42 wants to merge 3 commits into from
+149 −18

Conversation

@cht42
Copy link
Contributor

@cht42 cht42 commented Jan 9, 2026

Which issue does this PR close?

Rationale for this change

Implementation of spark add_months function.

What changes are included in this PR?

Are these changes tested?

yes

Are there any user-facing changes?

yes

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) spark labels Jan 9, 2026
cht42
cht42 commented Jan 9, 2026
View reviewed changes
datafusion/sqllogictest/test_files/spark/datetime/date_add.slt

query D
SELECT date_sub('2016-07-30'::date, 0::int);
SELECT date_add('2016-07-30'::date, 0::int);
Copy link
Contributor Author

@cht42 cht42 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caught this issue

Jefffrey
Jefffrey reviewed Jan 9, 2026
View reviewed changes
datafusion/spark/src/function/datetime/add_months.rs
Comment on lines +72 to +76
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()));
Copy link
Contributor

@Jefffrey Jefffrey Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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()));
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

datafusion/sqllogictest/test_files/spark/datetime/date_add.slt
Comment on lines -54 to -66
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
Copy link
Contributor

@Jefffrey Jefffrey Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can copy the date_sub ones into date_sub.slt?

Copy link
Contributor Author

@cht42 cht42 Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

datafusion/sqllogictest/test_files/spark/datetime/add_months.slt
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

Copy link
Contributor

@Jefffrey Jefffrey Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does spark have a behaviour for overflow/result date too large that we need to consider?

Copy link
Contributor Author

@cht42 cht42 Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@Jefffrey Jefffrey Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is interesting; panicking is definitely not good, and I can reproduce that on main too:

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

Copy link
Contributor Author

@cht42 cht42 Jan 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jefffrey Jefffrey Jefffrey left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

spark sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[datafusion-spark]: Implement add_months function

2 participants

@cht42 @Jefffrey