all.equal dispatches to column methods

[MichaelChirico]

Closes #4543

A bit clunky, open to suggestions for other ways to solve #4543 and also keep test 1710.5.

The improvement here has flushed out some really long-time broken tests in our suite of two flavors:

• Comparisons on integer64 columns were really broken. Actually we were comparing the underlying double representations, which are typically size 1e-300 floats, which of course "look the same" from all.equal.numeric() PoV. Really obvious mismatches were not caught.
• Comparisons on POSIXct columns that differ in milliseconds. We might prefer to ignore such tiny differences, but I found in our cases it is a meaningful difference useful for testing when fwrite() retains sub-second representations vs. not.

Overall I think this is a really nice catch by this PR & glad the suite is improved.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.61%. Comparing base (fad5b17) to head (9efeeac).

❗ Current head 9efeeac differs from pull request most recent head 4dae909. Consider uploading reports for the commit 4dae909 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4546      +/-   ##
==========================================
+ Coverage   97.53%   99.61%   +2.07%     
==========================================
  Files          80       73       -7     
  Lines       14916    14105     -811     
==========================================
- Hits        14548    14050     -498     
+ Misses        368       55     -313     

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

[jangorecki]

any idea if this can be a breaking change for any proper use of all.equal?

[MichaelChirico]

I tried to guard against that but I can't say for 100% certain... perhaps best to slip this release

[jangorecki]

how is this related to rbindlist? I think you wanted to link #4543 @MichaelChirico

[MichaelChirico]

Yes thanks @jangorecki

[MichaelChirico]

I think this can be included in 1.16.0 👍

[github-actions]

Generated via commit e324926

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

Time taken to finish the standard R installation steps: 12 minutes and 6 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 44 seconds

[MichaelChirico]

It might be preferable to split off the test fixes that are not specifically related to this PR (but were identified as bugs by the code changes here); let me know.

[ben-schwen]

It might be preferable to split off the test fixes that are not specifically related to this PR (but were identified as bugs by the code changes here); let me know.

Nice! Change looks good, but I think the broken tests warrant a NEWS item now (also since there is a slight change of behavior).

[MichaelChirico]

Oh, good catch. It needed a NEWS item anyway.