feat(ibis): override query cache #1138
Conversation
WalkthroughThe changes enhance the caching functionality for query processing. A new method is introduced to extract timestamps from cache files, and the cache file naming logic is updated to handle missing files by incorporating a Unix timestamp. Additionally, a new parameter is added to both v2 and v3 query endpoints to allow overriding the cache. This new flag alters the control flow to include or bypass cached data and updates response headers accordingly. Tests have been expanded to validate both the cache override and the proper assignment of header values. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as QueryRouter (v2/v3)
participant CacheManager
Client->>Router: Sends query request (with override_cache flag)
Router->>CacheManager: Retrieve cache file info (using get_cache_file_timestamp/_get_cache_file_name)
alt Cache exists and override_cache is False
CacheManager-->>Router: Returns cache file timestamp/filename
Router->>Client: Return cached response with header X-Cache-Create-At
else override_cache is True
Router->>CacheManager: (Proceed with override logic)
Router->>Client: Return fresh response with headers X-Cache-Override & X-Cache-Override-At
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (1)
ibis-server/app/query_cache/__init__.py (1)
80-93: Consider consolidating duplicate file listing logic.There's duplication between
get_cache_file_timestampand_get_cache_file_name- both iterate through all files looking for those that start with the cache key. This could be extracted into a helper method for better maintainability.+ def _find_cache_file(self, cache_key: str): + """Find a cache file that starts with the given cache key.""" + op = self._get_dal_operator() + for file in op.list("/"): + if file.path.startswith(cache_key): + return file.path + return None def get_cache_file_timestamp(self, data_source: str, sql: str, info) -> int: cache_key = self._generate_cache_key(data_source, sql, info) - op = self._get_dal_operator() - for file in op.list("/"): - if file.path.startswith(cache_key): + file_path = self._find_cache_file(cache_key) + if file_path: # xxxxxxxxxxxxxx-1744016574.cache # we only care about the timestamp part - timestamp = int(file.path.split("-")[-1].split(".")[0]) + timestamp = int(file_path.split("-")[-1].split(".")[0]) return timestamp return None def _get_cache_file_name(self, cache_key: str) -> str | None: - op = self._get_dal_operator() - for file in op.list("/"): - if file.path.startswith(cache_key): - return file.path + existing_file = self._find_cache_file(cache_key) + if existing_file: + return existing_file # If no cache file found, create a new one # ...rest of method unchanged
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ibis-server/app/query_cache/__init__.py(3 hunks)ibis-server/app/routers/v2/connector.py(2 hunks)ibis-server/app/routers/v3/connector.py(3 hunks)ibis-server/tests/routers/v2/connector/test_postgres.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/test_query.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
ibis-server/app/routers/v2/connector.py (4)
ibis-server/app/util.py (1)
to_json(23-27)ibis-server/app/query_cache/__init__.py (1)
get_cache_file_timestamp(56-66)ibis-server/app/routers/v3/connector.py (1)
query(36-111)ibis-server/app/model/connector.py (5)
query(56-57)query(72-73)query(112-115)query(143-181)query(197-203)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (15)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (1)
166-166: Good addition of cache timestamp verification.This assertion validates that the cache creation timestamp header is present and contains a value greater than April 7, 2025, aligning with the new cache timestamp functionality.
ibis-server/tests/routers/v2/connector/test_postgres.py (2)
195-195: Good addition of cache timestamp verification.This assertion validates that the cache creation timestamp header is present and contains a value greater than April 7, 2025, aligning with the cache timestamp functionality.
204-231: Well-structured test for the cache override functionality.This test case properly validates the cache override feature by:
- First making a request with caching enabled to populate the cache
- Then making a second request with both cache enabled and override enabled
- Verifying the appropriate headers are present in the response
The test ensures that the X-Cache-Override header is properly set to "true" and that the X-Cache-Override-At timestamp is valid.
ibis-server/app/routers/v2/connector.py (6)
46-46: Good addition of the override_cache parameter.The override_cache parameter with default value of False maintains backward compatibility while adding the new functionality to bypass the cache when needed.
88-89: Simplified cache retrieval call.This change removes unnecessary string conversion of data_source, making the code cleaner.
92-92: Enhanced cache condition logic.The condition now correctly checks both cache_hit and override_cache, ensuring the cache is used only when appropriate.
96-100: Good addition of cache creation timestamp header.The X-Cache-Create-At header provides valuable information about when the cache was created, enhancing client-side decision making.
104-112: Appropriate response handling for non-cached results.The code now properly structures the response for non-cached results and adds the cache creation timestamp header if there was a cache hit, ensuring consistent header presence in both code paths.
119-125: Well-implemented cache override headers.The addition of X-cache-override and X-cache-override-at headers provides clear indication to clients when the cache has been bypassed and when the new cache was created.
ibis-server/app/routers/v3/connector.py (4)
41-41: Good addition of the override_cache parameter.The override_cache parameter with default value of False maintains backward compatibility while adding the new functionality to bypass the cache when needed.
72-74: Simplified cache retrieval call.This change removes unnecessary string conversion of data_source, making the code cleaner.
81-85: Good addition of cache creation timestamp header.The X-Cache-Create-At header provides valuable information about when the cache was created, enhancing client-side decision making.
106-106: Proper parameter forwarding to v2 connector.Correctly passes the override_cache parameter to the v2 connector's query method when falling back, ensuring consistent behavior across API versions.
ibis-server/app/query_cache/__init__.py (2)
2-2: Added time module import to support timestamp functionality.The import is necessary for the new timestamp features added to the cache file naming and retrieval.
91-92: LGTM: The timestamp generation aligns with PR objectives.The implementation correctly adds a Unix timestamp to the cache file name, which supports the PR objective of tracking when the cache was generated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ibis-server/tests/routers/v2/connector/test_postgres.py (1)
204-233: Good coverage for the cache override scenario.The logic correctly ensures the second request overrides the existing cache and updates related headers. As a minor refinement, you might also confirm that the second response’s data matches the newly fetched result rather than the cached data, to fully validate the override.
ibis-server/app/routers/v3/connector.py (1)
41-41: Clarify documentation for theoverride_cacheparameter.Consider explicitly documenting in the function docstring or type hint usage the behavior of
override_cache—that it ignores (and then refreshes) any existing cache to fetch fresh data from the source.ibis-server/app/query_cache/__init__.py (1)
93-103: Deleting old cache files before creating new ones.Removing previous cache files helps keep the cache directory clean and ensures only the latest file remains. However, if concurrency is possible, multiple requests could inadvertently remove each other’s cache. A more robust eviction strategy might be needed based on usage patterns and concurrency requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ibis-server/app/query_cache/__init__.py(4 hunks)ibis-server/app/routers/v2/connector.py(2 hunks)ibis-server/app/routers/v3/connector.py(3 hunks)ibis-server/tests/routers/v2/connector/test_postgres.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/test_query.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ibis-server/app/routers/v2/connector.py
- ibis-server/tests/routers/v3/connector/postgres/test_query.py
🧰 Additional context used
🧬 Code Definitions (3)
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (4)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (2)
test_query_with_cache_override(174-199)manifest_str(96-97)ibis-server/tests/routers/v2/connector/test_postgres.py (2)
test_query_with_cache_override(204-232)manifest_str(106-107)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/postgres/test_model_substitute.py (1)
manifest_str(27-28)
ibis-server/tests/routers/v2/connector/test_postgres.py (3)
ibis-server/tests/routers/v2/connector/test_mssql.py (1)
manifest_str(72-73)ibis-server/tests/routers/v2/test_relationship_valid.py (2)
manifest_str(61-62)postgres(66-69)ibis-server/tests/routers/v2/connector/test_local_file.py (1)
connection_info(85-89)
ibis-server/app/routers/v3/connector.py (3)
ibis-server/app/query_cache/__init__.py (1)
get_cache_file_timestamp(56-70)ibis-server/app/routers/v2/connector.py (1)
query(41-140)ibis-server/app/model/connector.py (5)
query(56-57)query(72-73)query(112-115)query(143-181)query(197-203)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (9)
ibis-server/tests/routers/v2/connector/test_postgres.py (1)
195-195: Be cautious with hard-coded epoch checks.This assertion relies on the system clock being beyond April 7, 2025. If your test runs in an environment where the date/time is behind that epoch, it may spuriously fail. Consider using relative time checks or mocking the time to ensure consistent test outcomes.
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (2)
74-74: Revisit fixed timestamp comparison.Validating the cache creation time against a fixed timestamp (April 7, 2025) might fail in environments where the local system time is older. Consider using a dynamic or mocked time strategy to avoid edge cases in CI or production tests.
83-109: Comprehensive override logic test.This test properly verifies creation of the cache on the first request and the override step on the second request. It appears thorough, and the header checks confirm the override timestamp is strictly greater than the creation timestamp.
ibis-server/app/routers/v3/connector.py (2)
77-125: Validate concurrency handling for cache override.Using
match (cache_enable, cache_hit, override_cache)is a clean structure. However, concurrent overrides might lead to unexpected states (e.g., partial re-writes). Ensure your underlying filesystem logic or concurrency controls (locks, etc.) suitably handle multiple simultaneous overrides.
136-136: Fallback parameter forwarding looks correct.Forwarding
override_cacheto v2 ensures consistent behavior across connector versions.ibis-server/app/query_cache/__init__.py (4)
2-2: Import oftimeis appropriate.No issues found; this is necessary for generating timestamps.
41-41: Potential concurrency race condition on cache file updates.Switching to
_set_cache_file_nameensures one file per query. However, if multiple clients set the cache for the same query simultaneously, they may step on one another. Consider verifying concurrency behavior or using atomic writes.
56-71: Timestamp extraction gracefully handles errors.Catching
IndexErrorandValueErroravoids crashes if the file name format is unexpected. This defensive approach is sound; no further concerns.
85-91: _get_cache_file_name logic effectively detects existing files.This updated method first checks for an existing matching cache file and reuses it if found. It then creates a new cache file name if necessary. Overall approach is fine for single-writer scenarios.
Summary by CodeRabbit
New Features
Tests