feat(ibis): query cache with connection url #1162
Conversation
WalkthroughThis change updates the internal cache key generation logic to support both traditional connection info objects and the newer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant QueryCacheManager
participant Database
Client->>Router: POST query with connection URL (cacheEnable=true)
Router->>QueryCacheManager: Generate cache key (detects ConnectionUrl)
QueryCacheManager->>QueryCacheManager: Check cache for key
alt Cache miss
QueryCacheManager->>Database: Execute query
Database-->>QueryCacheManager: Query result
QueryCacheManager->>QueryCacheManager: Store result in cache
QueryCacheManager-->>Router: Return result (X-Cache-Hit: false)
else Cache hit
QueryCacheManager-->>Router: Return cached result (X-Cache-Hit: true)
end
Router-->>Client: Respond with results and cache headers
sequenceDiagram
participant Client
participant Router
participant QueryCacheManager
participant Database
Client->>Router: POST query with connection URL (cacheEnable=true, overrideCache=true)
Router->>QueryCacheManager: Generate cache key (detects ConnectionUrl)
QueryCacheManager->>Database: Execute query (force bypass cache)
Database-->>QueryCacheManager: Query result
QueryCacheManager->>QueryCacheManager: Update cache with new result
QueryCacheManager-->>Router: Return result (X-Cache-Override: true)
Router-->>Client: Respond with results and override headers
Possibly related PRs
Suggested reviewers
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:
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 (
|
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Could you add the case for the fallback test? Others make sense to me.
![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 (2)
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (2)
111-121: Basic connection URL query test looks good, but should validate result dataThe test successfully verifies that queries can be executed using the connection URL format. However, unlike the parallel
test_queryfunction, this test doesn't validate the structure and content of the returned data.Consider extending this test to validate the response data similar to the original test:
async def test_query_with_connection_url(client, manifest_str, connection_url): response = await client.post( url=f"{base_url}/query", json={ "connectionInfo": {"connectionUrl": connection_url}, "manifestStr": manifest_str, "sql": "SELECT orderkey FROM orders LIMIT 1", }, ) assert response.status_code == 200 + result = response.json() + assert len(result["columns"]) == 1 + assert len(result["data"]) == 1
123-155: Cache hit/miss test with connection URL looks goodThis test correctly validates the cache functionality with connection URLs, properly checking for cache misses on first requests, hits on subsequent requests, and verifying result consistency.
However, one minor inconsistency compared to the original
test_query_with_cachefunction:Consider adding the timestamp validation that exists in the original test:
assert response2.status_code == 200 assert response2.headers["X-Cache-Hit"] == "true" + assert int(response2.headers["X-Cache-Create-At"]) > 1743984000 # 2025.04.07 result2 = response2.json()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/app/query_cache/__init__.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ibis-server/app/query_cache/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (4)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (4)
test_query_with_connection_url(202-216)manifest_str(96-97)test_query_with_connection_url_and_cache_enable(219-249)test_query_with_connection_url_and_cache_override(252-279)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/postgres/test_model_substitute.py (1)
manifest_str(46-47)ibis-server/tests/routers/v3/connector/postgres/conftest.py (1)
connection_url(48-50)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (2)
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (2)
157-185: Cache override test with connection URL is well implementedThe test correctly validates that cache override functionality works with connection URLs. It properly verifies the override flag is set and that timestamps are logically consistent.
111-185:Details
❓ Verification inconclusive
Verify connection URL and connection info caching are equivalent
The tests you've added ensure that connection URL-based queries work with the caching system, but it's worth verifying that both connection methods produce the same cache key for equivalent connections.
🏁 Script executed:
#!/bin/bash # Find where cache keys are generated from connection info echo "Searching for cache key generation code..." rg -A 10 "def get_cache_key" --type py # Look for connection URL handling echo "Searching for connection URL handling in cache code..." rg -A 5 "connectionUrl" --type pyLength of output: 13901
Ensure cache key parity for
connectionUrland structuredconnectionInfo
We still need to confirm that both ways of specifying a connection produce the exact same cache key. Since noget_cache_keydefinition surfaced in the initial search, please:
- Locate the function responsible for computing cache keys (e.g. in your cache or router modules—often under
ibis-server/app/cacheor alongside your query handlers).- Add a small unit test that:
- Constructs a
PostgresConnectionInfomodel (with host, port, user, password, database).- Builds the equivalent
connectionUrlstring.- Invokes the cache‐key function on both inputs and asserts the returned keys are identical.
This will guarantee that caching is consistent regardless of how the client supplies connection details.
2 participants
We can support query cache from connection info using connection URL.
Summary by CodeRabbit
Bug Fixes
Tests