[GitHub] [arrow-rs] waitingkuo commented on a diff in pull request #3132: use cargo add/sub months

[GitBox <gi...@apache.org>]

waitingkuo commented on code in PR #3132:
URL: https://github.com/apache/arrow-rs/pull/3132#discussion_r1025946084


##########
arrow-array/src/delta.rs:
##########
@@ -23,86 +23,33 @@
 // Copied from chronoutil crate
 
 //! Contains utility functions for shifting Date objects.
-use chrono::Datelike;
-
-/// Returns true if the year is a leap-year, as naively defined in the Gregorian calendar.
-#[inline]
-pub(crate) fn is_leap_year(year: i32) -> bool {
-    year % 4 == 0 && (year % 100 != 0 || year % 400 == 0)
-}
-
-// If the day lies within the month, this function has no effect. Otherwise, it shifts
-// day backwards to the final day of the month.
-// XXX: No attempt is made to handle days outside the 1-31 range.
-#[inline]
-fn normalise_day(year: i32, month: u32, day: u32) -> u32 {
-    if day <= 28 {
-        day
-    } else if month == 2 {
-        28 + is_leap_year(year) as u32
-    } else if day == 31 && (month == 4 || month == 6 || month == 9 || month == 11) {
-        30
-    } else {
-        day
-    }
-}
+use chrono::{Datelike, Months};
 
 /// Shift a date by the given number of months.
-/// Ambiguous month-ends are shifted backwards as necessary.
-pub(crate) fn shift_months<D: Datelike>(date: D, months: i32) -> D {
-    let mut year = date.year() + (date.month() as i32 + months) / 12;
-    let mut month = (date.month() as i32 + months) % 12;
-    let mut day = date.day();
-
-    if month < 1 {
-        year -= 1;
-        month += 12;
-    }
-
-    day = normalise_day(year, month as u32, day);
-
-    // This is slow but guaranteed to succeed (short of interger overflow)
-    if day <= 28 {
-        date.with_day(day)
-            .unwrap()
-            .with_month(month as u32)
-            .unwrap()
-            .with_year(year)
-            .unwrap()
+pub(crate) fn shift_months<
+    D: Datelike
+        + std::ops::Add<chrono::Months, Output = D>
+        + std::ops::Sub<chrono::Months, Output = D>,
+>(
+    date: D,
+    months: i32,
+) -> D {
+    if months == 0 {
+        date
+    } else if months > 0 {
+        date + Months::new(months as u32)
     } else {
-        date.with_day(1)
-            .unwrap()
-            .with_month(month as u32)
-            .unwrap()
-            .with_year(year)
-            .unwrap()
-            .with_day(day)
-            .unwrap()
+        date - Months::new(-months as u32)
     }
 }

Review Comment:
   Months's new constructor only support u32
   https://docs.rs/chrono/latest/chrono/struct.Months.html#method.new
   
   so i keep the function `shift_month` (which support i32 instead) as a wrapper of chrono's add/sub months



##########
arrow-array/Cargo.toml:
##########
@@ -48,7 +48,7 @@ ahash = { version = "0.8", default-features = false, features = ["runtime-rng"]
 arrow-buffer = { version = "27.0.0", path = "../arrow-buffer" }
 arrow-schema = { version = "27.0.0", path = "../arrow-schema" }
 arrow-data = { version = "27.0.0", path = "../arrow-data" }
-chrono = { version = "0.4", default-features = false, features = ["clock"] }
+chrono = { version = "0.4.23", default-features = false, features = ["clock"] }

Review Comment:
   chrono is upgraded to 0.4.23



-- 
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