Skip to content

Conversation

@aksh08022006
Copy link

This PR addresses #7463 by allowing fcoalesce() and setcoalesce() to handle mixing of Date (double) and IDate (integer) classes, as well as general mixing of integer and double types. Previously, these combinations would throw a strict type/class mismatch error.

Technical Details The core of the change is in
src/coalesce.c
. I've updated the logic to:

Relax Type Checks: Added a promote_to_real flag that triggers when fcoalesce sees an integer vector followed by a double. This allows fcoalesce to return a REALSXP vector even if the first argument was INTSXP.
Handle Date/IDate Inheritance: Modified the class-matching check to allow any two objects that both inherit from the Date class. This handles the specific pairing of Date and IDate which were previously rejected as "different classes".
Strict In-place Rules: For setcoalesce(), I've kept the strict rule that we cannot promote an integer vector to double in-place (as that would require memory reallocation). It now throws a specific, helpful error suggesting manual coercion instead of a generic "type mismatch".
Attribute Persistence: Corrected the attribute handling so that if we promote an IDate to a double Date, the class attributes are correctly propagated to the new object.
Testing I've added six new tests to the end of
inst/tests/tests.Rraw
(numbered 2357.1–2357.6) covering:

fcoalesce(Date, IDate)
fcoalesce(IDate, Date) (promotion check)
Numeric mixing (INTSXP + REALSXP)
setcoalesce safety checks (ensuring we don't accidentally reallocate in-place).
Verified that these fail on the current master and pass with this build. I also re-ran the full local test suite to ensure no regressions in existing fcoalesce logic.

A Note to Maintainers I've tried my best to follow the data.table coding style and memory safety principles, but I'm still getting my head around some of the C-level internals. I could be wrong about how I've handled the promotion flags or if there's a more efficient way to check for the Date class inheritance at this level. I'm very much open to learning if there's a better or more "the data.table way" to implement this!

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