Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DuckDB v1.2.0 and removing any duckdb.h dependencies #364

Merged
merged 80 commits into from
Mar 4, 2025
Merged

DuckDB v1.2.0 and removing any duckdb.h dependencies #364

merged 80 commits into from
Mar 4, 2025

Conversation

taniabogatsch
Copy link
Collaborator

@taniabogatsch taniabogatsch commented Feb 12, 2025 •
edited
Loading

Breaking changes

- This PR drops support for FreeBSD, as duckdb does not build and publish any artifacts.
- This PR makes the arrow dependency opt-in.

This PR significantly changes go-duckdb's internals and causes a few breaking changes, as outlined above.
Any C.duckdb... call has been moved to call into https://github.com/duckdb/duckdb-go-bindings.

Open ToDo's

  • Guide on building and linking a custom FreeBSD static library based on the duckdb code.
  • Make Arrow opt-in.
  • Test duckdb-go-bindings with a custom static/dynamic lib.
  • Test go-duckdb with a custom static/dynamic lib.
  • CI run to duplicate mapping.go for each pre-built combination.
  • CI run to test linux_arm64
  • Run Instruments with Leak detection
  • Update README

Future Work

  • Remove any remaining C imports in the core files.
  • Provide a flag to build this project without importing "C".

Related Issues / Discussions

@taniabogatsch taniabogatsch added feature / enhancement Code improvements or a new feature build Building go-duckdb dependencies Updates a dependency file go Updates Go code CI / infra CI or other project infrastructure changes labels Feb 12, 2025
@taniabogatsch taniabogatsch changed the title Remove any duckdb.h dependencies by importing duckdb-go-bindings modules DuckDB v1.2.0 and removing any duckdb.h dependencies Feb 12, 2025
@james-lawrence james-lawrence mentioned this pull request Feb 13, 2025
Open
@taniabogatsch
Copy link
Collaborator Author

Hi Marc! Thanks for the detailed review! I am working on implementing your suggestions. I'll move some things into separate PRs and merge them (like the Go version and the mappings) so we can further decrease the scope of this one.

@taniabogatsch taniabogatsch mentioned this pull request Feb 28, 2025
Merged
taniabogatsch
taniabogatsch commented Feb 28, 2025
View reviewed changes
Copy link
Collaborator Author

@taniabogatsch taniabogatsch left a comment

Choose a reason for hiding this comment

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

@marcboeker @JAicewizard, I can review this now. It loads slowly, but all the files and changes are there. Does it work better for you, now, too? I tried Brave and Safari.

types.go Outdated
lower: C.uint64_t(r.Uint64()),
upper: C.int64_t(q.Int64()),
}, nil
var hugeInt m.HugeInt
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, the bindings module only exposes Get and Set for each field individually. Should we add New... methods to the bindings module?

JAicewizard
JAicewizard reviewed Mar 1, 2025
View reviewed changes
Copy link
Contributor

@JAicewizard JAicewizard left a comment

Choose a reason for hiding this comment

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

Mostly small nits, looks really good!

appender.go
import (
"database/sql/driver"
"errors"
"unsafe"

"github.com/marcboeker/go-duckdb/mapping"
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit, but reading mapping.XXX is like 1% confusing everytime I read it. Maybe make this a named import and call it duckdb (or ddb to avoid confusion with this package)? not sure what others think of it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am pinging @marcboeker about this. I don't have a strong opinion about the name, so I would choose whatever you (two) prefer. :)

Copy link
Owner

Choose a reason for hiding this comment

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

What about api as it was before, but now in its own package instead of the duckdb package? Or mapper? @JAicewizard what confuses you about mapping? That it is too generic?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah mapping is too generic, arrowmapping is an example that better IMO since it clearly states that it is for arrow.

@marcboeker
Copy link
Owner

@taniabogatsch Maybe we can change the module to include to a v2, merge it into a v2.0.0 branch and iterate on this one. Then we have a v2 which could be used by people and we can gather more feedback.
I would add a note, that the v2 is still experimental and its API subject to change. But I think we need to get this on the road.

@taniabogatsch
Copy link
Collaborator Author

@marcboeker, that sounds good. I'll finish this up, merge it into the v2-preview branch, and push a tag. I'll also move the remaining comments into an issue so we can merge them separately.

Nit: Could you clean up the branches (I don't have permission to delete a branch, not even my own, haha)? There are quite a few stale ones, which might be confusing.

  • These can all go, I think: pre-release, download-libs, windows, refactor-decimal-tests
  • And maybe also the dependabot things, but I'm not sure what they do.

@taniabogatsch taniabogatsch changed the base branch from main to v2-preview March 4, 2025 08:07
@taniabogatsch taniabogatsch mentioned this pull request Mar 4, 2025
Open
3 tasks
@taniabogatsch taniabogatsch merged commit 381ff2b into marcboeker:v2-preview Mar 4, 2025
16 checks passed
@taniabogatsch taniabogatsch deleted the bindings branch March 4, 2025 08:24
@taniabogatsch
Copy link
Collaborator Author

taniabogatsch commented Mar 4, 2025 •
edited
Loading

The changes in this PR can now be used/tried out via _ "github.com/marcboeker/go-duckdb/v2". 🥳
They're on the v2-preview branch: https://github.com/marcboeker/go-duckdb/tree/v2-preview.

@taniabogatsch
Copy link
Collaborator Author

I also added a small note to the README.md on the main branch.
Screenshot 2025-03-04 at 09 31 26

@JAicewizard
Copy link
Contributor

Nice work! Must have been a pain to work on this

@james-lawrence
Copy link

this is wonderful news! =) looking forward to updating!

@PadenZach PadenZach mentioned this pull request Mar 5, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcboeker marcboeker marcboeker left review comments

@james-lawrence james-lawrence james-lawrence left review comments

@JAicewizard JAicewizard JAicewizard left review comments

Assignees
No one assigned
Labels
build Building go-duckdb CI / infra CI or other project infrastructure changes dependencies Updates a dependency file feature / enhancement Code improvements or a new feature go Updates Go code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@taniabogatsch @marcboeker @JAicewizard @james-lawrence