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.20779% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.98%. Comparing base (813ed95) to head (d86967d).

Files with missing lines	Patch %	Lines
src/nafill.c	91.54%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7819      +/-   ##
==========================================
- Coverage   99.01%   98.98%   -0.04%     
==========================================
  Files          88       88              
  Lines       17309    17344      +35     
==========================================
+ Hits        17139    17168      +29     
- Misses        170      176       +6

☔ 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 faster P<0.001 for memrecycle regression fixed in #5463

Generated via commit d86967d

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

Task	Duration
R setup and installing dependencies	2 minutes and 57 seconds
Installing different package versions	47 seconds
Running and plotting the test cases	5 minutes and 35 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!

I'll revert this. I thought a single loop would be cleaner

You're right that a single loop looks cleaner, but its not about what we are here after.

In the single loop you have to do the check every iteration, why otherwise you do it once. Now you could hope that the compiler is smart enough enough to optimize this after all, but you cannot be sure that it does.

This also applies to the if (nan_is_na) checks which you rewrote to single loops. I did not flag the other occurrences...

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

    }
  }

-  unsigned int itype=-1;


why do we make this change here? please explain!

I initially changed this to use a more compact format thinking it would be cleaner, but I'll revert it now to maintain consistency with the original style

I dont get why a nested ternary operator should be ever cleaner :D

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 4 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

..

c5381ce

ben-schwen reviewed Aug 10, 2026

View reviewed changes

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

fdf8f3b

venom1204 added 3 commits August 10, 2026 21:09

..

113076c

added test for that uncovered line

5026744

..

d86967d

		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!

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