Align melt with docs for list measure.vars by Mukulyadav2004 · Pull Request #7257 · Rdatatable/data.table

Mukulyadav2004 · 2025-08-21T19:15:07Z

In melt, docs stated that when measure.vars is a list, the variable column contains integer indices. Actual behavior was inconsistent when the list length was 1 (character name), with a “warn now, break later” warning #5209.

So in this PR
melt breaking change (consistency with docs)(fmelt.c)

When measure.vars is a list (including length 1), the variable column now contains integer indices and the transitional warning is removed.
When measure.vars is a character or integer vector,behavior is unchanged(variable shows column names).
If variable.factor=TRUE, levels are '1', '2', .. when measure.vars is a list,unchaged behaviour for vector measure.vars

In test 2182.6, 2182.71, 2182.73, 2182.75, expectations no swiched from names to integer and also removed the warning. And in 1037.404 and 1037.407 - Int

@tdhock @joshhwuu @Anirban166 please review.

codecov · 2025-08-21T19:26:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@c27ec26). Learn more about missing BASE report.
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #7257   +/-   ##
=========================================
  Coverage          ?   99.00%           
=========================================
  Files             ?       81           
  Lines             ?    15252           
  Branches          ?        0           
=========================================
  Hits              ?    15100           
  Misses            ?      152           
  Partials          ?        0

☔ 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 · 2025-08-21T19:34:09Z

HEAD=6629#issue stopped early for DT[by,verbose=TRUE] improved in #6296

Generated via commit f552d56

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

Task	Duration
R setup and installing dependencies	2 minutes and 52 seconds
Installing different package versions	40 seconds
Running and plotting the test cases	2 minutes and 50 seconds

tdhock · 2025-08-22T13:47:01Z

src/fmelt.c

