[cbindlist/mergelist] Implement cbindlist #6435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

MichaelChirico merged 34 commits into master from cbindlist

Jul 1, 2025

Member

MichaelChirico commented Aug 29, 2024 •

edited

Loading

Towards #4370

This was referenced Aug 29, 2024

[cbindlist/mergelist] Whitespace changes #6431

Merged

[cbindlist/mergelist] Add ERR to the mult enum in bmerge.c #6432

Merged

Member Author

MichaelChirico commented Aug 29, 2024 •

edited

Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Aug 29, 2024

[cbindlist/mergelist] Add some C utilities: NROW/NCOL/isDataTable/isDataList/perhapsDataTable #6433

Merged

[cbindlist/mergelist] New seqexp helper #6434

Closed

[cbindlist/mergelist] Helpers for mergepair: onkeys/someCols/hasindex/fdistinct/dtmerge/seqexp/perhaps.data.table #6436

Merged

[cbindlist/mergelist] mergepair workhorse #6437

Merged

[cbindlist/mergelist] copyCols #6438

Merged

[dup of #4370, to be deleted] #6439

Closed

MichaelChirico force-pushed the cbind-merge-list-seqexp branch from 090a21b to f1ee884 Compare

August 29, 2024 17:31

MichaelChirico force-pushed the cbindlist branch from 874e7af to e4f3bc3 Compare

August 29, 2024 17:31

MichaelChirico changed the title ~~cbindlist~~ [cbindlist/mergelist] Implement cbindlist

MichaelChirico added the graphite-ready label

github-actions bot commented Aug 29, 2024 •

edited

Loading

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

Generated via commit 94167f2

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

Task	Duration
R setup and installing dependencies	2 minutes and 43 seconds
Installing different package versions	39 seconds
Running and plotting the test cases	2 minutes and 29 seconds

MichaelChirico force-pushed the cbindlist branch from e4f3bc3 to 5e149be Compare

August 29, 2024 17:56

MichaelChirico commented

View reviewed changes

man/cbindlist.Rd Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

man/cbindlist.Rd Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

man/cbindlist.Rd Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

man/cbindlist.Rd Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

man/cbindlist.Rd Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

src/mergelist.c Outdated Show resolved Hide resolved

Member Author

MichaelChirico commented Aug 29, 2024

Did you explore implementing this in R without needing any new C code?

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico force-pushed the cbindlist branch from 12003a3 to 7fb0f8e Compare

June 30, 2025 20:14

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico added 2 commits

June 30, 2025 13:21


          r->R

3aa76ba


          fixed approach for reading addresses

ea2a32c

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

mis

a1ca991

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico commented

View reviewed changes

inst/tests/mergelist.Rraw Outdated Show resolved Hide resolved

MichaelChirico added 2 commits

June 30, 2025 13:45


          more updated error expectations

d758eee


          disable new test for now

fdbff9b

MichaelChirico commented

View reviewed changes

Member Author

MichaelChirico left a comment

I think this is ready to merge after one more set of eyes takes a pass. Possibly there could be some follow-ups we require before 1.18.0 is released, but this PR per se is ~GTG.

codecov bot commented Jun 30, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.63%. Comparing base (96c3e6a) to head (94167f2).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6435      +/-   ##
==========================================
+ Coverage   98.50%   98.63%   +0.13%     
==========================================
  Files          79       81       +2     
  Lines       14759    14830      +71     
==========================================
+ Hits        14538    14628      +90     
+ Misses        221      202      -19

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

ben-schwen reviewed

View reviewed changes

inst/tests/mergelist.Rraw

Comment on lines +24 to +25

		expected = setDT(c(l$d1, l$d2, l$d3))
		test(11.03, ans, expected)

Member

ben-schwen Jul 1, 2025

Isn't this more about testing setDT than about testing cbindlist?

Member Author

MichaelChirico Jul 1, 2025

It's a mix of both, right? I'm not sure there's an easy way to avoid that...


          move setDT behavior test into tests.Rraw

6afd693

MichaelChirico mentioned this pull request

cbindlist could support retaining merged keys across inputs #7116

Open

MichaelChirico and others added 11 commits

July 1, 2025 09:51


          split out setcbindlist

633206f


          Merge branch 'master' into cbindlist

71219c6


          missed setcbindlist in Rd

d5e76c6


          Add as TODO

bc95d4e


          attempt to fix tests

146cf6a


          fix setDT test

edf7a36


          one last ironing out

9d663f4

ugh

cf5e5e4


          drop unreachable code

70d26eb


          Also unreachable by known code paths


          Annotate UNPROTECT

94167f2

Member Author

MichaelChirico commented Jul 1, 2025

Further refinements can be added in follow-ups, I think this is ready to go

MichaelChirico merged commit ae2b815 into master

13 checks passed

MichaelChirico deleted the cbindlist branch

July 1, 2025 17:53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet