Add limit parameter to nafill and setnafill by venom1204 · Pull Request #7819 · Rdatatable/data.table

venom1204 · 2026-07-18T10:12:12Z

this PR adds a limit argument to nafill and setnafill to restrict the maximum number of consecutive NA values filled. Default is Inf to preserve existing behavior. Updated R interface, C kernel logic, and documentation.

hi @tdhock @joshhwuu can you please have a look at this when you got time thanks .

codecov · 2026-07-18T10:18:19Z

Codecov Report

❌ Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.99%. Comparing base (895067d) to head (7c59afe).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nafill.c	91.93%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7819      +/-   ##
==========================================
- Coverage   99.02%   98.99%   -0.03%     
==========================================
  Files          88       88              
  Lines       17266    17318      +52     
==========================================
+ Hits        17097    17144      +47     
- Misses        169      174       +5

☔ 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.

github-actions · 2026-07-18T10:36:18Z

HEAD=issue777666777 much slower for transform improved in #5493

Generated via commit a6018a9

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

Task	Duration
R setup and installing dependencies	6 minutes and 35 seconds
Installing different package versions	12 minutes and 31 seconds
Running and plotting the test cases	5 minutes and 31 seconds

ben-schwen · 2026-07-25T10:30:49Z

+test(2379.01, nafill(c(1, NA, NA, NA, 5), type="locf", limit=1), c(1, 1, NA, NA, 5))
+test(2379.02, nafill(c(1, NA, NA, 5, NA, NA, NA, 9), type="locf", limit=2), c(1, 1, 1, 5, 5, 5, NA, 9))
+test(2379.03, nafill(c(1, NA, NA, 4, NA), type="locf", limit=Inf), c(1, 1, 1, 4, 4))
+test(2379.04, nafill(c(NA, NA, 3, NA, NA), type="locf", limit=1), c(NA, NA, 3, 3, NA))


whats the difference between 01 and 04?

can we also test edge cases like negative, 0, NA, NaN, NULL?

ben-schwen · 2026-07-25T10:32:53Z

+# limit= restricts the number of consecutive fills
+y = c(1, NA, NA, NA, 5)
+nafill(y, "locf", limit=1) # Only fills the first NA
+nafill(y, "locf", limit=2) # Fills the first two NAs


I dont get the added value of the second line, maybe change it to Inf for showcasing filling all.

ben-schwen · 2026-07-25T10:33:25Z

  type = match.arg(type)
-  .Call(CnafillR, x, type, fill, nan_is_na(nan), FALSE, NULL)
+  if (!is.numeric(limit) || length(limit) != 1L || limit < 0)
+     stopf("limit must be a non-negative scalar numeric or Inf")


unprecise since Inf is also non-negative

ben-schwen

What about type="const". What about fractions like limit=1.5?

Please add tests for the following cases:

nocb
limit: 0, NA, NaN, negative, fractional, complex, NULL, and length > 1
integer and character
setnafill() by-reference

NEWS is missing.

What nafill(c(NA, 1), "locf", fill=0, limit=0)

ben-schwen · 2026-07-26T10:50:56Z

      ans->dbl_v[0] = ISNAN(x[0]) ? fill : x[0];
+      if (ISNAN(x[0])) fills = 1;


We do not need this double branching on ISNAN(x[0]). This will also happen at the other cases below!

ben-schwen · 2026-07-26T10:59:20Z

  if (verbose)
    tic = omp_get_wtime();

+  double limit_val = REAL(limit)[0];


Might be cleaner to represent as uint64_fast64_t since we also compare with it later

const double limit_d = REAL(limit)[0]; const uint_fast64_t limit_n = !R_FINITE(limit_d) || limit_d >= (double)UINT_FAST64_MAX ? UINT_FAST64_MAX : (uint_fast64_t)limit_d;

ben-schwen

Ty for the changes. We are almost there:
Still missing.

the double branching thingy
the c conversion of limit (this I consider as optional but it seems the right/cleaner approach)
clarify in the docs whats happening for fractions of limit
clarify whats happening for type='const'
clarify boundary case for nafill(c(NA, 1), "locf", fill=0, limit=0) which gives c(0, 1)
code coverage is still failing

ben-schwen · 2026-08-10T09:25:27Z

  if (verbose)
    tic = omp_get_wtime();
  if (type==0) { // const
-    if (nan_is_na) {


why do we remove this explicit branching? please dont just revert but explain!

ben-schwen · 2026-08-10T09:26:32Z

    }
  }

-  unsigned int itype=-1;


why do we make this change here? please explain!

ben-schwen · 2026-08-10T09:29:50Z

+dt = data.table(a=c(1, NA, NA))
+setnafill(dt, type="locf", limit=1)
+test(2380.09, {dt=data.table(a=c(1,NA,NA)); setnafill(dt, type="locf", limit=1); dt$a}, c(1, 1, NA))


