Skip to content

rowwiseDT support for key - #7940

Open
rikivillalba wants to merge 7 commits into
masterfrom
rowwiseDT_allow_key
Open

rikivillalba wants to merge 7 commits into
masterfrom
rowwiseDT_allow_key

Conversation

@rikivillalba

@rikivillalba rikivillalba commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

closes #7905

  • key specification can occur in front or last position, or between headers and data blocks, but not within them, to prevent confusing code.
  • missing "key" is a column name, for backwards compatibility.
  • key = "" means missing key (again bc, as A = "" is interpreted as a column named A).
    So
rowwiseDT(
  a=, b=,
  key = "a",
  1, 2,
  3, 4
)

is valid but rowwiseDT(a=, key="a", b=, ...) isn't

@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.08%. Comparing base (7b7f63b) to head (d8864d9).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7940   +/-   ##
=======================================
  Coverage   99.08%   99.08%           
=======================================
  Files          88       88           
  Lines       17420    17432   +12     
=======================================
+ Hits        17261    17273   +12     
  Misses        159      159           

☔ View full report in Codecov by Harness.
📢 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.

Comment thread R/rowwiseDT.R Outdated
Comment thread R/rowwiseDT.R Outdated
Comment thread R/rowwiseDT.R Outdated
stopf("Named arguments must be empty")
if (!identical(header_pos, seq_along(header_pos)))
stopf("Header must be the first N arguments")
if (key_pos && !(key_pos - 1L) %in% c(0L, length(header_pos), length(x)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why length(header_pos)? do we lose a lot by making this an error as well?

(it can be a bit hard to map the test case to how it might look in "real" code, so I might be missing something, although this was something mentioned by me as 'bad' in #7905)

test(2282.11, rowwiseDT(A=, B=, key="B", 1, 2), DT)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is because I imagined data.table(a=.., b=.., key = "a") plus a data block, but I agree the extra flexibility is not very worthy here. I'll remove the middle option.

@tdhock

tdhock commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Is there a strong reason why you can’t just use setkey(rowwiseDT(…), colname) ?
(if no strong reason, i would suggest closing this issue/pr without adding the new arg, which could be confusing… after all the whole point of rowwiseDT is readable code?)

@MichaelChirico

Copy link
Copy Markdown
Member

The same can be said of data.table(); to me it's more awkward to have one data.table constructor that can set key but not the other.

I.e., I think this issue is about consistency with data.table().

@github-actions

Copy link
Copy Markdown
  • HEAD=rowwiseDT_allow_key much slower for transform improved in #5493

(Plot image upload to picrd.com failed; download the artifact below to view plots)

Generated via commit d8864d9

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

Task Duration
R setup and installing dependencies 1 minutes and 20 seconds
Installing different package versions 12 minutes and 36 seconds
Running and plotting the test cases 5 minutes and 33 seconds

This branch has not been deployed

No deployments
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.

rowwiseDT() could support setting key

3 participants