fix: Trino SQL parameterization and add integration tests #22
Conversation
📝 WalkthroughWalkthroughAdds auto-detection of SQL Sequence Diagram(s)sequenceDiagram
participant Caller
participant execute_sql
participant render_jinja_sql_template
participant ParamStyleAutoDetect
participant prepare_params
participant pandas.read_sql_query
Caller->>execute_sql: call execute_sql(sql, conn_url, bind_params?, param_style?)
execute_sql->>render_jinja_sql_template: render templates (if any)
render_jinja_sql_template-->>execute_sql: rendered_sql, rendered_bind_params
execute_sql->>ParamStyleAutoDetect: was param_style provided?
alt param_style is None
ParamStyleAutoDetect->>ParamStyleAutoDetect: extract driver from URL
ParamStyleAutoDetect-->>execute_sql: detected param_style (e.g., trino -> "qmark")
else
ParamStyleAutoDetect-->>execute_sql: use explicit param_style
end
execute_sql->>prepare_params: evaluate bind_params vs param_style
alt param_style in ["qmark","format"] and bind_params is list
prepare_params->>prepare_params: convert list -> tuple
prepare_params-->>execute_sql: prepared_params (tuple)
else
prepare_params-->>execute_sql: prepared_params (unchanged)
end
execute_sql->>pandas.read_sql_query: call with rendered_sql and prepared_params
pandas.read_sql_query-->>execute_sql: DataFrame
execute_sql-->>Caller: return DataFrame
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/test_sql_execution.py (1)
🪛 Ruff (0.14.3)tests/unit/test_sql_execution.py311-311: Use a regular Replace (PT009) 340-340: Use a regular Replace (PT009) 371-371: Use a regular Replace (PT009) 401-401: Use a regular Replace (PT009) 432-432: Use a regular Replace (PT009) 437-437: Use a regular Replace (PT009) 438-438: Use a regular Replace (PT009) 465-465: Use a regular Replace (PT009) 472-472: Use a regular Replace (PT009) 473-473: Use a regular Replace (PT009) 500-500: Use a regular Replace (PT009) 507-507: Use a regular Replace (PT009) 508-508: Use a regular Replace (PT009) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
Comment |
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
- Coverage 76.60% 75.36% -1.25%
==========================================
Files 99 99
Lines 5476 5606 +130
Branches 748 783 +35
==========================================
+ Hits 4195 4225 +30
- Misses 1281 1381 +100 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignore(1 hunks)deepnote_toolkit/sql/jinjasql_utils.py(1 hunks)deepnote_toolkit/sql/sql_execution.py(2 hunks)pyproject.toml(1 hunks)tests/integration/test_trino.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/test_trino.py (1)
deepnote_toolkit/sql/sql_execution.py (1)
execute_sql(197-233)
🪛 Ruff (0.14.3)
tests/integration/test_trino.py
156-156: Missing return type annotation for private function mock_get_variable_value
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.10
- GitHub Check: Typecheck - 3.13
- GitHub Check: Test - Python 3.12
- GitHub Check: Typecheck - 3.9
- GitHub Check: Typecheck - 3.12
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.9
🔇 Additional comments (12)
deepnote_toolkit/sql/jinjasql_utils.py (1)
17-17: Good documentation improvements.The added examples and clarifications about parameter styles help developers understand the default behavior and database-specific requirements.
Also applies to: 25-26
pyproject.toml (1)
179-180: LGTM.The python-dotenv dev dependency supports environment-based credential loading in integration tests.
deepnote_toolkit/sql/sql_execution.py (2)
154-161: Auto-detection logic looks correct.The mapping approach is extensible. Currently only Trino is mapped to
qmark, which aligns with the PR objectives.
438-446: Correct handling of parameter types for pandas.The tuple conversion ensures compatibility with qmark/format parameter styles while preserving dict params for pyformat style.
tests/integration/test_trino.py (8)
18-24: Clean context manager design.Ensures environment variable cleanup via try/finally block.
27-56: Well-designed credential management.Loads from .env, skips gracefully if unavailable, and provides sensible defaults. Credentials stay in gitignored .env file.
59-81: Proper connection lifecycle management.Module-scoped fixture with optional authentication and guaranteed cleanup via try/finally.
110-135: Well-constructed toolkit connection fixture.Properly URL-encodes credentials and explicitly sets
qmarkparam_style for Trino.
84-107: Effective baseline connectivity tests.Simple smoke tests validate connection and catalog access before proceeding to toolkit integration tests.
141-150: Solid baseline toolkit test.Validates execute_sql with a simple query, checking DataFrame structure and content.
152-177: Good coverage of Jinja templating integration.Mock-based test validates variable substitution for both string and numeric types.
179-227: Critical test for the auto-detection fix.Validates the regression fix (BLU-5135) by omitting param_style and verifying auto-detection works correctly. This test directly validates the core PR functionality.
|
🚀 Review App Deployment Started
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.gitignore(1 hunks)tests/unit/test_sql_execution.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_sql_execution.py (1)
deepnote_toolkit/sql/sql_execution.py (1)
execute_sql_with_connection_json(73-194)
🪛 Ruff (0.14.3)
tests/unit/test_sql_execution.py
313-313: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
344-344: Use a regular assert instead of unittest-style assertIsNone
Replace assertIsNone(...) with assert ...
(PT009)
377-377: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
409-409: Use a regular assert instead of unittest-style assertIsNone
Replace assertIsNone(...) with assert ...
(PT009)
432-432: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
440-440: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
445-445: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
446-446: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.10
🔇 Additional comments (2)
.gitignore (1)
135-136: Good addition for environment variant handling.Adding
.env.*alongside the existing.enventry appropriately ignores environment-specific config files (.env.local,.env.test, etc.) while keeping the base pattern intact. This aligns well with the PR's addition of python-dotenv and integration tests.tests/unit/test_sql_execution.py (1)
281-447: Comprehensive test coverage for Trino param_style auto-detection.The test suite thoroughly validates the auto-detection feature across multiple scenarios: default Trino behavior, non-Trino databases, explicit overrides, edge cases, and Jinja template integration. Well-structured and properly mocked.
|
Approved 👍 |
Summary by CodeRabbit
New Features
Documentation
Tests
Chores