refactor: streamline date64 tests #9165

cht42 · 2026-01-14T04:49:12Z

Which issue does this PR close?

Closes #N/A (internal refactoring - no issue)

Rationale for this change

Noticed while writing tests in #9144, that the current tests could be re-written to be easier to read/re-use.

⚠️ FIY, I used claude to refactor those tests, I read the changes and we are keeping the same test cases.

The Date64 boundary tests in arrow-arith/src/numeric.rs had significant code duplication. Each test function for Date64 operations (to_naive_date_opt, add_year_months_opt, subtract_year_months_opt, add_day_time_opt, subtract_day_time_opt, add_month_day_nano_opt, subtract_month_day_nano_opt) repeated similar setup code and boundary checks, making the test suite harder to maintain and extend.

What changes are included in this PR?

This PR refactors the Date64 boundary tests by:

Introducing shared constants for commonly used values:
- MAX_VALID_DATE, MIN_VALID_DATE, EPOCH - NaiveDate constants for chrono's valid date range
Adding utility functions to reduce repetition:
- date_to_millis(year, month, day) - converts a date to milliseconds from epoch
- max_valid_millis(), min_valid_millis(), year_2000_millis() - common millisecond values
Consolidating similar test patterns into parameterized helper functions:
- test_year_month_op() - tests add_year_months_opt and subtract_year_months_opt
- test_day_time_op() - tests add_day_time_opt and subtract_day_time_opt
- test_month_day_nano_op() - tests add_month_day_nano_opt and subtract_month_day_nano_opt
Reducing 8 separate test functions to 4 while maintaining the same test coverage

Net result: -297 lines (163 added, 460 removed) with equivalent functionality.

Are these changes tested?

Yes - this is a refactoring of existing tests. The same boundary conditions and edge cases are still tested, just organized more efficiently. Running cargo test confirms all tests pass.

Are there any user-facing changes?

No. This is an internal test refactoring with no changes to public APIs or functionality.

…d introducing utility functions

cht42 · 2026-01-14T04:49:33Z

re-iterating here, that I used claude to refactor those tests, I read the changes and we are keeping the same test cases.

cht42 · 2026-01-14T04:50:19Z

@scovich can you review ?

Jefffrey · 2026-01-14T09:05:18Z

arrow-arith/src/numeric.rs

