Set datatable.logical01 to TRUE by default

[MichaelChirico]

data.table/NEWS.1.md

Line 1181 in 6c1fd83

getOption("datatable.logical01", TRUE) # future release

The plan to change the default has been in place since v1.11.0 (2018). Let's include it in 1.15.0 as a good opportunity to include such changes.

[jangorecki]

deprecations IMO should be added at the beginning of dev cycle so there is more time to spot potential issues and overall impact, changingn default shortly before release is just extra risk. I don't see a reason why not to include it in at start of 1.15.99 instead.

[MichaelChirico]

The reason is maintenance burden. Keeping around all these pending deprecations needlessly complicates the codebase. The change was discussed almost 6 years ago -- more than enough time.

There are already dozens or more downstreams that have begun setting logical01=TRUE manually -- clearly there's demand for this value:

https://github.com/search?q=%2Flogical01%5Cs*%3D%5Cs*TRUE%2F+lang%3AR&type=code

There are also (fewer) downstreams that have begun setting logical01=FALSE despite the default, possibly anticipating the planned deprecation:

https://github.com/search?q=%2Flogical01%5Cs*%3D%5Cs*FALSE%2F+lang%3AR&type=code

@tdhock's nightly revdep runs are meant to help mitigate the risk. I don't see a reason not to merge to master first. If the new error footprint turns out to be very large we can revert before release.

Realistically, with the end of year, the release will not happen for at least 5 or 6 weeks. I think that is plenty of time.

[jangorecki]

Unfortunately revdeps are just a tiny fraction of code that uses data.table. Also 5-6 weeks is not much as users of dev pkg may not update it so frequently.
Assumption was to not add anything new to 1.15 other than fixes to functionality added between 1.14.10 and 1.15.0.

[MichaelChirico]

All .0 releases are the right time to include potentially breaking changes. Waiting another 6 months/year doesn't make sense to me if we really want to make the change.

revdeps aren't everything, but will give a good idea of what's broken anyway. We have seen time and again that even obscure behavior changes can get caught by revdeps. Something non-trivial as this will surely turn up something.

That said, the changes needed for #5843 didn't leave me with the most confidence that this feature is ready to be the default yet. For example, I'm not sure casting column of single value '1' or '0' to logical is the best behavior. Possibly we should require finding two unique values before casting. But in that case something like "read all files in this folder" may have unpleasant behavior like different column types for different files.

OTOH, maybe that's just an artifact of our tests on 1-row tables, and it's not very practically relevant.

IMO the best next step is to merge the change and see what revdeps turn up. I have a feeling we will be reverting.

[tdhock]

I agree with Michael, let's just merge the change and see if it affects any revdeps. Best case, no revdeps to fix.

[jangorecki]

OK, merged, although not happy about that, as I see it pointless to increase the risk for breaking downstream code for no real benefit (remember revdeps is just a fraction of downstream code). Change could be shipped a week/month later to devel and 6 months later to CRAN without increasing the risk more than necessary. Just doesn't feel optimal.

[MichaelChirico]

no real benefit

The benefit is that we believe the new default is a better one. If better behavior is turned on by default, the package is improving.

If we believe the new behavior is worse, then we shouldn't change the default at all.

---

As an aside, @tdhock, is the revdep testing you're doing specific to {data.table}? I'm wondering if the scripts/etc should be stored in the repo so that anyone could run it / possibly offer PRs to improve it over time.

Found the wiki https://github.com/Rdatatable/data.table/wiki/Release-management-and-revdep-checks

[MichaelChirico]

Hey @tdhock I'm not sure I'm interpreting the results correctly.

Here: https://rcdata.nau.edu/genomic-ml/data.table-revdeps/analyze/2023-12-21/

I see

## data.table versions checked:
## [1] "master_1.14.99.6782251bcbc38c5240bb41f3af1b5f7436dac948" "release_1.14.10"

6782251 is the commit where we set the new default, so that means this run does include the new default right?

I then looked for 678225 on that page but didn't see anything. Does that mean there were no new broken revdeps?

[MichaelChirico]

OK, looks like the issue in the revdep script discovered in #5128 also uncovered the breakages here in the latest run:

https://rcdata.nau.edu/genomic-ml/data.table-revdeps/analyze/2023-12-24/

If I'm reading it correctly, that's 21 packages broken by this change. Pretty substantial! Let's roll it back and start a new issue discussing whether the breaking change is worthwhile for the next release.