setDF deletes the index attribute

[OfekShilon]

Closes #4889

[MichaelChirico]

change looks great! please also add the following:

1. NEWS item briefly describing the change & citing yourself for the fix (imitate the other items should be fine)
2. add yourself to DESCRIPTION
3. write a test to ensure this behavior doesn't regress later
4. Denes mentioned a slight amendment to ?setDF as well, could you add that?

[codecov]

Codecov Report

Merging #4893 (6d71dba) into master (2791043) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 6d71dba differs from pull request most recent head d38211d. Consider uploading reports for the commit d38211d to get more accurate results

@@           Coverage Diff           @@
##           master    #4893   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          75       75           
  Lines       14808    14812    +4     
=======================================
+ Hits        14730    14734    +4     
  Misses         78       78           

Impacted Files	Coverage Δ
R/data.table.R	99.94% <100.00%> (+<0.01%)	⬆️
src/utils.c	94.77% <100.00%> (ø)
R/setops.R	100.00% <0.00%> (ø)
src/fread.c	99.47% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7325a3...d38211d. Read the comment docs.

[OfekShilon]

change looks great! please also add the following:

1. NEWS item briefly describing the change & citing yourself for the fix (imitate the other items should be fine)
2. add yourself to DESCRIPTION
3. write a test to ensure this behavior doesn't regress later
4. Denes mentioned a slight amendment to ?setDF as well, could you add that?

What change is needed to ?setDF ? Where was it discussed?

[ColeMiller1]

The tests are failing due to the external references changes. Those changes seem like a separate PR and outside the scope of removing the index attribute during setDF. That doesn't mean they are not worthwhile - it's just the index thing is a no-brainer and needs to be fixed. The external reference needs additional design and review (e.g., the tests are failing!) and maybe there are other ways to deal with it.

See the comment here #4889 (comment) for the ?setDF enhancement. Here's a copy of the suggestion Denes had:

All data.table attributes including any keys and indices of the input data.table are stripped off.

[OfekShilon]

The tests are failing due to the external references changes. Those changes seem like a separate PR and outside the scope of removing the index attribute during setDF. That doesn't mean they are not worthwhile - it's just the index thing is a no-brainer and needs to be fixed. The external reference needs additional design and review (e.g., the tests are failing!) and maybe there are other ways to deal with it.

This made me learn the hard way that any commits I push to the same branch are automatically added to the same PR :(
I never intended for the other commits to go in. Do I have a way out of this mess or should I delete the PR and start from scratch?

[jangorecki]

PR is always based on a branch. If you don't want commits to go into same PR then you have to make new branch for them.
If you are not OK with recent commits you can do git revert and git push. If you are not sharing a branch to others, then git reset and git push --force are fine.

[OfekShilon]

This is a small but rather important fix ([.data.table occasionally gives wrong results without it). Anything holding it back from being merged?

[OfekShilon]

@MichaelChirico is there anything I can do to help merge this?