-            Date64Type::to_naive_date_opt(i64::MIN).is_none(),
-            "i64::MIN should return None"
-        );
+    fn date_to_millis(year: i32, month: u32, day: u32) -> i64 {


I feel these can be made constants as well; for example:

const MAX_VALID_MILLIS: i64 = MAX_VALID_DATE .signed_duration_since(EPOCH) .num_milliseconds(); const MIN_VALID_MILLIS: i64 = MIN_VALID_DATE .signed_duration_since(EPOCH) .num_milliseconds(); const YEAR_2000_MILLIS: i64 = date_to_millis(2000, 1, 1); const fn date_to_millis(year: i32, month: u32, day: u32) -> i64 { let date = NaiveDate::from_ymd_opt(year, month, day).unwrap(); date.signed_duration_since(EPOCH).num_milliseconds() }

Then can simplify their usages in the functions by removing the function call to a variable and just use constant directly.

scovich

The approach looks very nice.

⚠️ FYI, I used claude to refactor those tests, I read the changes and we are keeping the same test cases.

Great use for an LLM IMO!

One issue tho, which I also failed to notice when eyeballing the changes, but which claude pointed out when I asked it to check the before/after tests for equivalence:

The original tests for subtract_day_time_opt and subtract_month_day_nano_opt specifically tested:

Subtracting a positive interval from i64::MIN (should fail going more negative)

Subtracting a negative interval from i64::MAX (should fail going more positive)

The refactored versions test:

Subtracting a positive interval from i64::MAX (mathematically valid direction but extreme input)

Subtracting a negative interval from i64::MIN (mathematically valid direction but extreme input)

It suggested three possible ways to address this (favoring 2/):

Update the test logic to be add/sub aware and test the original edge cases
Expand the test logic to test both the original and new edge cases
Accept that the negative test "succeeds" (both BEFORE and AFTER) because we can't create a date from i64::MIN/MAX in the first place -- so it doesn't matter what interval we pass because the test will never get that far.

However, 3/ raised red flags for me: There are already tests to exercise physical (integer) overflow, and based on the comments here I suspect that the original test author intended to test logical overflow, where the interval shifts a valid date in a "bad" direction that takes it out of gamut. If so, the original tests were broken and the refactored ones should actually be working with MIN/MAX_VALID_MILLIS instead of ia64::MIN/MAX. At which point it would be important to also apply 1/ from the list above.

It also raises the question of whether we should use ? for intermediate operations like the date creation, or if unwrap is more appropriate to be sure the test failed/succeeded for the reason we think it did?

AndreaBozzo · 2026-01-14T15:34:08Z

The approach looks very nice.

⚠️ FYI, I used claude to refactor those tests, I read the changes and we are keeping the same test cases.

Great use for an LLM IMO!

One issue tho, which I also failed to notice when eyeballing the changes, but which claude pointed out when I asked it to check the before/after tests for equivalence:

The original tests for subtract_day_time_opt and subtract_month_day_nano_opt specifically tested:

Subtracting a positive interval from i64::MIN (should fail going more negative)

Subtracting a negative interval from i64::MAX (should fail going more positive)

The refactored versions test:

Subtracting a positive interval from i64::MAX (mathematically valid direction but extreme input)

Subtracting a negative interval from i64::MIN (mathematically valid direction but extreme input)

It suggested three possible ways to address this (favoring 2/):
1. Update the test logic to be add/sub aware and test the original edge cases

2. Expand the test logic to test both the original and new edge cases

3. Accept that the negative test "succeeds" (both BEFORE and AFTER) because we can't create a date from i64::MIN/MAX in the first place -- so it doesn't matter what interval we pass because the test will never get that far.
However, 3/ raised red flags for me: There are already tests to exercise physical (integer) overflow, and based on the comments here I suspect that the original test author intended to test logical overflow, where the interval shifts a valid date in a "bad" direction that takes it out of gamut. If so, the original tests were broken and the refactored ones should actually be working with MIN/MAX_VALID_MILLIS instead of ia64::MIN/MAX. At which point it would be important to also apply 1/ from the list above.

It also raises the question of whether we should use ? for intermediate operations like the date creation, or if unwrap is more appropriate to be sure the test failed/succeeded for the reason we think it did?

Hi, sorry for the intrusion, i approved this PR because the refactoring itself is clean and the core test coverage is preserved.

Regarding scovich's observation about the subtle difference in edge case testing (i64::MIN/MAX with positive/negative intervals): since neither i64::MIN nor i64::MAX can be converted to a valid date in the first place, the test fails at date creation before the interval logic is even reached, the behavior is effectively the same (?)

That said, if the original intent was to test logical overflow (a valid date shifted out of range), then both the original and refactored tests miss that case. Testing with MIN_VALID_MILLIS/MAX_VALID_MILLIS instead would be a nice improvement i super agree, even in separate pr.

This would also be a great "good first issue" for newcomers to the repo (like myself 😄) who want to improve test coverage.

cht42 · 2026-01-14T17:02:26Z

@scovich this part here is testing the close to limit -> going over limit cases
https://github.com/apache/arrow-rs/pull/9165/changes#diff-94fd6c08249d7fd9b2364e8b8cd66163e6d0e0c0a8d04007b7a693dfd3e919d5R1745-R1763

cht42 · 2026-01-14T17:04:04Z

arrow-arith/src/numeric.rs

+        // Large intervals that would overflow - behavior differs for add vs subtract
+        // For add: near_max + huge_days overflows, near_min + huge_neg_days overflows
+        // For subtract: near_min - huge_days overflows, near_max - huge_neg_days overflows
+        if is_subtract {


here @scovich (linking is behaving weirdly for me)

Ah nice. So we do have the coverage. Is the other test in question even useful then? Or do we just leave it as-is?

removed, you're right it wasn't useful.

alamb

Thanks @cht42 and @scovich -- this is a weird set of tests (just for overflow rather than the actual values, but I agree this is much better than what was here before)

alamb · 2026-01-14T22:14:54Z

arrow-arith/src/numeric.rs

-
-        // Test with extreme input values that would cause overflow
-        assert!(
-            Date64Type::add_year_months_opt(i64::MAX, 1).is_none(),


I didn't see any corresponding coverage for i64::MIN and i64::MAX in the helpers but then I see the discussion between @scovich and @cht42 about this point and I think it makes sense to avoid this

# Which issue does this PR close? - Closes #N/A (internal refactoring - no issue) # Rationale for this change Noticed while writing tests in apache#9144, that the current tests could be re-written to be easier to read/re-use. ⚠️ FIY, I used claude to refactor those tests, I read the changes and we are keeping the same test cases. The Date64 boundary tests in `arrow-arith/src/numeric.rs` had significant code duplication. Each test function for Date64 operations (`to_naive_date_opt`, `add_year_months_opt`, `subtract_year_months_opt`, `add_day_time_opt`, `subtract_day_time_opt`, `add_month_day_nano_opt`, `subtract_month_day_nano_opt`) repeated similar setup code and boundary checks, making the test suite harder to maintain and extend. # What changes are included in this PR? This PR refactors the Date64 boundary tests by: 1. **Introducing shared constants** for commonly used values: - `MAX_VALID_DATE`, `MIN_VALID_DATE`, `EPOCH` - NaiveDate constants for chrono's valid date range 2. **Adding utility functions** to reduce repetition: - `date_to_millis(year, month, day)` - converts a date to milliseconds from epoch - `max_valid_millis()`, `min_valid_millis()`, `year_2000_millis()` - common millisecond values 3. **Consolidating similar test patterns** into parameterized helper functions: - `test_year_month_op()` - tests `add_year_months_opt` and `subtract_year_months_opt` - `test_day_time_op()` - tests `add_day_time_opt` and `subtract_day_time_opt` - `test_month_day_nano_op()` - tests `add_month_day_nano_opt` and `subtract_month_day_nano_opt` 4. **Reducing 8 separate test functions to 4** while maintaining the same test coverage Net result: **-297 lines** (163 added, 460 removed) with equivalent functionality. # Are these changes tested? Yes - this is a refactoring of existing tests. The same boundary conditions and edge cases are still tested, just organized more efficiently. Running `cargo test` confirms all tests pass. # Are there any user-facing changes? No. This is an internal test refactoring with no changes to public APIs or functionality. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

refactor: streamline date64 tests by consolidating boundary checks an…

578d4f9

…d introducing utility functions

github-actions bot added the arrow Changes to the arrow crate label Jan 14, 2026

cht42 mentioned this pull request Jan 14, 2026

Avoid panic on Date32 overflow #9144

Open

Jefffrey reviewed Jan 14, 2026

View reviewed changes

better const in tests

2908e6a

AndreaBozzo approved these changes Jan 14, 2026

View reviewed changes

scovich reviewed Jan 14, 2026

View reviewed changes

cht42 commented Jan 14, 2026

View reviewed changes

cht42 and others added 2 commits January 14, 2026 22:14

update tests

efd766f

Merge branch 'main' into refactor-date64-tests

07848dd

alamb approved these changes Jan 14, 2026

View reviewed changes

alamb merged commit 989044c into apache:main Jan 14, 2026
13 checks passed

refactor: streamline date64 tests #9165

refactor: streamline date64 tests #9165

Uh oh!

Conversation

cht42 commented Jan 14, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

cht42 commented Jan 14, 2026

Uh oh!

cht42 commented Jan 14, 2026

Uh oh!

Jefffrey Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

cht42 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

scovich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreaBozzo commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cht42 commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cht42 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

scovich Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

cht42 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scovich left a comment •

edited

Loading

AndreaBozzo commented Jan 14, 2026 •

edited

Loading

cht42 commented Jan 14, 2026 •

edited

Loading