feat: Add support for duckdb arrays in R by joakimlinde · Pull Request #1090 · duckdb/duckdb-r

joakimlinde · 2025-04-07T19:28:02Z

Limited to one-dimensional duckdb arrays for now.

Closes #102

joakimlinde · 2025-04-07T20:04:34Z

Usage example:

library(tidyverse)
library(duckdb)

con <- dbConnect(duckdb())
on.exit(dbDisconnect(con, shutdown = TRUE))

dbExecute(con, "CREATE TABLE tbl (a INTEGER, b INTEGER[3])")
dbExecute(con, "INSERT INTO tbl VALUES (10, [1, 4, 7])")
dbExecute(con, "INSERT INTO tbl VALUES (11, [2, 5, 8])")
dbExecute(con, "INSERT INTO tbl VALUES (12, [3, 6, 9])")

df <- as_tibble(dbGetQuery(con, "FROM tbl"))
df
#> # A tibble: 3 × 2
#>       a b[,1]  [,2]  [,3]
#>   <int> <int> <int> <int>
#> 1    10     1     4     7
#> 2    11     2     5     8
#> 3    12     3     6     9

df %>% filter(a == 11)
#> # A tibble: 1 × 2
#>       a b[,1]  [,2]  [,3]
#>   <int> <int> <int> <int>
#> 1    11     2     5     8

df %>% arrange(desc(a))
#> # A tibble: 3 × 2
#>       a b[,1]  [,2]  [,3]
#>   <int> <int> <int> <int>
#> 1    12     3     6     9
#> 2    11     2     5     8
#> 3    10     1     4     7

krlmlr

Thanks, got halfway through, will follow up. Why are the checks failing?

krlmlr · 2025-04-08T04:03:05Z

+		// This one is for speed. Maybe a tiny bit faster if the optimzer can figure it out. 	
+		for (size_t row_idx = 0; row_idx < count; row_idx++) {
+			dest_ptr[row_idx] = !mask.RowIsValid(row_idx) ? na_val : src_ptr[row_idx];
+		}
+	} else {
+		for (size_t row_idx = 0; row_idx < count; row_idx++) {
+			dest_ptr[0] = !mask.RowIsValid(row_idx) ? na_val : src_ptr[row_idx];
+			dest_ptr += dest_step_size;
+		}


IIUC, the difference between the two loops is between ++ and += dest_step_size ?

I'd like to look at the generated code before optimizing at this fine-grained level. My gut feeling says it won't matter because there is a lot of unpredictable branching going on due to the ? inside the loop, and that for modern compilers/processors the += is not the main bottleneck.

I wasn't sure how sensitive you were to performance so I provided both as options. I'll remove the special case of 1 and let the generic one handle it.

krlmlr · 2025-04-08T04:03:34Z

+		auto array_size = ArrayType::GetSize(type);
+		auto &child_type = ArrayType::GetChildType(type);
+		if (child_type.IsNested())
+			cpp11::stop("rapi_execute: Arrays must not be nested.");


"Nested arrays cannot be returned to R as column data" or similar?

What's a good wording here? DuckDB supports it, only we don't yet.

Error messages are always hard. You want to capture everything in a short sentence. I like your proposed text and will update.

joakimlinde · 2025-04-08T05:47:13Z

The tests are not failing, they need to be approved to run. I see "1 workflow awaiting approval. This workflow requires approval from a maintainer." More info here.

krlmlr

Thanks. Are we covering ingestion (going from R to DuckDB) in a separate PR?

krlmlr · 2025-04-08T05:49:23Z

+		auto array_size = ArrayType::GetSize(type);
+		auto &child_type = ArrayType::GetChildType(type);
+		if (child_type.IsNested())
+			cpp11::stop("rapi_execute: Arrays must not be nested.");


Same as above. Why do we need to repeat the error message here? Can we get away with checking just once?

From my perspective we don't so I'll remove it. The default in the case statement has an error message so I added one just to match.

krlmlr · 2025-04-08T05:56:30Z

+		// See: https://svn.r-project.org/R/trunk/src/main/attrib.c:656
+		// SET_CLASS(dest, RStrings::get().matrix_array_str);
+		cpp11::sexp dims = NEW_INTEGER(2);
+		INTEGER(dims)[0] = nrows;


I wonder if we can infer nrows from Rf_xlength(dest) and child_type, to avoid threading it through in many places.

That's a good point. Let me try that. It would simplify things.

krlmlr · 2025-04-08T06:01:52Z

+			dest_ptr[row_idx] = !mask.RowIsValid(row_idx) ? na_val : src_ptr[row_idx];
+		}
+	} else {
+		for (size_t row_idx = 0; row_idx < count; row_idx++) {


Shouldn't this be a nested loop? The loop over row_idx iterates over rows, and in each row, we need to populate a matrix row? What am I missing?

VectorToR() takes one DuckDB array in one DuckDB record and writes it to one row in the R matrix we are building. The for loop you are looking for, the one that loops over all the records in the DuckDB table, is statement.cpp:181.

idx_t dest_offset = 0; for (auto &chunk : result->Collection().Chunks()) { D_ASSERT(chunk.ColumnCount() == ncols); D_ASSERT(chunk.ColumnCount() == (idx_t)Rf_length(data_frame)); for (size_t col_idx = 0; col_idx < chunk.ColumnCount(); col_idx++) { SEXP dest = VECTOR_ELT(data_frame, col_idx); duckdb_r_transform(chunk.data[col_idx], dest, dest_offset, 1, chunk.size(), integer64); } dest_offset += chunk.size(); }

It seems like you think each DuckDB record generates a matrix but that's not the case. It's the whole column with an array in each record that forms the matrix.

joakimlinde · 2025-04-08T06:08:27Z

We can do ingestion too? I missed that. I'll add it.

krlmlr · 2025-04-08T06:08:46Z

Let's do it in a separate PR then.

krlmlr · 2025-04-08T06:09:14Z

Maybe we can simplify this PR a bit if we don't ingest here?

joakimlinde · 2025-04-08T07:06:47Z

Sure, I'll add ingestion in a separate PR.

joakimlinde · 2025-04-08T17:26:05Z

I've updated the submission to address the comments as discussed.

krlmlr

Thanks. IIUC, getting rid of the idx_t dest_step_size argument to duckdb_r_transform() requires a temporary buffer. I'm fine with that because I think the true optimization potential is in improving cache efficiency.

krlmlr · 2025-04-08T18:31:12Z

+		}
+		break;


For consistency:

Suggested change

}

