Skip to content

Conversation

lmmx
Copy link

@lmmx lmmx commented Aug 26, 2025

I was going to continue with the other DB dependency bumps (outlined in #843) but I thought best to initially verify thisi s sound.

I can't actually see any tests for rusqlite in here, perhaps these ought be contributed too?

@lmmx
Copy link
Author

lmmx commented Aug 26, 2025

I tried building a crate depending on this branch and it stopped due to an error that may be unrelated

evaluation panicked: Your generated code is out of date, please regenerate using planus version 1.2.0

reported from polars-arrow-format

It looks like we need a planus crate pin now to =1.1.1 (pola-rs/polars#24208)

@lmmx
Copy link
Author

lmmx commented Aug 26, 2025

Confirmed with a local setup that bumping these versions and pinning Planus compiles OK and rusqlite/connectorx integration tests pass 👍

I don't know if pinning a problematic dependency like Planus is good practice for crates so I've not done so in the PR, but it requires this to build:

planus = { version = "=1.1.1" }

I'm thinking perhaps I should contribute a test suite for sqlite though rather than having to do testing via my downstream project

@wangxiaoying
Copy link
Contributor

Thank you @lmmx for the PR!

I'm thinking perhaps I should contribute a test suite for sqlite though rather than having to do testing via my downstream project

That would be great. Please feel free to add tests to rust. This is definitely something missing.

Currently we do have some simple tests for sqlite in python, which is also included in the ci for sqlite. The scripts for creating the test tables can be found here.

Please let me know if you need any help from my side!

@lmmx
Copy link
Author

lmmx commented Aug 26, 2025

Thank you @lmmx for the PR!

Thank you for taking a look over it! 😁 🙏

I am just reviewing it more closely and spotted this was in the original connectorx :

rust_decimal = {version = "1", features = ["db-postgres"], optional = true}

So when I turn on the rust_decimal feature for dst_arrow that is going to turn on the rust_decimal/db-postgres dependency feature, which we don't want! I'll revise that to ensure it's done properly.

I'm thinking perhaps I should contribute a test suite for sqlite though rather than having to do testing via my downstream project

That would be great. Please feel free to add tests to rust. This is definitely something missing.

Yes I have some in here which are essentially just integration tests in Rust for the rusqlite and arrow layer, this would be easy to "unwrap" back into pure connectorx.

Currently we do have some simple tests for sqlite in python, which is also included in the ci for sqlite. The scripts for creating the test tables can be found here.

Thank you for linking me, I'll take a look and try to keep the Rust scripts to the same style :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants