Add MoveArrowToDataChunk to allow support using arrow in table UDF #47

wmTJc9IK0Q · 2025-11-28T07:01:55Z

This would allow a table UDF to use an arrow to directly fill a data chunk in the FillChunk callback.

wmTJc9IK0Q

Tests TODO, but opening for feedback in conjunction with the linked duckdb-go PR: duckdb/duckdb-go#75

wmTJc9IK0Q · 2025-12-01T05:29:14Z

darwin-amd64/bindings_arrow.go

 // The returned DataChunk must be destroyed with DestroyDataChunk.
 // The returned ErrorData must be checked for errors and destroyed with DestroyErrorData.
-func DataChunkFromArrow(conn Connection, rec arrow.RecordBatch, schema ArrowConvertedSchema) (DataChunk, ErrorData) {
+func NewDataChunkFromArrow(conn Connection, rec arrow.RecordBatch) (DataChunk, ErrorData) {


This was renamed for clarity

I agree that the new name is more clear but unfortunately we need to stay consistent with the function names of the C API header we're wrapping (in case we ever want to generate this package). Also, the duckdb-go-bindings should be as-thin-as-possible of a wrapper of the C API functions - we're now changing the function signature by no longer passing the schema. 🤔

Maybe we can leave the current functions as they are + add one helper functions (MoveArrowToDataChunk)? Maybe two, if we really want to omit the schema, too.

And do we need the helpers on this package's level? I.e., they can't happen higher up, say in duckdb-go?

@taniabogatsch Ok, that design goal makes sense. I think in that case then the right path forward would be to just change the existing DataChunkFromArrow to not create a new data chunk and instead take one to be passed in as an arg (and add back the schema arg). I will move forward with this change.

I think we still want this in this package to avoid all of the C.* and alloc calls in the duckdb-go package?

yes, sounds good. 👍

wmTJc9IK0Q · 2025-12-01T05:29:49Z

darwin-amd64/bindings_arrow.go

+// MoveArrowToDataChunk moves an Arrow RecordBatch into the provided DuckDB DataChunk using the provided Connection and ArrowConvertedSchema.
+// The returned DataChunk must be destroyed with DestroyDataChunk.
+// The returned ErrorData must be checked for errors and destroyed with DestroyErrorData.
+func MoveArrowToDataChunk(conn Connection, rec arrow.RecordBatch, chunk DataChunk) ErrorData {


Why doesn't this also implicitly set the chunk size? Maybe this should happen on the duckdb side?

I am not sure - it could be an oversight when exposing the new functionality? If so, then it should be patched on the DuckDB side.

Copilot

Pull request overview

This PR refactors the Arrow-to-DataChunk conversion API to support table UDFs that need to populate existing DataChunks with Arrow data. It introduces MoveArrowToDataChunk as a new function and renames the existing DataChunkFromArrow to NewDataChunkFromArrow.

Key Changes

Introduced MoveArrowToDataChunk function that moves Arrow RecordBatch data into an existing DataChunk
Renamed DataChunkFromArrow to NewDataChunkFromArrow to better reflect its purpose
Refactored the implementation to separate chunk creation from data population

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 18 comments.

Show a summary per file

File	Description
bindings_arrow.go	Adds `MoveArrowToDataChunk` function and refactors `NewDataChunkFromArrow` to use it
bindings_arrow_test.go	Updates test to call renamed function `NewDataChunkFromArrow`
darwin-amd64/bindings_arrow.go	Platform-specific variant with identical changes
darwin-amd64/bindings_arrow_test.go	Platform-specific test update
darwin-arm64/bindings_arrow.go	Platform-specific variant with identical changes
darwin-arm64/bindings_arrow_test.go	Platform-specific test update
linux-amd64/bindings_arrow.go	Platform-specific variant with identical changes
linux-amd64/bindings_arrow_test.go	Platform-specific test update
linux-arm64/bindings_arrow.go	Platform-specific variant with identical changes
linux-arm64/bindings_arrow_test.go	Platform-specific test update
windows-amd64/bindings_arrow.go	Platform-specific variant with identical changes
windows-amd64/bindings_arrow_test.go	Platform-specific test update

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bindings_arrow.go

darwin-amd64/bindings_arrow.go

darwin-arm64/bindings_arrow.go

linux-arm64/bindings_arrow.go

darwin-arm64/bindings_arrow.go

linux-amd64/bindings_arrow.go

windows-amd64/bindings_arrow.go

linux-amd64/bindings_arrow.go

wmTJc9IK0Q · 2025-12-01T05:48:54Z

Copilot lost it's mind and posted the same 2 comments 12 times.

VGSML · 2025-12-01T06:15:04Z

Thanks! Good point!

VGSML · 2025-12-01T18:58:24Z

For me it would be better to have here the two DuckDB functions: (New)DataChunkFromArrow (with chunk creation) and FillDataChunkFromArrow. @taniabogatsch In any case it should be your decision.

wmTJc9IK0Q · 2025-12-01T19:19:16Z

@VGSML I think we can have these both in the duckdb-go layer and only have one here.

wmTJc9IK0Q · 2025-12-01T19:19:36Z

This isn't working right yet with the duckdb-go PR

taniabogatsch

Just a small nit and then this is good to go from my side. 👍

darwin-amd64/bindings_arrow.go

taniabogatsch · 2025-12-02T14:28:31Z

Looks good from my side - could you just check the failing tests? :)

wmTJc9IK0Q force-pushed the move-arrow branch from 872b075 to 6b01067 Compare December 1, 2025 05:28

wmTJc9IK0Q mentioned this pull request Dec 1, 2025

Add necessary support for arrow-backed table UDFs duckdb/duckdb-go#75

Open

wmTJc9IK0Q commented Dec 1, 2025

View reviewed changes

wmTJc9IK0Q marked this pull request as ready for review December 1, 2025 05:35

Copilot AI review requested due to automatic review settings December 1, 2025 05:35

Copilot started reviewing on behalf of wmTJc9IK0Q December 1, 2025 05:36 View session

Copilot finished reviewing on behalf of wmTJc9IK0Q December 1, 2025 05:40

Copilot AI reviewed Dec 1, 2025

View reviewed changes

Add chunk parameter to DataChunkFromArrow

f36d684

wmTJc9IK0Q force-pushed the move-arrow branch from 62c2043 to f36d684 Compare December 1, 2025 18:43

wmTJc9IK0Q marked this pull request as draft December 1, 2025 19:19

taniabogatsch reviewed Dec 2, 2025

View reviewed changes

darwin-amd64/bindings_arrow.go Show resolved Hide resolved

taniabogatsch marked this pull request as ready for review December 2, 2025 14:15

taniabogatsch approved these changes Dec 2, 2025

View reviewed changes

Add MoveArrowToDataChunk to allow support using arrow in table UDF #47

Are you sure you want to change the base?

Add MoveArrowToDataChunk to allow support using arrow in table UDF #47

Uh oh!

Conversation

wmTJc9IK0Q commented Nov 28, 2025

Uh oh!

wmTJc9IK0Q left a comment

Choose a reason for hiding this comment

Uh oh!

wmTJc9IK0Q Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

wmTJc9IK0Q Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

wmTJc9IK0Q Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

taniabogatsch Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wmTJc9IK0Q commented Dec 1, 2025

Uh oh!

VGSML commented Dec 1, 2025

Uh oh!

VGSML commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wmTJc9IK0Q commented Dec 1, 2025

Uh oh!

wmTJc9IK0Q commented Dec 1, 2025

Uh oh!

taniabogatsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taniabogatsch commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VGSML commented Dec 1, 2025 •

edited

Loading