Skip to content

feat: Add ingestion of matrices - #1150

Merged
krlmlr merged 2 commits into
duckdb:mainfrom
joakimlinde:feature/array-ingestion
May 19, 2025
Merged

feat: Add ingestion of matrices#1150
krlmlr merged 2 commits into
duckdb:mainfrom
joakimlinde:feature/array-ingestion

Conversation

@joakimlinde

Copy link
Copy Markdown
Contributor

No description provided.

@joakimlinde

joakimlinde commented May 13, 2025

Copy link
Copy Markdown
Contributor Author

Looking at R API issue... Fixed.

@krlmlr krlmlr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks. Is there a good way to make this feature opt-in, based on ConvertOpts ?

I see this adds some support for complex numbers. Could this have been a separate PR? Same question regarding opt-in behavior.

Comment thread src/scan.cpp Outdated
break;

case RType::INTEGER64: //REALSXP
AppendMatrixSegmentAtomic<int64_t, int64_t, RInteger64Type>(((int64_t *)DATAPTR(source_data)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be DATAPTR_RO() here and elsewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fixed.

Comment thread src/utils.cpp Outdated
}

R_len_t RApiTypes::GetVecSize(RType rtype, SEXP coldata) {
if (rtype.id() == RTypeId::MATRIX) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this check and the while loop be swapped?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can swap them. You mean that if I have a matrix inside a structure it would work better? Not sure if I've thought about that case... Is that a case we want to support?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For completeness and accuracy, yes. Not that I expect it to occur; if it does nevertheless, we'll save a lot of time later by doing a small step now.

@joakimlinde
joakimlinde force-pushed the feature/array-ingestion branch from f91148d to 2b6bf5b Compare May 13, 2025 19:49
@codecov-commenter

codecov-commenter commented May 13, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 87.05882% with 11 lines in your changes missing coverage. Please review.

Project coverage is 52.62%. Comparing base (5c88c6f) to head (a2b189e).
Report is 1539 commits behind head on main.

Files with missing lines Patch % Lines
src/scan.cpp 83.33% 8 Missing ⚠️
src/types.cpp 90.90% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1150       +/-   ##
===========================================
- Coverage   63.61%   52.62%   -10.99%     
===========================================
  Files         123      146       +23     
  Lines        5266    10025     +4759     
===========================================
+ Hits         3350     5276     +1926     
- Misses       1916     4749     +2833     

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

@joakimlinde

joakimlinde commented May 13, 2025

Copy link
Copy Markdown
Contributor Author

Is there a good way to make this feature opt-in, based on ConvertOpts ?

No, I don't think so. Here is why. If the user gives me a matrix I only have two options. Either reject it as "I don't know what to do with a matrix" or slice up the matrix into arrays and push that into the database. The intent is clear from the user - he/she is giving me a matrix and there is only one constructive thing I can do with it. If the user had given me lists instead it would have been different. In that case I would have to know what to do with them, either create lists or arrays in the database. In that case the user would have to give me additional direction through options.

I see this adds some support for complex numbers. Could this have been a separate PR? Same question regarding opt-in behavior.

I added support so I can generate an error message saying that complex numbers are not supported. I could do it in a different way by looking at the SEXP type instead if that works better for you. It would break the pattern in scan.cpp though — look a little bit as a quick-and-dirty.

@joakimlinde
joakimlinde force-pushed the feature/array-ingestion branch from 2b6bf5b to f8fd4e7 Compare May 13, 2025 22:23
@joakimlinde

joakimlinde commented May 13, 2025

Copy link
Copy Markdown
Contributor Author

The Ubuntu-22.04 (4.2) check fails. Seems like the snapshot files didn't make it across to Ubuntu. Can I ask you to manually rerun the test?

UPDATE: After triggering a few reruns (using dummy changes) the tests are now passing.

@joakimlinde
joakimlinde force-pushed the feature/array-ingestion branch 2 times, most recently from 3b2b940 to 4bf92f4 Compare May 15, 2025 17:35
@joakimlinde
joakimlinde force-pushed the feature/array-ingestion branch from 4bf92f4 to a2b189e Compare May 15, 2025 18:04
@krlmlr
krlmlr force-pushed the main branch 2 times, most recently from 3e5d5fa to f344576 Compare May 18, 2025 20:46
@krlmlr krlmlr changed the title Add ingestion of matrices feat: Add ingestion of matrices May 19, 2025
@krlmlr
krlmlr enabled auto-merge (squash) May 19, 2025 09:43
@krlmlr
krlmlr disabled auto-merge May 19, 2025 09:43
@krlmlr
krlmlr enabled auto-merge (squash) May 19, 2025 09:43
@krlmlr

krlmlr commented May 19, 2025

Copy link
Copy Markdown
Collaborator

Thanks!

@krlmlr
krlmlr disabled auto-merge May 19, 2025 13:56
@krlmlr
krlmlr merged commit 07e471d into duckdb:main May 19, 2025
@joakimlinde
joakimlinde deleted the feature/array-ingestion branch July 24, 2025 15:00
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants