Use timestamp in place of version for query table for expired tables #322
Conversation
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a fallback to timestamp-based live queries when a requested materialization version is expired, and adds guards against using expired versions in various query and metadata methods.
- Adds a cached
available_versionsproperty to fetch non-expired materialization versions. - Implements a timestamp fallback in
query_tablefor expired versions. - Adds expiry validations (raising
ValueError) injoin_query,get_views,get_view_metadata,get_view_schemas, andquery_view, and updates test mocks to cover the new endpoint.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| caveclient/tools/testing.py | Extended mock setup with available_versions endpoint and removed stray whitespace. |
| caveclient/materializationengine.py | Introduced available_versions, get_timestamp caching, expiry checks in queries, and timestamp fallback logic. |
Comments suppressed due to low confidence (1)
caveclient/tools/testing.py:535
- Add test cases for the expired-version fallback path to verify that
query_tablecorrectly uses the timestamp-basedlive_querywhen versions are expired.
responses.add(
| if ( | ||
| materialization_version not in self.available_versions | ||
| and materialization_version is not None | ||
| ): |
There was a problem hiding this comment.
[nitpick] Consider extracting version validation and fallback logic into a shared helper to reduce duplication and maintain consistent behavior across query methods.
caveclient/materializationengine.py
Outdated
|
|
||
| if materialization_version is None: | ||
| materialization_version = self.version | ||
| if materialization_version not in self.available_versions: |
There was a problem hiding this comment.
[nitpick] In join_query, expired versions raise an error while query_table falls back to a timestamp-based query; consider aligning these behaviors or centralizing the fallback logic.
caveclient/materializationengine.py
Outdated
| if version not in self.available_versions: | ||
| raise ValueError( | ||
| f"Materialization version must not be expired for views. " | ||
| f"Available versions: {self.available_versions}" | ||
| ) |
There was a problem hiding this comment.
[nitpick] These expiry checks are duplicated in several methods (get_views, get_view_metadata, etc.); refactor into a common validator to improve maintainability.
| if version not in self.available_versions: | |
| raise ValueError( | |
| f"Materialization version must not be expired for views. " | |
| f"Available versions: {self.available_versions}" | |
| ) | |
| self.validate_version(version) |
When a materialization version is set or requested that is currently expired, fall back on a
live_queryusing the timestamp.Currently this uses
live_queryinstead oflive_live_queryto followsynapse_querybehavior, although this should probably all be replaced with live query. The main difficulty there is remapping all of the join and query logic because every function has a different approach to this. My intuition is that it would be better to do inside thelive_queryfunction itself, and we change the default behavior over to the current weirdly namedlive_live_query.We can discuss if this should also be done in this pull request.