Feature: Enhance trino connector #1207
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce enhanced support for multiple schemas in Trino data source connections across the Wren UI project. The modifications span several files, updating the connection information handling to allow multiple schemas instead of a single schema. This involves changes to the data source adaptor, connection info interfaces, form components, and error text dictionary to support a more flexible schema configuration approach. Changes
Sequence DiagramsequenceDiagram
participant UI as Trino Properties UI
participant Repo as Project Repository
participant Adaptor as Ibis Adaptor
participant DataSource as Data Source Handler
UI->>Repo: Submit connection info with multiple schemas
Repo->>DataSource: Process connection info
DataSource->>DataSource: Convert to multiple connection infos
DataSource-->>Adaptor: Provide connection details
Adaptor->>Adaptor: Fetch tables for each connection
Adaptor-->>UI: Return consolidated table list
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx (1)
Line range hint
85-90: Fix inconsistency inrequiredvalidation forPasswordfieldThe
Passwordfield hasrequiredset totruein theForm.Item, butrequiredis set tofalsein the validationrules. This inconsistency may lead to incorrect form validation. If thePasswordfield is intended to be required, setrequired: truein the validation rules.Apply this diff to fix the validation rule:
<Form.Item label="Password" name="password" required rules={[ { - required: false, + required: true, message: ERROR_TEXTS.CONNECTION.PASSWORD.REQUIRED, }, ]} >
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (2)
276-286: Add type annotations togetTablesByConnectionInfofor type safetyAdding explicit type annotations to the
getTablesByConnectionInfofunction's parameters and return type improves type safety and code readability.Apply this diff to add type annotations:
- const getTablesByConnectionInfo = async (ibisConnectionInfo) => { + const getTablesByConnectionInfo = async ( + ibisConnectionInfo: IbisConnectionInfo, + ): Promise<CompactTable[]> => {
296-301: Consider handling individual connection failures when fetching tablesCurrently, if fetching tables from one of the multiple connections fails, the entire operation will fail due to the use of
Promise.all. Consider usingPromise.allSettledto handle individual failures, allowing the successful retrieval of tables from other connections while appropriately handling or logging the errors from failed connections.wren-ui/src/utils/error/dictionary.ts (1)
47-47: Improve error message wording forSCHEMAS.REQUIREDConsider rephrasing the error message for better clarity and grammar:
Apply this diff to update the error message:
SCHEMAS: { - REQUIRED: 'Please input list of catalog.schema separated by comma.', + REQUIRED: 'Please input a list of catalog.schema entries separated by commas.', },wren-ui/src/apollo/server/dataSource.ts (1)
181-198: Enhance error handling for schema parsing.The schema parsing logic could be more robust. Consider these improvements:
- Trim whitespace from schema parts
- Validate schema format before splitting
- const [catalog, schema] = schemas.split(',')?.[0]?.split('.') ?? []; + const firstSchema = schemas.split(',')[0]?.trim(); + if (!firstSchema) { + throw new Error('No valid schema provided'); + } + const [catalog, schema] = firstSchema.split('.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(2 hunks)wren-ui/src/apollo/server/dataSource.ts(3 hunks)wren-ui/src/apollo/server/repositories/projectRepository.ts(1 hunks)wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx(2 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
17-20: ImportingtoMultipleIbisConnectionInfosto handle multiple connectionsThe addition of
toMultipleIbisConnectionInfosenhances the ability to process multiple Ibis connection information objects, aligning with the new requirement to support multiplecatalog.schemaconfigurations.wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx (2)
54-64: UpdateSchemasfield to support multiplecatalog.schemaconfigurationsThe changes to the
Schemasfield appropriately reflect the support for multiplecatalog.schemaconfigurations, enhancing the flexibility of the Trino connector as per the PR objectives.
90-90: Use ofInput.Passwordfor password fieldChanging the password input to
Input.Passwordenhances security by masking user input, providing a better user experience.wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
56-56: Verify backward compatibility of the schema property rename.The change from
schematoschemasin theTRINO_CONNECTION_INFOinterface might break existing code that relies on the old property name.Let's check for any existing usage of the old property name:
✅ Verification successful
The schema property rename is safe to proceed with
The rename from
schematoschemasinTRINO_CONNECTION_INFOinterface is actually a bug fix that better reflects the expected format of multiple catalog.schema entries. No usage of the old property name was found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of the old schema property rg -l "schema.*:.*string.*TRINO_CONNECTION_INFO" || rg -l "TRINO_CONNECTION_INFO.*schema.*:.*string" # Search for destructuring of the old schema property from TRINO_CONNECTION_INFO rg "const.*{.*schema.*}.*as.*TRINO_CONNECTION_INFO"Length of output: 292
Script:
#!/bin/bash # Find the interface definition and any related types rg -l "interface.*TRINO_CONNECTION_INFO" # Search for any usage of .schema property rg "\.schema\b" --type typescript # Search for schema in type definitions rg "schema.*:.*string" --type typescript # Search for any destructuring patterns with schema rg "{[^}]*schema[^}]*}" --type typescriptLength of output: 389
Script:
#!/bin/bash # View the content of the file containing the interface cat wren-ui/src/apollo/server/repositories/projectRepository.ts # Search for any schema-related patterns rg "\.schema\b" # Search for schema in type definitions with fixed regex rg "schema: string" # Search for TRINO_CONNECTION_INFO usage rg "TRINO_CONNECTION_INFO"Length of output: 15160
wren-ui/src/apollo/server/dataSource.ts (3)
50-57: LGTM! Well-structured null-safe implementation.The new
toMultipleIbisConnectionInfosfunction safely handles data sources that don't support multiple connection infos by returning null.
62-62: LGTM! Good use of optional method.Making
toMultipleIbisConnectionInfosoptional in the interface allows for gradual adoption by different data sources.
192-192: LGTM! Proper SSL handling.The SSL implementation correctly prefixes the host with
https://when SSL is enabled.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
296-301: Handle Partial Failures When Fetching TablesCurrently, if one of the connections fails in
Promise.all, the entire operation will fail. Consider handling partial failures so that tables from successful connections are returned, and errors are logged or communicated appropriately.You might modify the implementation to use
Promise.allSettledand process the results accordingly:- const results = await Promise.all( - multipleIbisConnectionInfos.map(getTablesByConnectionInfo), - ); + const results = await Promise.allSettled( + multipleIbisConnectionInfos.map(getTablesByConnectionInfo), + ); + const tables = results + .filter(result => result.status === 'fulfilled') + .flatMap(result => result.value); + if (tables.length === 0) { + throw new Error('Failed to retrieve tables from all connections'); + } + return tables;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(2 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(2 hunks)wren-ui/src/apollo/server/dataSource.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
276-287: Correct Implementation of Multiple Connection InfosThe addition of the
getTablesByConnectionInfofunction and handling multiple Ibis connection infos enhances thegetTablesmethod to support multiple schemas effectively. The asynchronous implementation usingasync/awaitis well-structured.wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (1)
77-77: Update Mock Trino Connection Info StructureThe change from
catalogtoschemasinmockTrinoConnectionInfocorrectly reflects the new structure supporting multiple schemas. This aligns the test data with the updated function expectations.wren-ui/src/apollo/server/dataSource.ts (3)
50-57: Well-DesignedtoMultipleIbisConnectionInfosFunctionThe introduction of the
toMultipleIbisConnectionInfosfunction is thoughtfully implemented, enhancing the ability to handle multiple schemas within the data source configuration.
181-199: Consider Reusing Schema Parsing Logic to Avoid DuplicationIn the
toIbisConnectionInfomethod for Trino, you're performing schema parsing similar totoMultipleIbisConnectionInfos. To maintain DRY (Don't Repeat Yourself) principles, consider reusing theparseSchemahelper function to avoid code duplication and ensure consistent validation.Since a similar suggestion was made in a previous review, applying this change will improve maintainability and consistency in your codebase.
200-243: Effective Handling of Multiple Schemas for TrinoThe
toMultipleIbisConnectionInfosmethod effectively parses and validates multiple schemas, and constructs the corresponding connection information objects. This implementation enhances the flexibility and scalability of the Trino connector.
Fix SSL issue
ref: #1106
Support using multiple catalog.schema
Users could now specify more than one catalog.schema
ref: #546
Summary by CodeRabbit
New Features
Bug Fixes
Documentation