- Notifications
You must be signed in to change notification settings - Fork207
feat: Integrate Trino as a data source using sfu-db/connector-x#405
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
vcknorket wants to merge10 commits intoroapi:mainChoose a base branch
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This commit introduces support for Trino as a data source by updatingthe connector-x dependency and integrating its Trino capabilities.Key changes include:1. **Dependency Update:** * Switched `connectorx` in `columnq/Cargo.toml` from the forked `roapi/connector-x` to the official `sfu-db/connector-x` (version `0.4.4-alpha.1`). * Updated `connectorx` features to `["fptr", "src_trino", "dst_arrow"]`.2. **Core Logic for Trino:** * Added a `Trino` variant to the `DatabaseLoader` enum in `columnq/src/table/database.rs`. * Updated conditional compilation flags in `columnq/src/table/database.rs` to include `database-trino`. * In `columnq/src/table/mod.rs`: * Added `Trino` to the `Extension` enum and updated its `From` and `TryFrom` implementations. * Added a `trino { table: Option<String> }` variant to the `TableLoadOption` enum. * Updated `TableLoadOption::extension()` method. * Updated `TableSource::parse_option()` to recognize "trino" URIs. * Updated the main `load()` function to handle Trino. * Modified table name extraction in `columnq/src/table/database.rs` to support `TableLoadOption::trino`.3. **Cargo Feature Flag:** * Added `database-trino = ["connectorx/src_trino"]` to the `[features]` section in `columnq/Cargo.toml`. * Included `database-trino` in the `database` feature group.4. **Testing:** * Added a new test file `columnq/tests/table_trino_test.rs`. * This includes a basic compile-time and structural test for the Trino integration, conditional on the `database-trino` feature. * A comment in the test notes that full integration testing requires a live Trino instance and appropriate environment setup.This integration allows you to specify Trino as a data source viaURI or configuration, leveraging the capabilities of the underlying`sfu-db/connector-x` library.columnq/tests/table_trino_test.rs Outdated
| // but avoid pulling in too much if it complicates things without a live server. | ||
| #[tokio::test] | ||
| async fn test_trino_loader_construction() { |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this test doesn't seem to do anything? I am okay with merging without a test if it's too much work to setup trio in the CI.
Member
houqp commentedJun 15, 2025
thanks for the contribution, but looks like the build is broken. |
Author
vcknorket left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
removed test
rusqlite version
rusqlite version update
Update Cargo.toml
Update get_arrow call to match connectorx API"
Enable Arrow 54 use 55
Revert the changes made
Update arrow version to be specific
Author
vcknorket commentedJun 24, 2025
Member
houqp commentedJun 27, 2025
@vcknorket you will need to send an upstream PR to connector to bump their version of arrow to 55 first. |
Author
vcknorket commentedJun 27, 2025 via email
Seems someone already asked the upstream but they said...it cannot be done but suggest that it should still work …________________________________From: QP Hou ***@***.***>Sent: June 27, 2025 2:47 AMTo: roapi/roapi ***@***.***>Cc: Venkata Chandra (VC) ***@***.***>; Mention ***@***.***>Subject: Re: [roapi/roapi] feat: Integrate Trino as a data source using sfu-db/connector-x (PR#405)[https://avatars.githubusercontent.com/u/670302?s=20&v=4]houqp left a comment (roapi/roapi#405)<#405 (comment)>@vcknorket<https://github.com/vcknorket> you will need to send an upstream PR to connector to bump their version of arrow to 55 first.—Reply to this email directly, view it on GitHub<#405 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AWCT64STYLLUBJHQDMTLJML3FTSG3AVCNFSM6AAAAAB7JE2GWWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAMJRHA4TGNBZG4>.You are receiving this because you were mentioned.Message ID: ***@***.***> |
Member
houqp commentedJun 29, 2025
@vcknorket you should be able to upgrade, I have don't that a few times in the past. Could you link me to the discussion? |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit introduces support for Trino as a data source by updating the connector-x dependency and integrating its Trino capabilities.
Key changes include:
Dependency Update:
connectorxincolumnq/Cargo.tomlfrom the forkedroapi/connector-xto the officialsfu-db/connector-x(version
0.4.4-alpha.1).connectorxfeatures to["fptr", "src_trino", "dst_arrow"].Core Logic for Trino:
Trinovariant to theDatabaseLoaderenum incolumnq/src/table/database.rs.columnq/src/table/database.rsto include
database-trino.columnq/src/table/mod.rs:Trinoto theExtensionenum and updated itsFromand
TryFromimplementations.trino { table: Option<String> }variant to theTableLoadOptionenum.TableLoadOption::extension()method.TableSource::parse_option()to recognize "trino" URIs.load()function to handle Trino.columnq/src/table/database.rsto support
TableLoadOption::trino.Cargo Feature Flag:
database-trino = ["connectorx/src_trino"]to the[features]section incolumnq/Cargo.toml.database-trinoin thedatabasefeature group.Testing:
columnq/tests/table_trino_test.rs.Trino integration, conditional on the
database-trinofeature.a live Trino instance and appropriate environment setup.
This integration allows you to specify Trino as a data source via URI or configuration, leveraging the capabilities of the underlying
sfu-db/connector-xlibrary.