Skip to content
New issue

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

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

Already on GitHub? # to your account

Update duckdb to 1.0 & setup tests #1

Closed
wants to merge 2 commits into from

Conversation

Binlogo
Copy link
Contributor

@Binlogo Binlogo commented Jul 22, 2024

Hi, @0xcaff, I'm happy to experiment with this crate. I've made some updates, hope them useful.


This PR updates the duckdb-rs crate to version 1.0 and introduces integration tests for the DuckDB Protobuf extension. The tests ensure that the extension can correctly parse and query protobuf data. The setup includes compiling the protobuf schema, generating test data, and querying the data using DuckDB.

Changes Made

  1. Update duckdb to 1.0:

    • Updated the duckdb crate to version 1.0 in Cargo.toml and relative dependencies.
  2. Protobuf Schema Definition:

    • Added user.proto file defining the protobuf schema for User messages.
  3. Protobuf Compilation and Test Data Generation:

    • Created a setup function in the integration test to compile the protobuf schema and generate test data.
    • The setup function ensures that the protobuf schema is compiled and the test data is generated before running the tests.
  4. Integration Tests:

    • Added tests to verify that the protobuf files are created correctly.
    • Added tests to query the protobuf data using DuckDB and validate the results.

Instructions for Testing

  1. Build the DuckDB Protobuf Extension:

    • Ensure that the DuckDB Protobuf extension is built correctly by running:
      cargo build --release
  2. Run the Integration Tests:

    • Run the integration tests using the following command:
      cargo test
  3. Verify the Results:

    • Ensure that the tests pass and the protobuf data is queried correctly.
    • Check the output of the test_query_protobuf_data test to verify that the queried data matches the expected results.

[Expected] Example Output

The test_query_protobuf_data test should print the following output, indicating that the data was queried correctly:

name: Alice, id: 1, created_at: ...
name: Bob, id: 2, created_at: ...
name: Charlie, id: 3, created_at: ...

Issues to resolve

  • FIXME: The file is not a DuckDB extension. The metadata at the end of the file is invalid

Please review the changes and provide feedback, particularly on the unexpected issue encountered🤓. Thank you! 🙏

@0xcaff
Copy link
Owner

0xcaff commented Jul 22, 2024

hey @Binlogo thanks for taking the time to try this out! I'm taking a look now

@0xcaff
Copy link
Owner

0xcaff commented Jul 22, 2024

ok regarding the metadata, I asked the duckdb-rs maintainers and they basically said build your own stuff for this or use our cmake duckdb/duckdb-rs#335

generally, I've found the duckdb-rs library lacking on a few fronts and ended up pulling it in tree and patching a couple things here master...threads I'm not thrilled about this workflow so I've left it on a branch for now.

I'm thinking the way I'd like to do this is by writing some sorta thing (probably in its own package after moving the repo into a workspace) which reads a config file and adds metadata. The cargo-psp ppl (example linked in the issue) seem to wrap cargo build with their own cargo command but this seems kinda fragile. https://github.com/overdrivenpotato/rust-psp/blob/master/cargo-psp/src/main.rs#L221-L232

I'll start by moving the repo into a workspace on master. Are you up for writing the metadata tool too? Either way I'd like to get this moved to 1.0 duckdb

@0xcaff 0xcaff mentioned this pull request Jul 22, 2024
@0xcaff
Copy link
Owner

0xcaff commented Jul 22, 2024

ok added the workspace, to fix conflicts apply the moves in https://github.com/0xcaff/duckdb_protobuf/pull/2/files

@0xcaff 0xcaff mentioned this pull request Jul 22, 2024
@Binlogo
Copy link
Contributor Author

Binlogo commented Jul 23, 2024

ok regarding the metadata, I asked the duckdb-rs maintainers and they basically said build your own stuff for this or use our cmake duckdb/duckdb-rs#335

generally, I've found the duckdb-rs library lacking on a few fronts and ended up pulling it in tree and patching a couple things here master...threads I'm not thrilled about this workflow so I've left it on a branch for now.

I'm thinking the way I'd like to do this is by writing some sorta thing (probably in its own package after moving the repo into a workspace) which reads a config file and adds metadata. The cargo-psp ppl (example linked in the issue) seem to wrap cargo build with their own cargo command but this seems kinda fragile. https://github.com/overdrivenpotato/rust-psp/blob/master/cargo-psp/src/main.rs#L221-L232

I'll start by moving the repo into a workspace on master. Are you up for writing the metadata tool too? Either way I'd like to get this moved to 1.0 duckdb

Thanks for your detailed explanation and quick updates. I'll close this one, try your new updates and may separate the tests to another PR.

@Binlogo Binlogo closed this Jul 23, 2024
@0xcaff
Copy link
Owner

0xcaff commented Jul 23, 2024

yay thanks so much for trying it out!! can't wait for the PR!

@Binlogo Binlogo mentioned this pull request Jul 23, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants