Skip to content

add setnafill support for character vectors#7748

Open
ben-schwen wants to merge 3 commits into
masterfrom
setnafill_chars
Open

add setnafill support for character vectors#7748
ben-schwen wants to merge 3 commits into
masterfrom
setnafill_chars

Conversation

@ben-schwen
Copy link
Copy Markdown
Member

Closes #7586 #3992

Maybe I'm overlooking smth here because the addition seems to easy

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

I guess we should add a test of set() behavior per se where we ensure the delta of addresses is as intended?

Comment thread src/nafill.c Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

  • HEAD=setnafill_chars much faster for fread disk overhead improved in #6925
  • HEAD=setnafill_chars faster P<0.001 for memrecycle regression fixed in #5463
    Comparison Plot

Generated via commit 6cce6fd

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

Task Duration
R setup and installing dependencies 2 minutes and 44 seconds
Installing different package versions 23 seconds
Running and plotting the test cases 5 minutes and 24 seconds

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.04%. Comparing base (188dd7b) to head (6cce6fd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7748      +/-   ##
==========================================
- Coverage   99.04%   99.04%   -0.01%     
==========================================
  Files          87       87              
  Lines       17065    17064       -1     
==========================================
- Hits        16902    16901       -1     
  Misses        163      163              

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

Comment thread src/nafill.c
for (R_len_t i=0; i<nx; i++) {
vans[i] = ((ans_t) { .dbl_v=dx[i], .int_v=ix[i], .int64_v=i64x[i], .status=0, .message={"\0","\0","\0","\0"} });
SEXP xi = VECTOR_ELT(x, i);
vans[i] = ((ans_t) { .dbl_v=dx[i], .int_v=ix[i], .int64_v=i64x[i], .char_v=isString(xi) ? xi : R_NilValue, .status=0, .message={"\0","\0","\0","\0"} });
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.

Isn't it the first time we use .char_v field of ans_t?

SEXP char_v; // ineligible for filling in parallel!

says to not use it in parallel

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought thats what the any_char guarding is about in

#pragma omp parallel for if (nx>1 && !any_char) num_threads(getDTthreads(nx, true))

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.

Add setnafill() support for character columns

3 participants