Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Jul 8, 2025

Add Firebolt Core connection support with dependency update

Summary

This PR adds support for Firebolt Core connections to the SQLAlchemy dialect. Core connections are identified by the presence of a "url" parameter in the connection string and use the FireboltCore authentication class instead of traditional username/password credentials.

Key Changes:

  • Connection Detection: Modified create_connect_args() to detect Core connections via "url" parameter
  • Authentication: Added FireboltCore auth support with credential validation (rejects username/password)
  • Code Structure: Refactored create_connect_args() into helper methods to reduce cognitive complexity
  • Dependency Fix: Updated firebolt-sdk dependency from non-existent core-support branch to main branch
  • Test Coverage: Added comprehensive unit tests and integration test framework for Core connections
  • Async Support: Fixed async dialect runtime error handling

Review & Testing Checklist for Human

This is a medium-risk change involving authentication and URL parsing modifications. Please verify:

  • Test both connection types: Verify regular Firebolt connections still work and Core connections work with url parameter
  • Credential validation: Confirm Core connections properly reject username/password credentials with clear error messages
  • URL parsing edge cases: Test invalid Core URLs and ensure proper error handling
  • Dependency stability: Verify the main branch SDK dependency doesn't introduce regressions in existing functionality
  • Integration testing: Run integration tests against both staging environment and local Core instance if available

Recommended Test Plan:

  1. Test regular connection: firebolt://user:pass@db/engine?account_name=test
  2. Test Core connection: firebolt://db?url=http://localhost:3473
  3. Test error cases: Core with credentials, invalid URLs, missing parameters

Diagram

%%{ init : { "theme" : "default" }}%%
graph TB
    setup["setup.cfg<br/>Dependency Update"]:::major-edit
    dialect["src/firebolt_db/<br/>firebolt_dialect.py<br/>Core Connection Logic"]:::major-edit
    async_dialect["src/firebolt_db/<br/>firebolt_async_dialect.py<br/>Runtime Error Fix"]:::minor-edit
    unit_tests["tests/unit/<br/>test_firebolt_dialect.py<br/>Core Unit Tests"]:::major-edit
    integration_tests["tests/integration/<br/>test_core_integration.py<br/>Core Integration Tests"]:::major-edit
    
    setup --> dialect
    dialect --> async_dialect
    dialect --> unit_tests
    dialect --> integration_tests
    
    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit
        L3[Context/No Edit]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF
Loading

Notes

  • Core integration tests requiring local Firebolt Core instance are currently excluded from CI due to environment limitations
  • The dependency update from core-support to main branch resolved CI failures and is confirmed working
  • Authentication changes are isolated to Core connections and shouldn't affect existing regular connections
  • All existing integration tests (10/10) pass with staging environment

Session Info:

- Add FireboltCore import from firebolt.client.auth
- Modify create_connect_args to detect 'url' parameter for Core connections
- Update _determine_auth to return FireboltCore auth when is_core=True
- Skip client_id/secret and account_name validation for Core connections
- Pass Core URL to SDK via kwargs['url']
- Add comprehensive unit tests for Core connection behavior
- Maintain backward compatibility with existing connection patterns

Co-Authored-By: [email protected] <[email protected]>
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner July 8, 2025 14:47
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@devin-ai-integration devin-ai-integration bot changed the title Add Firebolt Core connection support feat: Add Firebolt Core connection support Jul 8, 2025
devin-ai-integration bot and others added 4 commits July 8, 2025 14:59
- Clarify that for Core connections, full URL (scheme, host, port) comes from url parameter
- Address GitHub comment feedback about misleading connection string format

Co-Authored-By: [email protected] <[email protected]>
- Fix line length violation in firebolt_dialect.py (90 > 88 characters)
- Apply isort and black formatting fixes from pre-commit hooks
- Ensure code passes flake8 line length requirements

Co-Authored-By: [email protected] <[email protected]>
- Break long documentation comment into two lines to meet 88-character limit
- All pre-commit checks now pass locally
- Unit tests continue to pass (40/40)

Co-Authored-By: [email protected] <[email protected]>
devin-ai-integration bot and others added 3 commits July 8, 2025 15:27
- Refactor _determine_auth to accept URL object directly
- Add validation for Core connections to reject engine_name, account_name, and credentials
- Update existing Core connection tests to use valid format
- Add comprehensive tests for Core connection validation errors
- Addresses GitHub PR comments for stricter Core connection handling

Co-Authored-By: [email protected] <[email protected]>
- Create GitHub workflow for Core integration tests based on firebolt-python-sdk
- Add simplified docker-compose.yml without nginx/SSL setup
- Add Core configuration file for docker container
- Create Core integration test fixtures and test class
- Test Core connection, authentication, and simple query execution
- Verify Core connections don't require traditional credentials

Co-Authored-By: [email protected] <[email protected]>
- Extract Core validation logic into _validate_core_connection method
- Extract token cache parsing into _parse_token_cache_flag method
- Extract connection kwargs building into _build_connection_kwargs method
- Extract account name handling into _handle_account_name method
- Extract environment config into _handle_environment_config method
- Extract additional parameters building into _build_additional_parameters method
- Add proper type annotations to satisfy mypy
- Maintain exact same functionality while improving code organization
- All unit tests pass (43/43) and pre-commit checks pass

Co-Authored-By: [email protected] <[email protected]>
devin-ai-integration bot and others added 11 commits July 9, 2025 12:39
- Explain that url.database maps to engine_name in Firebolt context
- Explain that url.host maps to database name in Firebolt context
- Address reviewer concerns about validation consistency

Co-Authored-By: [email protected] <[email protected]>
- Remove engine_name and account_name validation checks from _validate_core_connection
- Keep only credentials validation since FireboltCore auth handles other parameters
- Update unit tests to reflect new validation behavior
- Address GitHub comment from ptiurin requesting simplified validation

Co-Authored-By: [email protected] <[email protected]>
- Add tests to verify SDK handles engine_name parameter validation
- Add tests to verify SDK handles account_name parameter validation
- Add tests to verify SDK handles invalid URL parameter validation
- Demonstrates that SQLAlchemy only validates credentials for Core connections
- Other parameters are passed through to SDK for proper validation

Addresses GitHub comment requesting integration tests for Core validation

Co-Authored-By: [email protected] <[email protected]>
- Handle 'Attempted to call run() from inside a run()' error
- Use await_only when already in async context
- Note: Async tests still fail due to trio/asyncio incompatibility

Co-Authored-By: [email protected] <[email protected]>
…upport' into devin/1751985773-firebolt-core-support
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 9, 2025

Copy link
Collaborator

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

Tests are passing. Confirmed Superset is working with Core as well.

@ptiurin ptiurin merged commit f708f51 into main Jul 10, 2025
7 checks passed
@ptiurin ptiurin deleted the devin/1751985773-firebolt-core-support branch July 10, 2025 08:03
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.

3 participants