Skip to content
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.

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

fix type coercion in bmerge #6603

Merged
merged 44 commits into from
Dec 2, 2024
Merged

fix type coercion in bmerge #6603

merged 44 commits into from
Dec 2, 2024

Conversation

ben-schwen
Copy link
Member

@ben-schwen ben-schwen commented Nov 3, 2024

Closes #6602

Base does not encounter this problem since one join column can not be in multiple join conditions.

What do we do:
In bmerge we check if the types to merge x and i in x[i] are compatible. This also does some type coercion for example when the responsible columns are integer and double.
If a single column of i is in multiple conditions with columns of x, all of the to be joined columns need to coerce, this is why we introduce the extra check length(ic_idx)>1L and then iterate over the corresponding columns of x.

Copy link

github-actions bot commented Nov 3, 2024

Comparison Plot

Generated via commit f5fb825

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 4 minutes and 49 seconds
Installing different package versions 7 minutes and 58 seconds
Running and plotting the test cases 2 minutes and 18 seconds

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.61%. Comparing base (546259d) to head (f5fb825).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6603   +/-   ##
=======================================
  Coverage   98.60%   98.61%           
=======================================
  Files          79       79           
  Lines       14518    14536   +18     
=======================================
+ Hits        14316    14334   +18     
  Misses        202      202           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ben-schwen ben-schwen marked this pull request as ready for review November 3, 2024 21:52
@ben-schwen
Copy link
Member Author

ben-schwen commented Nov 3, 2024

@MichaelChirico is it worth to make the if more specific to this corner case e.g.

(length(unique(icols))!=length(icols) || length(unique(xcols))!=length(xcols))

ensuring that either icols has a double condition or xcols has a double condition?

R/bmerge.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Member

Sorry, I just saw that this is a CRAN requirement, checking now.

R/bmerge.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Member

Definitely thanks for triaging a fix here. I think we both agree it's pretty hack-y & ideally not needed.

I'm not sure I fully understand the bug yet, but as stated in OP we're doing as.Date().

IMO as.Date() should always coerce to double storage, even if it's possible to get Date objects with integer storage. Therefore I would hold off until (1) R-core confirms that yes, as.Date(x) might return integer sometimes; and/or (2) I get a clearer understanding of the underlying issue here.

@ben-schwen
Copy link
Member Author

IMO as.Date() should always coerce to double storage, even if it's possible to get Date objects with integer storage. Therefore I would hold off until (1) R-core confirms that yes, as.Date(x) might return integer sometimes; and/or (2) I get a clearer understanding of the underlying issue here.

Unfortunately this "contract" does not hold. I have a Windows dev version installed here and get the following

> typeof(.Date(0L))
[1] "integer"
> typeof(as.Date(.Date(0L)))
[1] "integer"

@MichaelChirico
Copy link
Member

I have a Windows dev version installed here and get the following

Thanks, also confirmed that on 4.4.1

@TysonStanley
Copy link
Member

This is strange as Kurt mentioned there wasn't an intention to do this but I figure it would have been fixed by now if that were the case.

@ben-schwen
Copy link
Member Author

Update this fix now can convert into one direction from integer to double

# this works
x = data.table(a=1L)
y = data.table(c=1L, d=2)
y[x, on=.(c==a, d==a)]
y[x, on=.(d==a, c==a)]
# this still needs to fixed
x = data.table(a=1)
y = data.table(c=1, d=2L)
y[x, on=.(c==a, d==a)]
y[x, on=.(d==a, c==a)]

@MichaelChirico
Copy link
Member

this still needs to fixed

Should I consider this PR in-progress for now?

The diff has grown enough that it would help to add a brief overview of the changes to the PR description to orient reading

R/bmerge.R Outdated Show resolved Hide resolved
@ben-schwen ben-schwen marked this pull request as draft November 25, 2024 11:58
@TysonStanley
Copy link
Member

Just received the "deadline" on this from Kurt:

Please correct before 2024-12-09 to safely retain your package on CRAN. Note that this will be the final reminder.

@MichaelChirico
Copy link
Member

MichaelChirico commented Nov 27, 2024

Aside for follow-up (#6628):

I think we should implement mergeType returning I?Date, POSIXct as separate cases. That would for example solve #3506 and could make the solution here clearer.

R/bmerge.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
Copy link
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

Awesome!

@MichaelChirico MichaelChirico merged commit 23dac21 into master Dec 2, 2024
9 checks passed
@MichaelChirico MichaelChirico deleted the bmerge_coercion branch December 2, 2024 05:55
MichaelChirico added a commit that referenced this pull request Dec 2, 2024
* fix type coercion in bmerge

* fix bracket

* add test cases

* fix lint

* fix old test case

* rename x/i class

* add minimal test

* indent loop

* add fix in one direction

* remove indent to cater for diff

* Revert "remove indent to cater for diff"

This reverts commit 562a9fd.

* remove indent

* add 2nd case

* remove trailing ws

* update all cases

* fix typo

* fix test cases

* update testcases

* update copying attributes from int to dbl

* start modularize

* fix cases

* ensure same types for test

* add test for codecov

* simplify

* fix test on windows

* simplify

* add coerce function

* modularize more

* Use gettext() on character strings directly

* rename getClass helper: mergeType

* rename: {i,x}c --> {i,x}col

I found myself wondering `ic`... "`i` character? `i` class?". Simpler to encode more info in the name

* comment ref. issue

* exchange subset with .shallow

* undo test

* Revert "undo test"

This reverts commit c9d3d74.

* update tests

* add comment

* add non right join testcase

* move helper outside bmerge

* update comment

* add NEWS

* update numbering

* tweak NEWS

---------

Co-authored-by: Michael Chirico <[email protected]>
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.

as.Date can result in different underlying types
4 participants