Skip to content

Date Time Functions - Modifiers and Basic Time Values#123

Merged
FletcherDares merged 3 commits into
mainfrom
fletcher/date-time-modifiers
Dec 13, 2025
Merged

Date Time Functions - Modifiers and Basic Time Values#123
FletcherDares merged 3 commits into
mainfrom
fletcher/date-time-modifiers

Conversation

@FletcherDares
Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs Outdated
let m = m + 12 * a - 3;

let jdn_int =
d + (153 * m + 2) / 5 + 365 * y + y / 4 - y / 100 + y / 400 - JULIAN_DAY_EPOCH_OFFSET;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Potential Bug: If a Value::Real represents a Julian Day Number with subsecond precision (e.g., 2461022.0000014235), creating JulianDay::new(*jdn_float) will again set is_subsecond to false. This will cause as_time() and as_datetime() to lose the subsecond information. Consider making JulianDay::new intelligent about inferring is_subsecond from the f64 value, or provide an explicit way to set it during construction.

Comment thread tests/suites/datetime_operations.rs Outdated
Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs
Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs
Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs
Comment thread tests/suites/datetime_operations.rs Outdated
Comment thread tests/suites/datetime_operations.rs Outdated
Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs
Comment thread src/db/table/operations/helpers/datetime_functions/time_values.rs
@github-actions
Copy link
Copy Markdown

AI review done up to commit: b2316b8

AI Review Summary:

This pull request introduces a new JulianDay struct and refactors datetime functions to use Julian Day Numbers. New datetime functions (DATE, TIME, DATETIME, JULIANDAY, UNIXEPOCH) are added. The overall quality of the changes is good, but there's a recurring logical error concerning subsecond precision handling. The JulianDay::new constructor always sets is_subsecond to false, potentially leading to precision loss when parsing inputs that legitimately contain subsecond data. This needs to be addressed to ensure accurate representation in as_time() and as_unix_epoch() methods. Additionally, the new test suite could benefit from more comprehensive tests, specifically for subsecond precision scenarios.

Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs
Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs
Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs Outdated
Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs
Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs Outdated
Comment thread src/db/table/operations/helpers/datetime_functions/modifiers.rs Outdated
@github-actions
Copy link
Copy Markdown

AI review done up to commit: 85b258d

AI Review Summary:

This PR introduces a new constructor for JulianDay from datetime components, refactors the Julian Day calculation into this constructor, and updates the parse_timevalue function to use it. It also begins implementing the parse_modifier function. The Julian Day calculations and time_values.rs changes appear correct. However, the parse_modifier function in modifiers.rs has two logical issues: it uses unwrap() for float parsing which can panic, and it only handles 'days' for numeric time unit offsets, missing 'years', 'months', 'hours', 'minutes', and 'seconds' as per SQLite documentation.

Comment thread src/db/table/operations/helpers/datetime_functions/julian_day.rs
@github-actions
Copy link
Copy Markdown

AI review done up to commit: 8d50304

AI Review Summary:

Code review completed. Reviewed 0 files with 0 comments.

Fix issue with seconds calculation

Refactor datetime to build a julian day object

Simple integration tests for the initial implementation of datetime functions

Move datetime builder into jdn

Parsing implemented for simple modifiers

Fix issue with testing

fix fmt

Add todo list

fix backup PR review service
@FletcherDares FletcherDares force-pushed the fletcher/date-time-modifiers branch from 8d50304 to f2db010 Compare December 13, 2025 17:06
@github-actions
Copy link
Copy Markdown

AI review done up to commit: f2db010

AI Review Summary:

Reviewed 11 files introducing new date/time functionality. Identified issues related to inconsistent return types and incorrect struct modification in datetime functions. Also noted a potential off-by-one error in Julian Day calculation and a redundant clone in tests. Overall, the new functionality is significant but requires refinement in error handling and type consistency.

Comment thread src/db/table/operations/helpers/datetime_functions/mod.rs
@github-actions
Copy link
Copy Markdown

AI review done up to commit: 522788c

AI Review Summary:

I have reviewed the changes, which involve refactoring date and time modifier parsing and updating associated tests.

One potential logic error was found in src/db/table/operations/helpers/datetime_functions/mod.rs regarding the mutation vs. reassignment of init_jdn. Additionally, the test suite in tests/suites/datetime_operations.rs has changed the argument order in several assertion calls, which could reduce readability.

Overall, the changes appear to be functional but require attention to the identified logic and testing concerns.

@github-actions
Copy link
Copy Markdown

AI review done up to commit: c9e41c3

AI Review Summary:

The changes involve reformatting the arguments of the JulianDay::new_relative_from_datetime_vals function calls to be on separate lines for improved readability. No functional changes, bugs, or security issues were introduced. The overall quality of the changes is good.

@FletcherDares FletcherDares merged commit bdf4134 into main Dec 13, 2025
4 checks passed
@FletcherDares FletcherDares deleted the fletcher/date-time-modifiers branch December 13, 2025 17:15
@FletcherDares FletcherDares changed the title Date Time Functions Date Time Functions - Modifiers and Basic Time Values Dec 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant