New arrow C-API implementation #50

VGSML · 2025-11-04T21:12:01Z

Rewrite the Arrow.QueryContext method to use new DuckDB Arrow C-API
Closes #5

VGSML · 2025-11-04T21:13:01Z

Merge after: duckdb/duckdb-go-bindings#39
And bindings versions

VGSML · 2025-11-04T21:14:36Z

@taniabogatsch FYI

taniabogatsch · 2025-11-11T20:46:54Z

Thanks! Will have a look once we've released the bindings with the latest changes. 👍

…_c_api

VGSML · 2025-11-17T18:08:48Z

@taniabogatsch Could you take a look this PR? What the sequence to merge it, should I the arrowmapping version increase in the duckdb package before merge or split pr to 2 - arrowmapping changes and the changes in the duckdb package?

taniabogatsch · 2025-11-17T18:59:33Z

I think your duckdb-go-bindings changes should already be on main! :)

taniabogatsch · 2025-11-17T18:59:38Z

We pushed a release today.

VGSML · 2025-11-17T19:19:35Z

Yes, I sow. I mean arrowmapping package in this repo.

taniabogatsch · 2025-11-17T19:25:00Z

ah, I see now! We likely need to bump it in a separate PR - i.e., first one with the new functions in the arrow mapping package, then I can bump that version. 👍

…_c_api

VGSML · 2025-11-21T07:22:32Z

@taniabogatsch Very strange with tests when the go version 1.24 is specified in the go.mod, the tests are failed. I have created an issue to update the go version to 1.25 in the CI and packages. #63

go.mod

VGSML · 2025-11-21T11:09:46Z

@taniabogatsch I revert transitive dependency versions. The failed tests was fixed
You can merge this PR.

VGSML · 2025-11-21T14:26:09Z

@taniabogatsch Will you merge it?

taniabogatsch · 2025-11-22T14:14:21Z

Yes, but I'll need to review it first. 👍 Didn't find the time to take a look yet, will so next week.

taniabogatsch

Thanks for the PR / implementation - I saw that its not a huge PR, so I squeezed in a review already ;)

taniabogatsch · 2025-11-22T14:24:21Z

arrow_test.go

Is this a breaking change? I.e., can people still use rdr.Record() and rdr.(*arrowStreamReader) in their implementations? Or would we break compatibility with this change as it currently is?

Hi, no, there are no breaking changes, the public methods are still the same.

taniabogatsch · 2025-11-22T14:27:56Z

arrow.go

+	if err != nil {
+		return nil, err
+	}


Should we call rows.Close() here before returning the error?

It's still using in the record reader and will be closed by last recordReader.Release. So it should be in non-closed state here

taniabogatsch · 2025-11-22T14:29:45Z

arrow.go

We're removing a bunch of exported functionality here - let's instead tag them as deprecated? And add the new functionality on top?

No, I just moved the public method to the top of the file. There are no braking changes

taniabogatsch · 2025-11-22T14:31:50Z

arrow.go

