feat(ibis): add statement_timeout for Clickhouse#1266
Conversation
WalkthroughThe changes add support for statement-level timeout settings for ClickHouse connections by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant ClickHouse
Client->>API_Server: POST /query (with x-wren-db-statement_timeout header)
API_Server->>API_Server: Parse header, set settings['max_execution_time']
API_Server->>ClickHouse: Connect with settings
API_Server->>ClickHouse: Execute query
alt Query exceeds timeout
ClickHouse-->>API_Server: QueryCancelled/DatabaseError
API_Server-->>Client: HTTP 504 with cancellation message
else Query completes
ClickHouse-->>API_Server: Query result
API_Server-->>Client: Query result
end
Estimated code review effort2 (~15 minutes) Possibly related issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
github-actions
bot
added
the
dependencies
goldmedal
force-pushed
the
feat/ch-statement-timeout
branch
from
97868f7 to
8078e6d
Compare
goldmedal
force-pushed
the
feat/ch-statement-timeout
branch
from
75fe9df to
8490ec6
Compare
goldmedal
force-pushed
the
feat/ch-statement-timeout
branch
from
bad1ba5 to
c29d922
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ibis-server/app/model/data_source.py (1)
185-186: Consider using a more robust boolean conversion.The current implementation handles common truthy values, but consider using Python's built-in
distutils.util.strtoboolor a more comprehensive approach for better reliability.- def _safe_strtobool(self, val: str) -> bool: - return val.lower() in {"1", "true", "yes", "y"} + def _safe_strtobool(self, val: str) -> bool: + from distutils.util import strtobool + try: + return bool(strtobool(val)) + except ValueError: + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ibis-server/resources/function_list/clickhouse.csvis excluded by!**/*.csv
📒 Files selected for processing (8)
ibis-server/app/dependencies.py(1 hunks)ibis-server/app/model/__init__.py(7 hunks)ibis-server/app/model/data_source.py(6 hunks)ibis-server/app/model/metadata/dto.py(1 hunks)ibis-server/app/routers/v3/connector.py(4 hunks)ibis-server/app/util.py(2 hunks)ibis-server/tests/routers/v2/connector/test_clickhouse.py(2 hunks)ibis-server/tests/routers/v3/connector/clickhouse/test_functions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ibis-server/app/util.py
- ibis-server/app/dependencies.py
- ibis-server/app/model/metadata/dto.py
- ibis-server/tests/routers/v2/connector/test_clickhouse.py
- ibis-server/app/model/init.py
⏰ 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). (1)
- GitHub Check: ci
🔇 Additional comments (11)
ibis-server/tests/routers/v3/connector/clickhouse/test_functions.py (1)
56-56: Verify that the function count change aligns with PR objectives.The test expects one additional function (
DATAFUSION_FUNCTION_COUNT + 6instead of+ 5), but the PR is focused on adding statement timeout functionality for ClickHouse. Please confirm that this change is intentional and related to the timeout feature implementation.#!/bin/bash # Description: Check if any new functions were added to the ClickHouse function list # Expected: Find evidence of new timeout-related or other functions # Search for function definitions or imports that might explain the count increase rg -A 5 -B 5 "function.*timeout|timeout.*function" # Check for recent changes in function list files fd "function_list" --type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 "{}"'Likely an incorrect or invalid review comment.
ibis-server/app/routers/v3/connector.py (4)
25-25: Good addition of timeout-specific exception handling.The import of
DatabaseTimeoutErrorproperly supports the new timeout functionality for ClickHouse connections.
177-179: Appropriate timeout exception handling prevents inappropriate fallback.The specific handling of
DatabaseTimeoutErrorcorrectly prevents fallback to v2 API when a timeout occurs. This ensures that timeout errors are properly propagated to the client rather than being masked by fallback logic.
337-339: Consistent timeout handling across validation endpoint.The timeout exception handling here mirrors the query endpoint implementation, maintaining consistency across the API.
420-422: Complete timeout handling coverage for model substitution.The timeout exception handling is consistently applied to the model substitution endpoint as well, ensuring comprehensive timeout behavior across all affected endpoints.
ibis-server/app/model/data_source.py (6)
5-9: Good addition of required imports for URL parsing and type annotations.The new imports support URL parsing functionality and improved type safety with more specific type annotations.
85-87: Improved type annotations enhance code clarity.The refined type annotations (
dict[str, Any]for data anddict[str, str]for headers) provide better type safety and code documentation.
106-111: Well-implemented ClickHouse timeout configuration.The timeout handling correctly:
- Uses the header value with a sensible default of 180 seconds
- Only sets max_execution_time if not already configured
- Initializes settings dict if needed
- Converts the timeout to integer as required by ClickHouse
116-124: Robust connection URL detection and handling.The logic properly detects connection URL formats and delegates ClickHouse URL parsing to a specialized method while falling back to generic ConnectionUrl for other data sources.
162-183: Comprehensive ClickHouse URL parsing implementation.The URL parsing method handles all necessary components:
- Schema validation
- User/password extraction with proper URL decoding
- Host/port/database extraction
- Query parameter parsing with special handling for
secureboolean- Proper error handling for invalid URLs
270-270: Proper settings propagation to ClickHouse connection.The explicit passing of settings ensures that timeout configurations are properly applied to the ClickHouse connection.
|
Thanks @goldmedal |
2 participants
part of #1267
Description
Set up
max_execution_timefor clickhouse connection by default.Other changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests