Respect integer64= selection when out-of-sample type bumps happen in fread()

[badasahog]

Closes #2749

I also changed while(true) to for(;;) for consistency. Technically should've been its own pr, hope you don't mind.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (2642b84) to head (ecda6f8).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7032   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          79       79           
  Lines       14676    14677    +1     
=======================================
+ Hits        14485    14486    +1     
  Misses        191      191           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

• HEAD=i64remapfix stopped early for fread disk overhead improved in #6925
  

Generated via commit ecda6f8

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

Task	Duration
R setup and installing dependencies	2 minutes and 42 seconds
Installing different package versions	41 seconds
Running and plotting the test cases	2 minutes and 2 seconds

[MichaelChirico]

Please add some test(s) in inst/tests/tests.Rraw, and be sure to write a NEWS entry.

Also please make note of the tiny tweak to the PR title / description -- mostly this is about respecting quirks of GitHub itself, which doesn't support #NNN issue linking in titles, and does have a nice integration where Closes #NNN will automatically close the issue when the PR is merged.

@oleksiyskononenko are you able to confirm if pydatatable is still maintained / whether this change to readInt64As affects that implementation?

[badasahog]

I'm sorry, I don't know how to add tests, I'm not familiar with the testing system.

[badasahog]

Also, it looks like there are 3 different news files? Which one should I use?

[MichaelChirico]

NEWS.x are archival files; NEWS.md is the "current" one, to which new entries should be added.

The structure of the tests file is pretty simple -- just add new test() entries, appropriately numbered, at the end.

Here's the documentation of test():

https://rdatatable.gitlab.io/data.table/reference/test.html

You might search around for other tests of out-of-sample type bumps, and piggy-back off of those. It's acceptable to, e.g., find test(100.1) that's closely related to your new behavior and insert a new test 100.2 in the middle of the file -- the simplest approach, also perfectly acceptable, is to just append your new tests to the end.

[oleksiyskononenko]

@MichaelChirico I'm not involved in datatable development anymore, so can't confirm. However, it doesn't share any code with data.table for a long time, so your changes to fread() wouldn't affect anything on the python side.

[MichaelChirico]

Hi @badasahog I see a NEWS entry, but no tests yet. Let me know if you're stuck.

[badasahog]

Sorry for the late response. I don't know how to make tests, I'm not proficient in R, and I don't know how the testing system works.

[MichaelChirico]

Writing tests is a required part of OSS contribution. By not writing tests, you're burdening other contributors/the maintainers with doing so on your behalf.

I pointed above to the documentation to test(). The README for cc() might help more -- it provides some simple steps for iterating through tests locally:

https://github.com/Rdatatable/data.table/tree/master/.dev

You should also feel free to just use the GitHub CI suite -- make changes to tests, push to GitHub, check for GHA failures, iterate.

If you're stuck somewhere, we are happy to help, but simply opting out of writing tests won't work.

[aitap]

Good test, thank you! It fails for me on master and passes on the PR branch. One thing: since R doesn't have variable declarations, the { braces } don't create a new scope; any variables created inside them are fully visible in the outer environment. (The only thing they do is combine multiple expressions, returning the value of the last expression in the group.) The code might as well not create an indentation level at all.

[MichaelChirico]

If you do want to enforce a new scope, you can use local({ ... }). Though we don't really do that in our tests (we should, but that's another issue)

[aitap]

Thank you for fixing the bug!