+	}
+	if arrowmapping.ArrowScan(a.conn.conn, name, arrowStream) == mapping.StateError {
+		release()
+		return nil, errors.New("duckdb_arrow_scan")


Can we turn this into a bit more elaborate error? I.e., that something went wrong in the DuckDB-side? Also, can we trigger this in a test? If not, should we tell people to file a bug report if they run into it?

Ok, but this part of the code is not changed. There is no method to get the real error in the ArrowScan (I think in the future it should be deprecated).

I want to add the new type table function - ArrowTableUDF, that will use the new C-API, and deprecated this method. With this new UDF and replacement scan we can provide the same functionality to have arrow view.

The main difference will be in the options to delete the view definition after using. Do you know, the C-API will contain the method to deregister replacement scans and UDFs?

taniabogatsch · 2025-11-22T14:34:37Z

arrow.go

+	defer mapping.DestroyErrorData(&ed)
+	if mapping.ErrorDataHasError(ed) {
+		return nil, fmt.Errorf("failed to create arrow schema: %w",
+			getDuckDBError(mapping.ErrorDataMessage(ed)))
+	}


Use func errorDataError(errorData mapping.ErrorData) error here.

taniabogatsch · 2025-11-22T14:35:19Z

arrow.go

+
+	schema, ed := arrowmapping.NewArrowSchema(arrowOptions, types, names)
+	defer mapping.DestroyErrorData(&ed)
+	if mapping.ErrorDataHasError(ed) {


If we return with an error here, then we also need to destroy the arrow options, no?

taniabogatsch · 2025-11-22T14:36:20Z

arrow.go

+type duckdbArrowReader struct {
+	ctx    context.Context
+	res    mapping.Result
+	opts   *arrowmapping.ArrowOptions


I don't think we need a pointer (heap allocation) here - the arrow mapping.ArrowOptions are already wrapping a pointer, which we can just copy.

taniabogatsch · 2025-11-22T14:37:09Z

arrow.go

-			r.currentRec = nil
+		defer mapping.DestroyDataChunk(&chunk)
+		rec, ed := arrowmapping.DataChunkToArrowArray(*r.opts, r.schema, chunk)
+		defer mapping.DestroyErrorData(&ed)


Same: func errorDataError(errorData mapping.ErrorData) error.

VGSML · 2025-11-23T13:18:21Z

The PR just replaces the internal implementation of the Arrow.QueryContext method and record reader implementation.

I also rename the reader to make it simple.

…o feat/new_arrow_c_api

feat: new arrow C-API implementation

6f9b12c

VGSML added 2 commits November 4, 2025 22:25

fix: update arrow options retrieval in recordReaderFromRows function

76fe5d2

fix: due bindings API change

3f68d21

VGSML changed the base branch from main to v2.6.0-preview November 16, 2025 13:22

VGSML changed the base branch from v2.6.0-preview to main November 17, 2025 10:01

Merge branch 'main' of github.com:VGSML/duckdb-go into feat/new_arrow…

dfcc1ef

…_c_api

taniabogatsch self-requested a review November 17, 2025 18:59

VGSML added 4 commits November 17, 2025 20:45

feat: move arrow mapping changes to the separate PR

7adfdbc

Merge branch 'main' of github.com:VGSML/duckdb-go into feat/new_arrow…

d12e54d

…_c_api

fix: update dependencies in go.mod and go.sum

5ae7658

fix: specify Go version as 1.24.0 in go.mod

3fc5d4c

VGSML mentioned this pull request Nov 21, 2025

Mappings: Bump Go version to 1.25 and update dependencies #64

Closed

taniabogatsch reviewed Nov 21, 2025

View reviewed changes

go.mod Show resolved Hide resolved

fix: update indirect dependencies in go.mod and go.sum

f0fbb66

VGSML mentioned this pull request Nov 21, 2025

Draft: Update dependencies for arrow mapping and mapping modules #65

Open

taniabogatsch requested changes Nov 22, 2025

View reviewed changes

taniabogatsch added feature / enhancement Code improvements or a new feature dependencies Updates a dependency file labels Nov 22, 2025

taniabogatsch added the changes requested Changes have been requested to a PR or issue label Nov 22, 2025

fix: review

3768a02

VGSML and others added 3 commits November 23, 2025 14:18

Merge branch 'main' into feat/new_arrow_c_api

bcfac1e

fix: lint

bc3fb5d

Merge branch 'feat/new_arrow_c_api' of github.com:VGSML/duckdb-go int…

5226990

…o feat/new_arrow_c_api

VGSML requested a review from taniabogatsch November 23, 2025 13:26

New arrow C-API implementation #50

Are you sure you want to change the base?

New arrow C-API implementation #50

Conversation

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

Uh oh!

Uh oh!

VGSML commented Nov 4, 2025

Uh oh!

VGSML commented Nov 4, 2025

Uh oh!

taniabogatsch commented Nov 11, 2025

Uh oh!

VGSML commented Nov 17, 2025

Uh oh!

taniabogatsch commented Nov 17, 2025

Uh oh!

taniabogatsch commented Nov 17, 2025

Uh oh!

VGSML commented Nov 17, 2025

Uh oh!

taniabogatsch commented Nov 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

VGSML commented Nov 21, 2025

Uh oh!

taniabogatsch commented Nov 22, 2025

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

VGSML Nov 23, 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!

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!

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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

VGSML commented Nov 4, 2025 •

edited

Loading

VGSML commented Nov 21, 2025 •

edited

Loading

VGSML commented Nov 21, 2025 •

edited

Loading

VGSML Nov 23, 2025 •

edited

Loading

VGSML commented Nov 23, 2025 •

edited

Loading