break;

break;

}

I see what you mean. Let me fix that.

krlmlr · 2025-04-08T19:13:56Z

+			size_t offset = (row_idx * array_size); 
+			size_t end = offset + array_size;
+			child_vector.Slice(ArrayVector::GetEntry(src_vec), offset, end);
+			duckdb_r_transform(child_vector, dest, dest_offset + row_idx, matrix_nrow, array_size, integer64);


I now see that duckdb_r_transform() is employed in two ways:

Iterating over rows

Iterating over all values in an array row

Essentially, we're implementing matrix transposition here, which I remember to be notoriously cache-inefficient.

Because of this, we can as well use a temporary buffer here (that saves us the dest_step_size argument everywhere) and distribute the values from that temporary buffer to the correct indices here. As a next step, we can then switch to process in 16x16 blocks or so; the buffer will then fit in the L1 cache, and the extra copy won't matter that much.

Would you like to go the extra mile here?

I would like to get basic functionality in place first and then do performance optimizations based on measurements. Then I can rewrite the code and make sure I've improved performance instead of making it worse.

My main concern with this PR is that we add an argument to a function that is called 10+ times, and we need to thread through that argument, just to cater for the new type (edge case?). It seems that, if we go with a buffer, we can simplify a lot and the diff of the PR gets much smaller. This seems consistent with getting in basic functionality first -- I need to balance this with code simplicity. I'm happy to put in that change, also happy to review yours.

May I suggest that we do both. If you think your solution will be significant faster, I encourage you to implement it, measure it, and to compare it to the existing (basic) solution. Then you pick the faster one. I then get the option of improving on the better of the two. Sounds good?

Joking aside, I understand and appreciate your concern regarding performance. It's a valid concern and it stems from how matrices are laid out in memory in R. A matrix row is scattered all over memory. Each element is nrows apart. Where we differ is that I don't think you can work around this problem — it's intrinsic to R matrices. Either your reads are going to be scattered or your writes are going to be scattered — pick your poison. This is a valid argument for using lists instead of matrices in R to represent DuckDB arrays.

joakimlinde · 2025-04-08T20:33:37Z

I've fixed the consistency issue and re-submitted.

krlmlr · 2025-04-09T18:37:00Z

Thanks!

krlmlr reviewed Apr 8, 2025

View reviewed changes

Comment thread tests/testthat/test-array.R Outdated

joakimlinde force-pushed the feature/array branch 2 times, most recently from e7aee07 to f593648 Compare April 8, 2025 16:58

krlmlr reviewed Apr 8, 2025

View reviewed changes

Add support for duckdb arrays in R

e8ebfd7

joakimlinde force-pushed the feature/array branch from f593648 to e8ebfd7 Compare April 8, 2025 20:28

krlmlr added 7 commits April 9, 2025 19:13

Merge branch 'main' into feature/array

a9aeeb4

Spaces

7493980

Extract function

18459a9

Use temporary buffer

60f4926

Remove dest_step_size

e75a091

Restore

ec8c487

Restore

728420a

krlmlr changed the title ~~Add support for duckdb arrays in R~~ feat: Add support for duckdb arrays in R Apr 9, 2025

krlmlr enabled auto-merge (squash) April 9, 2025 18:36

krlmlr disabled auto-merge April 9, 2025 18:36

krlmlr merged commit f6af94c into duckdb:main Apr 9, 2025

joakimlinde deleted the feature/array branch July 24, 2025 14:59

-		}
-		break;
+		break;
+	}

Uh oh!

Conversation

joakimlinde commented Apr 7, 2025

Uh oh!

joakimlinde commented Apr 7, 2025

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakimlinde Apr 8, 2025 • 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!

joakimlinde commented Apr 8, 2025

Uh oh!

krlmlr 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!

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!

joakimlinde Apr 8, 2025 • 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!

joakimlinde commented Apr 8, 2025

Uh oh!

krlmlr commented Apr 8, 2025

Uh oh!

krlmlr commented Apr 8, 2025

Uh oh!

Uh oh!

joakimlinde commented Apr 8, 2025

Uh oh!

joakimlinde commented Apr 8, 2025

Uh oh!

krlmlr 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!

krlmlr Apr 8, 2025 • 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!

joakimlinde commented Apr 8, 2025

Uh oh!

krlmlr commented Apr 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

joakimlinde Apr 8, 2025 •

edited

Loading

joakimlinde Apr 8, 2025 •

edited

Loading

krlmlr Apr 8, 2025 •

edited

Loading