R: Full support of lists and structs in R #8503

krlmlr · 2023-08-07T14:51:11Z

This adds support for lists of atomic vectors. All elements must be of the same type for this to work. The commits make sense individually, but mostly depend on each other.

ALTREP support can be built on top of that.

Closes #2841.

Tmonster

Looks good to me. Would just like to see more test cases for other types.

If I'm not mistaken, nested types are registered per every nested child object?

tools/rpkg/src/types.cpp

tools/rpkg/tests/testthat/test_register.R

tools/rpkg/src/scan.cpp

tools/rpkg/tests/testthat/test_struct.R

tools/rpkg/src/scan.cpp

Tmonster · 2023-08-08T09:15:13Z

tools/rpkg/src/include/typesr.hpp

+
+private:
+	RTypeId id_;
+	child_list_t<RType> aux_;


what does aux_ stand for in this case? Just auxiliary?

Yes, auxiliary information. Can you think of a better name?

maybe child_Rtypes? auxiliary feels to broad to me

aux_ holds the list child type, struct child types and names, or factor levels. It's really a union type, but luckily everything fits into the child_list_t . Could be extra_, or perhaps child_types_or_enum_levels_, or a proper virtual base class with subclasses (but then how do we name that member?).

tools/rpkg/src/scan.cpp

krlmlr · 2023-08-08T14:30:42Z

nested types are registered per every nested child object

Can you please rephrase?

krlmlr · 2023-08-08T14:38:01Z

I'll follow up with more tests.

krlmlr · 2023-08-08T15:35:47Z

I'm not too sure about my usage of std::move() .

Tmonster

Was trying to test around and it seems like the following fails

df <- vctrs::data_frame(a = 1:4, b = list(4:6, 2:3, 1L, 5))
dbWriteTable(con, "df", df)

while

df <- vctrs::data_frame(a = 1:4, b = list(4:6, 2:3, 1L, 5L))
dbWriteTable(con, "df", df)

The error is Error: rapi_execute: Can't convert R type. I know that all nested structs/lists need the same type, but I think the user would appreciate feedback in this case that we don't yet support two types in the same nested object.

Might also be nice to have a RTypeToString() method for debugging? Possibly also for the above mentioned case?

Should we also test nested lists/structs that have above the default chunk size?

Tmonster · 2023-08-09T09:11:46Z

I'm not too sure about my usage of std::move() .

Your usage of move() looks ok to me

krlmlr · 2023-08-09T09:16:23Z

Can you please construct a data frame that would exceed the chunk size limits?

Happy to tackle the detailed comments in a follow-up PR. The vctrs package in R also has a list_of type -- we can detect objects of this class (no need to drag in a dependency) to avoid scanning the entire list.

Tmonster · 2023-08-10T13:10:41Z

Can you please construct a data frame that would exceed the chunk size limits?
Meant the STANDARD_VECTOR_SIZE, but it doesn't seem to be an issue.

Happy to tackle the detailed comments in a follow-up PR. The vctrs package in R also has a list_of type -- we can detect objects of this class (no need to drag in a dependency) to avoid scanning the entire list.

Yea, I think a follow up PR would be fine, this looks good to me.

hannes · 2023-08-16T10:44:28Z

Looks good to me, can we maybe now remove some of the excludes in test_types.R?

krlmlr · 2023-08-16T16:44:07Z

Absolutely. Can we do that in a follow-up PR?

krlmlr · 2023-08-17T09:44:45Z