+        SET_VECTOR_ELT(ansvars, 0, target=allocVector(STRSXP, data->totlen));
+        if (data->lvalues == 1) {
+          const int *thisvaluecols = INTEGER(VECTOR_ELT(data->valuecols, 0));
+          for (int j=0, ansloc=0; j<data->lmax; ++j) {


there should be no such addition here, why did you do this?

You’re right the non-list path in getvarcols should not change. I’ve reverted that addition so the only functional change is confined to the list branch

tdhock · 2025-08-22T13:47:57Z

can you please separate dcast and melt into two separate PRs? (currently they are combined in this PR) That would be much easier to review.

Mukulyadav2004 · 2025-08-22T17:28:00Z

can you please separate dcast and melt into two separate PRs? (currently they are combined in this PR) That would be much easier to review.

Sure

tdhock · 2025-08-23T01:04:14Z

src/fmelt.c

-      if (data->lvalues == 1) {//one value column to output. TODO #5247 change to !data->measure_is_list
-        SEXP thisvaluecols = VECTOR_ELT(data->valuecols, 0);
-        int len = length(thisvaluecols);
-        levels = PROTECT(allocVector(STRSXP, len)); protecti++;


please revert these changes and instead use the approach here https://github.com/Rdatatable/data.table/pull/5247/files

Mukulyadav2004 · 2025-08-23T16:13:20Z

Thanks! I reverted non-list changes and followed from PR #5247 - branch on data->measure_is_list inside getvarcols. For list measure.vars, variable returns integer indices and 1..lmax levels when variable.factor=TRUE. Non-list paths are unchanged.

Does this seems right to you?

Mukulyadav2004 · 2025-08-25T09:37:10Z

@tdhock what all I'm understanding and doing here is that when variable.factor is FALSE, list measure.vars give INTSXP indices 1to lmax while non-list remains STRSXP of column names and when variable.factor is TRUE, list give a factor with integer codes 1 to lmax and levels "1" to "lmax", and non list remains the original name based factor logic, keeping the if (data->lvalues == 1).
Could you please clarify if I'm lagging somewhere.

tdhock · 2025-08-25T18:39:45Z

inst/tests/tests.Rraw

-test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(c("b1","b2")), b=c(1,2)), warning="measure.vars is a list with length=1") # measure.vars named list length=1, #5065
-# consistency between measure.vars=list with length=1 and length>1, #5209
-test(2182.71, melt(DT.wide, measure.vars=list("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor("a2"), value=2), warning="measure.vars is a list with length=1")
+test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(1:2), b=c(1,2))) # list yields indices


please add these tests from the other pr

test(2182.6, melt(DT.wide, measure.vars=list(b=c("b1","b2"))), data.table(a2=2, variable=factor(c("1","2")), b=c(1,2))) # measure.vars named list length=1, #5065 # consistency between measure.vars=list with length=1 and length>1, #5209 test(2182.71, melt(DT.wide, measure.vars=list("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor(1), value=2)) test(2182.72, melt(DT.wide, measure.vars=c("a2"), variable.factor=TRUE), data.table(b1=1, b2=2, variable=factor("a2"), value=2)) test(2182.73, melt(DT.wide, measure.vars=list("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="1", value=2)) test(2182.74, melt(DT.wide, measure.vars=c("a2"), variable.factor=FALSE), data.table(b1=1, b2=2, variable="a2", value=2)) test(2182.75, melt(data.table(a=10, b=20), measure.vars=list(n="a"), variable.factor=FALSE), data.table(b=20, variable="1", n=10))#thanks @mnazarov

tdhock · 2025-08-25T18:42:18Z

src/fmelt.c

+        } else {//multiple output columns.
+          int nlevel=0;
+          levels = PROTECT(allocVector(STRSXP, data->lmax)); protecti++;
+          for (int j=0, ansloc=0; j<data->lmax; ++j) {


all of this code is un-necessary. where did it come from?
all you need to do is change the test in the if statements, as in https://github.com/Rdatatable/data.table/pull/5247/files#diff-1727bfee28bf4521c354c6f4463e6fcaca14a4d62eda781552a489a749b5ec59

tdhock · 2025-08-27T13:37:47Z

this pr is confusing to me, because it has a lot of irrelevant changes/commits.
I am closing. can you please create a new one?
an acceptable PR will just undo what this PR did:
https://github.com/Rdatatable/data.table/pull/6333/files

Mukulyadav2004 · 2025-08-28T00:13:28Z

Thank you @tdhock for the guidance! I've opened a new PR in which only revert the changes from #6333 as you said. Please review.

Mukulyadav2004 added 2 commits August 21, 2025 20:16

use integer indices for measure.vars and error on non scalar aggregates

8280e2d

1037 - 404 and 407

7e02a0d

Mukulyadav2004 requested review from MichaelChirico and tdhock as code owners August 21, 2025 19:15

remove nws item

5abd761

Mukulyadav2004 changed the title ~~6629#issue~~ Align melt with docs for list measure.vars and make dcast scalar only by removing warn path Aug 21, 2025

tdhock reviewed Aug 22, 2025

View reviewed changes

removed extra multiple value column loop

e3b8a64

Mukulyadav2004 changed the title ~~Align melt with docs for list measure.vars and make dcast scalar only by removing warn path~~ Align melt with docs for list measure.vars Aug 22, 2025

revert fcast modifications

4d283ba

tdhock requested changes Aug 23, 2025

View reviewed changes

keep non list path

879198c

revert

ed1712e

tdhock reviewed Aug 25, 2025

View reviewed changes

Mukulyadav2004 added 4 commits August 26, 2025 00:48

delete multiple output column else branch

c0e03ed

Fix test cases

3f0e0a8

closed bracket

c16469b

EXPECT INTEGER

f552d56

tdhock closed this Aug 27, 2025

Conversation

Mukulyadav2004 commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdhock Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Mukulyadav2004 Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

tdhock commented Aug 22, 2025

Uh oh!

Mukulyadav2004 commented Aug 22, 2025

Uh oh!

tdhock Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Mukulyadav2004 commented Aug 23, 2025

Uh oh!

Mukulyadav2004 commented Aug 25, 2025

Uh oh!

tdhock Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

tdhock Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

tdhock commented Aug 27, 2025

Uh oh!

Mukulyadav2004 commented Aug 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Mukulyadav2004 commented Aug 21, 2025 •

edited

Loading

codecov bot commented Aug 21, 2025 •

edited

Loading

github-actions bot commented Aug 21, 2025 •

edited

Loading