why no test at the 2nd line?

Does also look quite more complicated than simple writing...

Suggested change

dt = data.table(a=c(1, NA, NA))

setnafill(dt, type="locf", limit=1)

test(2380.09, {dt=data.table(a=c(1,NA,NA)); setnafill(dt, type="locf", limit=1); dt$a}, c(1, 1, NA))

test(2380.09, setnafill(data.table(a=c(1, NA, NA)), type="locf", limit=1), data.table(a=c(1, 1, NA)))

ben-schwen · 2026-08-10T09:32:51Z

+test(2380.13, nafill(1:5, limit=-1), error="limit must be a non-negative scalar numeric")
+test(2380.14, nafill(1:5, limit=NA), error="limit must be a non-negative scalar numeric")
+test(2380.15, nafill(1:5, limit=NULL), error="limit must be a non-negative scalar numeric")
+test(2380.16, nafill(1:5, limit=c(1, 2)), error="limit must be a non-negative scalar numeric")
+test(2380.17, nafill(1:5, limit=1+2i), error="limit must be a non-negative scalar numeric")
+test(2380.18, setnafill(data.table(a=1:5), limit=-1), error="limit must be a non-negative scalar numeric")


Maybe loop over?

Suggested change

test(2380.13, nafill(1:5, limit=-1), error="limit must be a non-negative scalar numeric")

test(2380.14, nafill(1:5, limit=NA), error="limit must be a non-negative scalar numeric")

test(2380.15, nafill(1:5, limit=NULL), error="limit must be a non-negative scalar numeric")

test(2380.16, nafill(1:5, limit=c(1, 2)), error="limit must be a non-negative scalar numeric")

test(2380.17, nafill(1:5, limit=1+2i), error="limit must be a non-negative scalar numeric")

test(2380.18, setnafill(data.table(a=1:5), limit=-1), error="limit must be a non-negative scalar numeric")

lims = list(-1, NA, NaN, NULL, c(1, 2), 1+2i)

for (i in seq_along(lims)) {

test(2380.12 + i / 100, nafill(1:5, limit=lims[[i]]), error="limit must be a non-negative scalar numeric")

}

ben-schwen · 2026-08-10T09:34:15Z

+test(2380.03, nafill(c(1, NA, NA, 4, NA), type="locf", limit=Inf), c(1, 1, 1, 4, 4))
+test(2380.04, nafill(c(NA, NA, 3, NA, NA), type="locf", limit=1), c(NA, NA, 3, 3, NA))
+test(2380.05, nafill(c(1, NA, NA, 5), type="nocb", limit=1), c(1, NA, 5, 5))
+test(2380.06, nafill(c(1L, NA, NA), type="locf", limit=1), c(1L, 1L, NA))


test 1,2,3,6,7, could probably be subsumed in 2 tests.

updated changes

dd875ac

venom1204 requested review from MichaelChirico and ben-schwen as code owners July 18, 2026 10:12

ben-schwen reviewed Jul 25, 2026

View reviewed changes

ben-schwen requested changes Jul 25, 2026

View reviewed changes

venom1204 force-pushed the issue777666777 branch 2 times, most recently from 67e1f87 to dd875ac Compare July 25, 2026 14:03

venom1204 added 4 commits July 25, 2026 14:18

Merge remote-tracking branch 'origin/master' into issue777666777

998fa2a

added changes

5bbc47d

..

14af4f4

..

7c59afe

ben-schwen reviewed Jul 26, 2026

View reviewed changes

ben-schwen requested changes Jul 26, 2026

View reviewed changes

venom1204 mentioned this pull request Jul 26, 2026

main discussion page for GSOC-2026 (VINIT) #1 venom1204/discussion-GSOC-2026#1

Open

tdhock changed the title ~~Add limit parameter to nafill and setnafill #7677~~ Add limit parameter to nafill and setnafill Jul 29, 2026

venom1204 added 3 commits August 5, 2026 18:19

Merge remote-tracking branch 'origin/master' into issue777666777

a6018a9

Merge remote-tracking branch 'upstream/master' into issue777666777

d2c06a7

updated changes

882189b

ben-schwen reviewed Aug 10, 2026

View reviewed changes

Comment thread src/nafill.c

}

}

unsigned int itype=-1;

ben-schwen Aug 10, 2026

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 do we make this change here? please explain!

ben-schwen reviewed Aug 10, 2026

View reviewed changes

		ans->dbl_v[0] = ISNAN(x[0]) ? fill : x[0];
		if (ISNAN(x[0])) fills = 1;

Uh oh!

Conversation

venom1204 commented Jul 18, 2026

Uh oh!

codecov Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-schwen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-schwen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 18, 2026 •

edited

Loading

github-actions Bot commented Jul 18, 2026 •

edited

Loading

ben-schwen left a comment •

edited

Loading