(FWIW, test-types.R isn't affected by this PR. We can already output nested types today, this is about ingestion.)

Co-authored-by: Kirill Müller <kirill@cynkra.com>

R: Full support of lists and structs in R

- Merge pull request duckdb/duckdb#8378 from Mytherin/enumcatalogremoval: Format serialization - add missing serialization logic for DelimJoin, AsOfJoin, CreateIndex and enums - Merge pull request duckdb/duckdb#8503 from krlmlr/f-2841-nested - Merge pull request duckdb/duckdb#8626 from carlopi/fixR: R: Fix warning on mismatched integer comparison

krlmlr requested a review from hannes August 7, 2023 14:51

krlmlr changed the title ~~R: Towards full support of lists and structs in R~~ R: Full support of lists and structs in R Aug 8, 2023

krlmlr mentioned this pull request Aug 8, 2023

R list column support (R is freezing) #2841

Closed

krlmlr force-pushed the f-2841-nested branch from 72f083c to 3b76976 Compare August 8, 2023 04:30

github-actions bot marked this pull request as draft August 8, 2023 04:31

Tmonster self-requested a review August 8, 2023 07:29

Tmonster reviewed Aug 8, 2023

View reviewed changes

krlmlr requested a review from Tmonster August 8, 2023 15:35

Tmonster reviewed Aug 9, 2023

View reviewed changes

krlmlr marked this pull request as ready for review August 9, 2023 09:13

krlmlr force-pushed the f-2841-nested branch from 2957699 to fbc2065 Compare August 9, 2023 09:17

github-actions bot marked this pull request as draft August 9, 2023 09:17

krlmlr marked this pull request as ready for review August 9, 2023 09:17

Tmonster self-requested a review August 10, 2023 13:10

Tmonster approved these changes Aug 10, 2023

View reviewed changes

github-actions bot marked this pull request as draft August 17, 2023 12:08

krlmlr added 5 commits August 17, 2023 14:09

Fix registration of list of NULLs

6526570

Extract RTypeId

e8f9bdb

Implement RType::LIST()

386d0fe

RType::FACTOR stores the levels

9bbed31

Add LogicalTypeFromRType()

3094e9e

krlmlr and others added 22 commits August 17, 2023 14:09

Support nested lists of atomic vectors

0b7830e

Improve error message

424d6e0

RApiTypes::GetVecSize()

00da412

Use GetVecSize() for binding

398338a

Move responsibility

e200579

Extract AppendAnyColumnSegment()

2f82076

Use GetVecSize() for LIST

1f1fbe1

Extract GetColDataPtr()

c722c51

Define RType::STRUCT()

cad8d55

Implement RType::STRUCT()

348eb55

Binding for STRUCT

ae48050

Logical type for STRUCT

1b6b07b

Scanning for STRUCT

561007a

STRUCT to Value

44553f9

LIST to Value

188a502

Add test

1026cde

Pull out of loop

1a63874

Rename variables

2cf22de

Explain

a392344

Add test for mixed types failure

5fe262d

Test all datatypes

06c30e8

avoid copy

b28e890

Co-authored-by: Kirill Müller <kirill@cynkra.com>

krlmlr force-pushed the f-2841-nested branch from d0e3a37 to b28e890 Compare August 17, 2023 12:10

krlmlr marked this pull request as ready for review August 17, 2023 12:10

hannes merged commit dc25e98 into duckdb:master Aug 18, 2023
9 checks passed

krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023

Merge pull request duckdb/duckdb#8503 from krlmlr/f-2841-nested

126cb2a

R: Full support of lists and structs in R

krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023

Merge pull request duckdb/duckdb#8503 from krlmlr/f-2841-nested

ff554f9

R: Full support of lists and structs in R

krlmlr pushed a commit to krlmlr/duckdb-r that referenced this pull request Sep 2, 2023

Merge pull request duckdb/duckdb#8503 from krlmlr/f-2841-nested

03a1559

R: Full support of lists and structs in R

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R: Full support of lists and structs in R #8503

R: Full support of lists and structs in R #8503

krlmlr commented Aug 7, 2023 •

edited

Tmonster left a comment

Tmonster Aug 8, 2023

krlmlr Aug 8, 2023

Tmonster Aug 9, 2023

krlmlr Aug 9, 2023

krlmlr commented Aug 8, 2023

krlmlr commented Aug 8, 2023

krlmlr commented Aug 8, 2023

Tmonster left a comment

Tmonster commented Aug 9, 2023

krlmlr commented Aug 9, 2023

Tmonster commented Aug 10, 2023

hannes commented Aug 16, 2023

krlmlr commented Aug 16, 2023

krlmlr commented Aug 17, 2023

R: Full support of lists and structs in R #8503

R: Full support of lists and structs in R #8503

Conversation

krlmlr commented Aug 7, 2023 • edited

Tmonster left a comment

Choose a reason for hiding this comment

Tmonster Aug 8, 2023

Choose a reason for hiding this comment

krlmlr Aug 8, 2023

Choose a reason for hiding this comment

Tmonster Aug 9, 2023

Choose a reason for hiding this comment

krlmlr Aug 9, 2023

Choose a reason for hiding this comment

krlmlr commented Aug 8, 2023

krlmlr commented Aug 8, 2023

krlmlr commented Aug 8, 2023

Tmonster left a comment

Choose a reason for hiding this comment

Tmonster commented Aug 9, 2023

krlmlr commented Aug 9, 2023

Tmonster commented Aug 10, 2023

hannes commented Aug 16, 2023

krlmlr commented Aug 16, 2023

krlmlr commented Aug 17, 2023

krlmlr commented Aug 7, 2023 •

edited